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

caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!. #65973

Merged
merged 2 commits into from
Nov 6, 2019
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
32 changes: 15 additions & 17 deletions src/libcore/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,29 @@ macro_rules! panic {
/// For details, see `std::macros`.
#[cfg(not(bootstrap))]
#[macro_export]
#[allow_internal_unstable(core_panic, panic_internals)]
#[allow_internal_unstable(core_panic,
// FIXME(anp, eddyb) `core_intrinsics` is used here to allow calling
// the `caller_location` intrinsic, but once `#[track_caller]` is implemented,
// `panicking::{panic, panic_fmt}` can use that instead of a `Location` argument.
core_intrinsics,
)]
#[stable(feature = "core", since = "1.6.0")]
macro_rules! panic {
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I think a previous PR forgot to change the macro in libstd (which is not the same as the one in libcore). cc @anp

Copy link
Member Author

Choose a reason for hiding this comment

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

@anp told me it was intentional, as they use different entry-points, and we can probably wait to have full #[track_caller] before changing libstd.

() => (
$crate::panic!("explicit panic")
);
($msg:expr) => ({
const LOC: &$crate::panic::Location<'_> = &$crate::panic::Location::internal_constructor(
$crate::file!(),
$crate::line!(),
$crate::column!(),
);
$crate::panicking::panic($msg, LOC)
});
($msg:expr) => (
$crate::panicking::panic($msg, $crate::intrinsics::caller_location())
);
($msg:expr,) => (
$crate::panic!($msg)
);
($fmt:expr, $($arg:tt)+) => ({
const LOC: &$crate::panic::Location<'_> = &$crate::panic::Location::internal_constructor(
$crate::file!(),
$crate::line!(),
$crate::column!(),
);
$crate::panicking::panic_fmt($crate::format_args!($fmt, $($arg)+), LOC)
});
($fmt:expr, $($arg:tt)+) => (
$crate::panicking::panic_fmt(
$crate::format_args!($fmt, $($arg)+),
$crate::intrinsics::caller_location(),
)
);
}

/// Asserts that two expressions are equal to each other (using [`PartialEq`]).
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_codegen_ssa/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx: &mut Bx,
span: Span,
) -> OperandRef<'tcx, Bx::Value> {
let caller = bx.tcx().sess.source_map().lookup_char_pos(span.lo());
let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span);
let caller = bx.tcx().sess.source_map().lookup_char_pos(topmost.lo());
let const_loc = bx.tcx().const_caller_location((
Symbol::intern(&caller.file.name.to_string()),
caller.line as u32,
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let intrinsic_name = &self.tcx.item_name(instance.def_id()).as_str()[..];
match intrinsic_name {
"caller_location" => {
let caller = self.tcx.sess.source_map().lookup_char_pos(span.lo());
let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span);
let caller = self.tcx.sess.source_map().lookup_char_pos(topmost.lo());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we de-dup this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be nice if we could pass Span into the query, but @anp ran into trouble with that, I think the result was ICEs from the incremental system.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could just do:

fn caller(&self, span: Span) -> Whatever {
    let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span);
    self.sess.source_map().lookup_char_pos(topmost.lo())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Where would you put this? Also, "caller" is misleading, in fact the name of caller_location is suboptimal (likely due to the RFC assuming an implementation strategy less versatile than the one being followed by @anp).

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb I think you are taking the snippet too literally; rename it to anything you see fit. It would be placed on TyCtxt -- it could also be placed on SourceMap.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, like, there is little generality in it. It's literally "caller_location helper" if it does those two things, and that doesn't feel right to put anywhere outside the query for it (which, again, would ideally take Span).

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose, but it does make sure the logics don't grow apart. I guess we'll have to ensure that through reviews instead.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW "you could just do" is my least favorite way to phrase review feedback.

@eddyb is correct that I had ICEs when using a Span as a parameter to a query. I agree with them that two callsites does not a need for deduplication make (yet).

let location = self.alloc_caller_location(
Symbol::intern(&caller.file.name.to_string()),
caller.line as u32,
Expand Down
13 changes: 1 addition & 12 deletions src/libsyntax_expand/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,18 +954,7 @@ impl<'a> ExtCtxt<'a> {
///
/// Stops backtracing at include! boundary.
pub fn expansion_cause(&self) -> Option<Span> {
let mut expn_id = self.current_expansion.id;
let mut last_macro = None;
loop {
let expn_data = expn_id.expn_data();
// Stop going up the backtrace once include! is encountered
if expn_data.is_root() || expn_data.kind.descr() == sym::include {
break;
}
expn_id = expn_data.call_site.ctxt().outer_expn();
last_macro = Some(expn_data.call_site);
}
last_macro
self.current_expansion.id.expansion_cause()
}

pub fn struct_span_warn<S: Into<MultiSpan>>(&self,
Expand Down
19 changes: 18 additions & 1 deletion src/libsyntax_pos/hygiene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
use crate::GLOBALS;
use crate::{Span, DUMMY_SP};
use crate::edition::Edition;
use crate::symbol::{kw, Symbol};
use crate::symbol::{kw, sym, Symbol};

use rustc_serialize::{Encodable, Decodable, Encoder, Decoder};
use rustc_data_structures::fx::FxHashMap;
Expand Down Expand Up @@ -119,6 +119,23 @@ impl ExpnId {
pub fn outer_expn_is_descendant_of(self, ctxt: SyntaxContext) -> bool {
HygieneData::with(|data| data.is_descendant_of(self, data.outer_expn(ctxt)))
}

/// Returns span for the macro which originally caused this expansion to happen.
///
/// Stops backtracing at include! boundary.
pub fn expansion_cause(mut self) -> Option<Span> {
let mut last_macro = None;
loop {
let expn_data = self.expn_data();
// Stop going up the backtrace once include! is encountered
if expn_data.is_root() || expn_data.kind.descr() == sym::include {
break;
}
self = expn_data.call_site.ctxt().outer_expn();
last_macro = Some(expn_data.call_site);
}
last_macro
}
}

#[derive(Debug)]
Expand Down
14 changes: 13 additions & 1 deletion src/test/ui/rfc-2091-track-caller/caller-location-intrinsic.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
// run-pass

#![feature(core_intrinsics)]

macro_rules! caller_location_from_macro {
() => (core::intrinsics::caller_location());
}

fn main() {
let loc = core::intrinsics::caller_location();
assert_eq!(loc.file(), file!());
assert_eq!(loc.line(), 5);
assert_eq!(loc.line(), 10);
assert_eq!(loc.column(), 15);

// `caller_location()` in a macro should behave similarly to `file!` and `line!`,
// i.e. point to where the macro was invoked, instead of the macro itself.
let loc2 = caller_location_from_macro!();
assert_eq!(loc2.file(), file!());
assert_eq!(loc2.line(), 17);
assert_eq!(loc2.column(), 16);
}