-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) | ||||||
} | ||||||
_ => false, | ||||||
} | ||||||
|
@@ -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) | ||||||
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
&& !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 | ||||||
}) | ||||||
} | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason why the (Though with the behaviour as it is, I'm sure that the bikeshed could go on ad infinitum here) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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); | ||
} |
There was a problem hiding this comment.
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.