Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support forwarding caller location through trait object method call #81360

Merged
merged 3 commits into from
Jul 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ pub type DepNode = rustc_query_system::dep_graph::DepNode<DepKind>;
// required that their size stay the same, but we don't want to change
// it inadvertently. This assert just ensures we're aware of any change.
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
static_assert_size!(DepNode, 17);
static_assert_size!(DepNode, 18);

#[cfg(not(any(target_arch = "x86", target_arch = "x86_64")))]
static_assert_size!(DepNode, 24);
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,10 @@ rustc_queries! {
desc { |tcx| "looking up const stability of `{}`", tcx.def_path_str(def_id) }
}

query should_inherit_track_caller(def_id: DefId) -> bool {
desc { |tcx| "computing should_inherit_track_caller of `{}`", tcx.def_path_str(def_id) }
}

query lookup_deprecation_entry(def_id: DefId) -> Option<DeprecationEntry> {
desc { |tcx| "checking whether `{}` is deprecated", tcx.def_path_str(def_id) }
}
Expand Down
52 changes: 48 additions & 4 deletions compiler/rustc_middle/src/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,9 @@ impl<'tcx> InstanceDef<'tcx> {

pub fn requires_caller_location(&self, tcx: TyCtxt<'_>) -> bool {
match *self {
InstanceDef::Item(def) => {
tcx.codegen_fn_attrs(def.did).flags.contains(CodegenFnAttrFlags::TRACK_CALLER)
InstanceDef::Item(ty::WithOptConstParam { did: def_id, .. })
| InstanceDef::Virtual(def_id, _) => {
tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::TRACK_CALLER)
Comment on lines -230 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, nice, I like that this is all it takes for virtual calls to start taking panic::Location.
I assume the rest of the changes is there "just" to handle edge cases.

}
_ => false,
}
Expand Down Expand Up @@ -403,7 +404,7 @@ impl<'tcx> Instance<'tcx> {
def_id: DefId,
substs: SubstsRef<'tcx>,
) -> Option<Instance<'tcx>> {
debug!("resolve(def_id={:?}, substs={:?})", def_id, substs);
debug!("resolve_for_vtable(def_id={:?}, substs={:?})", def_id, substs);
let fn_sig = tcx.fn_sig(def_id);
let is_vtable_shim = !fn_sig.inputs().skip_binder().is_empty()
&& fn_sig.input(0).skip_binder().is_param(0)
Expand All @@ -412,7 +413,50 @@ impl<'tcx> Instance<'tcx> {
debug!(" => associated item with unsizeable self: Self");
Some(Instance { def: InstanceDef::VtableShim(def_id), substs })
} else {
Instance::resolve_for_fn_ptr(tcx, param_env, def_id, substs)
Instance::resolve(tcx, param_env, def_id, substs).ok().flatten().map(|mut resolved| {
match resolved.def {
InstanceDef::Item(def) => {
// We need to generate a shim when we cannot guarantee that
// the caller of a trait object method will be aware of
// `#[track_caller]` - this ensures that the caller
// and callee ABI will always match.
//
// The shim is generated when all of these conditions are met:
//
// 1) The underlying method expects a caller location parameter
// in the ABI
if resolved.def.requires_caller_location(tcx)
// 2) The caller location parameter comes from having `#[track_caller]`
// on the implementation, and *not* on the trait method.
&& !tcx.should_inherit_track_caller(def.did)
// If the method implementation comes from the trait definition itself
// (e.g. `trait Foo { #[track_caller] my_fn() { /* impl */ } }`),
// then we don't need to generate a shim. This check is needed because
// `should_inherit_track_caller` returns `false` if our method
// implementation comes from the trait block, and not an impl block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// implementation comes from the trait block, and not an impl block
// implementation comes from the trait block, and not an impl block.

&& !matches!(
tcx.opt_associated_item(def.did),
Some(ty::AssocItem {
container: ty::AssocItemContainer::TraitContainer(_),
..
})
)
{
debug!(
" => vtable fn pointer created for function with #[track_caller]"
);
resolved.def = InstanceDef::ReifyShim(def.did);
}
}
InstanceDef::Virtual(def_id, _) => {
debug!(" => vtable fn pointer created for virtual call");
resolved.def = InstanceDef::ReifyShim(def_id);
}
_ => {}
}

resolved
})
}
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_typeck/src/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ pub fn provide(providers: &mut Providers) {
generator_kind,
codegen_fn_attrs,
collect_mod_item_types,
should_inherit_track_caller,
..*providers
};
}
Expand Down Expand Up @@ -2652,7 +2653,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
let attrs = tcx.get_attrs(id);

let mut codegen_fn_attrs = CodegenFnAttrs::new();
if should_inherit_track_caller(tcx, id) {
if tcx.should_inherit_track_caller(id) {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::TRACK_CALLER;
}

Expand Down
56 changes: 47 additions & 9 deletions src/test/ui/rfc-2091-track-caller/tracked-trait-obj.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,60 @@

trait Tracked {
#[track_caller]
fn handle(&self) {
fn track_caller_trait_method(&self, line: u32, col: u32) {
let location = std::panic::Location::caller();
assert_eq!(location.file(), file!());
// we only call this via trait object, so the def site should *always* be returned
assert_eq!(location.line(), line!() - 4);
assert_eq!(location.column(), 5);
// The trait method definition is annotated with `#[track_caller]`,
// so caller location information will work through a method
// call on a trait object
assert_eq!(location.line(), line, "Bad line");
assert_eq!(location.column(), col, "Bad col");
}

fn track_caller_not_on_trait_method(&self);

#[track_caller]
fn track_caller_through_self(self: Box<Self>, line: u32, col: u32);
}

impl Tracked for () {}
impl Tracked for u8 {}
impl Tracked for () {
// We have `#[track_caller]` on the implementation of the method,
// but not on the definition of the method in the trait. Therefore,
// caller location information will *not* work through a method call
// on a trait object. Instead, we will get the location of this method
#[track_caller]
fn track_caller_not_on_trait_method(&self) {
let location = std::panic::Location::caller();
assert_eq!(location.file(), file!());
assert_eq!(location.line(), line!() - 3);
assert_eq!(location.column(), 5);
}
Comment on lines +22 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why the file!/line! don't fall-back to their usual behaviour of providing precisely the line/column they are on, rather than that of where the method definition item begins? As implemented right now, this seems like a 3rd new behaviour for these macros.

(Though with the behaviour as it is, I'm sure that the bikeshed could go on ad infinitum here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the current behavior whenever we need to generate a shim:

trait Foo {
    #[track_caller] fn bar(&self);
}

impl Foo for () {
    fn bar(&self) {
        panic!("Where does this end up?")
    }
}

fn foo(val: &dyn Foo) {
    val.bar();
}

fn main() {
    foo(&());
}

On the latest nightly, this prints thread 'main' panicked at 'Where does this end up?', src/main.rs:6:5

I agree that it would probably be good to change this, but I think that could be done separately.


// We don't have a `#[track_caller]` attribute, but
// `#[track_caller]` is present on the trait definition,
// so we'll still get location information
fn track_caller_through_self(self: Box<Self>, line: u32, col: u32) {
let location = std::panic::Location::caller();
assert_eq!(location.file(), file!());
// The trait method definition is annotated with `#[track_caller]`,
// so caller location information will work through a method
// call on a trait object
assert_eq!(location.line(), line, "Bad line");
assert_eq!(location.column(), col, "Bad col");
}
}

fn main() {
let tracked: &dyn Tracked = &5u8;
tracked.handle();
let tracked: &dyn Tracked = &();
// The column is the start of 'track_caller_trait_method'
tracked.track_caller_trait_method(line!(), 13);

const TRACKED: &dyn Tracked = &();
TRACKED.handle();
// The column is the start of 'track_caller_trait_method'
TRACKED.track_caller_trait_method(line!(), 13);
TRACKED.track_caller_not_on_trait_method();

// The column is the start of `track_caller_through_self`
let boxed: Box<dyn Tracked> = Box::new(());
boxed.track_caller_through_self(line!(), 11);
}