-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Track span of function in method calls, and use this in #[track_caller] #73182
Changes from all commits
28946b3
6e4d0b4
f69a2a6
bbf497b
e5f3b94
9d36fa3
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 |
---|---|---|
|
@@ -1371,7 +1371,7 @@ pub struct Expr<'hir> { | |
|
||
// `Expr` is used a lot. Make sure it doesn't unintentionally get bigger. | ||
#[cfg(target_arch = "x86_64")] | ||
rustc_data_structures::static_assert_size!(Expr<'static>, 64); | ||
rustc_data_structures::static_assert_size!(Expr<'static>, 72); | ||
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. You'll want to do a perf run for this even though it's only 8 bytes. |
||
|
||
impl Expr<'_> { | ||
pub fn precedence(&self) -> ExprPrecedence { | ||
|
@@ -1568,12 +1568,14 @@ pub enum ExprKind<'hir> { | |
/// and the remaining elements are the rest of the arguments. | ||
/// Thus, `x.foo::<Bar, Baz>(a, b, c, d)` is represented as | ||
/// `ExprKind::MethodCall(PathSegment { foo, [Bar, Baz] }, [x, a, b, c, d])`. | ||
/// The final `Span` represents the span of the function and arguments | ||
/// (e.g. `foo::<Bar, Baz>(a, b, c, d)` in `x.foo::<Bar, Baz>(a, b, c, d)` | ||
/// | ||
/// To resolve the called method to a `DefId`, call [`type_dependent_def_id`] with | ||
/// the `hir_id` of the `MethodCall` node itself. | ||
/// | ||
/// [`type_dependent_def_id`]: ../ty/struct.TypeckTables.html#method.type_dependent_def_id | ||
MethodCall(&'hir PathSegment<'hir>, Span, &'hir [Expr<'hir>]), | ||
MethodCall(&'hir PathSegment<'hir>, Span, &'hir [Expr<'hir>], Span), | ||
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. Having a 4-field tuple variant where two fields have the same type is not great. Maybe now is the time to switch to named fields? I really wish there was an automated way of doing this. 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. I thought about that, but I wanted to avoid creating even more churn. This field is ignored everywhere exept in a few specific cases, so I don't think it's too bad. 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 doesn't seem necessary as |
||
/// A tuple (e.g., `(a, b, c, d)`). | ||
Tup(&'hir [Expr<'hir>]), | ||
/// A binary operation (e.g., `a + b`, `a * b`). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1131,6 +1131,9 @@ pub enum TerminatorKind<'tcx> { | |
/// `true` if this is from a call in HIR rather than from an overloaded | ||
/// operator. True for overloaded function call. | ||
from_hir_call: bool, | ||
/// This `Span` is the span of the function, without the dot and receiver | ||
/// (e.g. `foo(a, b)` in `x.foo(a, b)` | ||
fn_span: Span, | ||
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. Needs a doc comment. Also, are we trying to move away from spans inside MIR data structures for incremental? Seems like 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. We're storing an almost identical span ( 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. I wonder if this should be the span in |
||
}, | ||
|
||
/// Jump to the target if the condition has the expected value, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
use std::convert::TryFrom; | ||
|
||
use rustc_hir::lang_items::PanicLocationLangItem; | ||
use rustc_middle::mir::TerminatorKind; | ||
use rustc_middle::ty::subst::Subst; | ||
use rustc_span::{Span, Symbol}; | ||
use rustc_target::abi::LayoutOf; | ||
|
@@ -14,19 +15,32 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
/// Walks up the callstack from the intrinsic's callsite, searching for the first callsite in a | ||
/// frame which is not `#[track_caller]`. | ||
crate fn find_closest_untracked_caller_location(&self) -> Span { | ||
self.stack() | ||
let frame = self | ||
.stack() | ||
.iter() | ||
.rev() | ||
// Find first non-`#[track_caller]` frame. | ||
.find(|frame| !frame.instance.def.requires_caller_location(*self.tcx)) | ||
.find(|frame| { | ||
debug!( | ||
"find_closest_untracked_caller_location: checking frame {:?}", | ||
frame.instance | ||
); | ||
!frame.instance.def.requires_caller_location(*self.tcx) | ||
}) | ||
// Assert that there is always such a frame. | ||
.unwrap() | ||
.current_source_info() | ||
// Assert that the frame we look at is actually executing code currently | ||
// (`current_source_info` is None when we are unwinding and the frame does | ||
// not require cleanup). | ||
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 comment was lost, but it still seems important. |
||
.unwrap() | ||
.span | ||
.unwrap(); | ||
let loc = frame.loc.unwrap(); | ||
let block = &frame.body.basic_blocks()[loc.block]; | ||
assert_eq!(block.statements.len(), loc.statement_index); | ||
debug!( | ||
"find_closest_untracked_caller_location:: got terminator {:?} ({:?})", | ||
block.terminator(), | ||
block.terminator().kind | ||
); | ||
if let TerminatorKind::Call { fn_span, .. } = block.terminator().kind { | ||
return fn_span; | ||
} | ||
unreachable!(); | ||
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. I think this 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.
|
||
} | ||
|
||
/// Allocate a `const core::panic::Location` with the provided filename and line/column numbers. | ||
|
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.
Is this
Span
not available as part of theIdent
that is the name of the method, inPathSegment
?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.
This
Span
also includes the opening and closing parenthesis, so thePathSegment
andVec<P<Expr>>
are insufficient to reconstruct it exactly.