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

libstd uses core::panic::Location where possible. #67137

Merged
merged 5 commits into from
Jan 4, 2020
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
12 changes: 3 additions & 9 deletions src/libcore/macros/mod.rs
Original file line number Diff line number Diff line change
@@ -1,27 +1,21 @@
#[doc(include = "panic.md")]
#[macro_export]
#[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,
const_caller_location,
)]
#[allow_internal_unstable(core_panic, track_caller)]
#[stable(feature = "core", since = "1.6.0")]
macro_rules! panic {
() => (
$crate::panic!("explicit panic")
);
($msg:expr) => (
$crate::panicking::panic($msg, $crate::intrinsics::caller_location())
$crate::panicking::panic($msg)
);
($msg:expr,) => (
$crate::panic!($msg)
);
($fmt:expr, $($arg:tt)+) => (
$crate::panicking::panic_fmt(
$crate::format_args!($fmt, $($arg)+),
$crate::intrinsics::caller_location(),
$crate::panic::Location::caller(),
)
);
}
Expand Down
5 changes: 3 additions & 2 deletions src/libcore/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ use crate::panic::{Location, PanicInfo};
// never inline unless panic_immediate_abort to avoid code
// bloat at the call sites as much as possible
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[track_caller]
#[lang = "panic"] // needed by codegen for panic on overflow and other `Assert` MIR terminators
pub fn panic(expr: &str, location: &Location<'_>) -> ! {
pub fn panic(expr: &str) -> ! {
if cfg!(feature = "panic_immediate_abort") {
unsafe { super::intrinsics::abort() }
}
Expand All @@ -48,7 +49,7 @@ pub fn panic(expr: &str, location: &Location<'_>) -> ! {
// truncation and padding (even though none is used here). Using
// Arguments::new_v1 may allow the compiler to omit Formatter::pad from the
// output binary, saving up to a few kilobytes.
panic_fmt(fmt::Arguments::new_v1(&[expr], &[]), location)
panic_fmt(fmt::Arguments::new_v1(&[expr], &[]), Location::caller())
}

#[cold]
Expand Down
10 changes: 2 additions & 8 deletions src/librustc_expand/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use syntax::ptr::P;
use syntax::source_map::{respan, Spanned};
use syntax::symbol::{kw, sym, Symbol};

use rustc_span::{Pos, Span};
use rustc_span::Span;

impl<'a> ExtCtxt<'a> {
pub fn path(&self, span: Span, strs: Vec<ast::Ident>) -> ast::Path {
Expand Down Expand Up @@ -350,16 +350,10 @@ impl<'a> ExtCtxt<'a> {
}

pub fn expr_fail(&self, span: Span, msg: Symbol) -> P<ast::Expr> {
anp marked this conversation as resolved.
Show resolved Hide resolved
let loc = self.source_map().lookup_char_pos(span.lo());
let expr_file = self.expr_str(span, Symbol::intern(&loc.file.name.to_string()));
let expr_line = self.expr_u32(span, loc.line as u32);
let expr_col = self.expr_u32(span, loc.col.to_usize() as u32 + 1);
let expr_loc_tuple = self.expr_tuple(span, vec![expr_file, expr_line, expr_col]);
let expr_loc_ptr = self.expr_addr_of(span, expr_loc_tuple);
self.expr_call_global(
span,
[sym::std, sym::rt, sym::begin_panic].iter().map(|s| Ident::new(*s, span)).collect(),
vec![self.expr_str(span, msg), expr_loc_ptr],
vec![self.expr_str(span, msg)],
)
}

Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,

fn find_mir_or_eval_fn(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
span: Span,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx>],
ret: Option<(PlaceTy<'tcx>, mir::BasicBlock)>,
Expand All @@ -199,7 +200,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
// Some functions we support even if they are non-const -- but avoid testing
// that for const fn! We certainly do *not* want to actually call the fn
// though, so be sure we return here.
return if ecx.hook_panic_fn(instance, args, ret)? {
return if ecx.hook_panic_fn(span, instance, args)? {
Ok(None)
} else {
throw_unsup_format!("calling non-const function `{}`", instance)
Expand Down
42 changes: 8 additions & 34 deletions src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,47 +366,21 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Returns `true` if an intercept happened.
pub fn hook_panic_fn(
&mut self,
span: Span,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, M::PointerTag>],
_ret: Option<(PlaceTy<'tcx, M::PointerTag>, mir::BasicBlock)>,
) -> InterpResult<'tcx, bool> {
let def_id = instance.def_id();
if Some(def_id) == self.tcx.lang_items().panic_fn() {
// &'static str, &core::panic::Location { &'static str, u32, u32 }
assert!(args.len() == 2);
if Some(def_id) == self.tcx.lang_items().panic_fn()
|| Some(def_id) == self.tcx.lang_items().begin_panic_fn()
{
// &'static str
assert!(args.len() == 1);

let msg_place = self.deref_operand(args[0])?;
let msg = Symbol::intern(self.read_str(msg_place)?);

let location = self.deref_operand(args[1])?;
let (file, line, col) = (
self.mplace_field(location, 0)?,
self.mplace_field(location, 1)?,
self.mplace_field(location, 2)?,
);

let file_place = self.deref_operand(file.into())?;
let file = Symbol::intern(self.read_str(file_place)?);
let line = self.read_scalar(line.into())?.to_u32()?;
let col = self.read_scalar(col.into())?.to_u32()?;
throw_panic!(Panic { msg, file, line, col })
} else if Some(def_id) == self.tcx.lang_items().begin_panic_fn() {
assert!(args.len() == 2);
// &'static str, &(&'static str, u32, u32)
let msg = args[0];
let place = self.deref_operand(args[1])?;
let (file, line, col) = (
self.mplace_field(place, 0)?,
self.mplace_field(place, 1)?,
self.mplace_field(place, 2)?,
);

let msg_place = self.deref_operand(msg.into())?;
let msg = Symbol::intern(self.read_str(msg_place)?);
let file_place = self.deref_operand(file.into())?;
let file = Symbol::intern(self.read_str(file_place)?);
let line = self.read_scalar(line.into())?.to_u32()?;
let col = self.read_scalar(col.into())?.to_u32()?;
let span = self.find_closest_untracked_caller_location().unwrap_or(span);
anp marked this conversation as resolved.
Show resolved Hide resolved
let (file, line, col) = self.location_triple_for_span(span);
throw_panic!(Panic { msg, file, line, col })
} else {
return Ok(false);
Expand Down
12 changes: 9 additions & 3 deletions src/librustc_mir/interpret/intrinsics/caller_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ use crate::interpret::{
};

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Walks up the callstack from the intrinsic's callsite, searching for the first frame which is
/// not `#[track_caller]`.
/// Walks up the callstack from the intrinsic's callsite, searching for the first callsite in a
/// frame which is not `#[track_caller]`. If the first frame found lacks `#[track_caller]`, then
/// `None` is returned and the callsite of the function invocation itself should be used.
crate fn find_closest_untracked_caller_location(&self) -> Option<Span> {
let mut caller_span = None;
for next_caller in self.stack.iter().rev() {
Expand Down Expand Up @@ -54,9 +55,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

pub fn alloc_caller_location_for_span(&mut self, span: Span) -> MPlaceTy<'tcx, M::PointerTag> {
let (file, line, column) = self.location_triple_for_span(span);
self.alloc_caller_location(file, line, column)
}

pub(super) fn location_triple_for_span(&self, span: Span) -> (Symbol, u32, u32) {
let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span);
let caller = self.tcx.sess.source_map().lookup_char_pos(topmost.lo());
self.alloc_caller_location(
(
Symbol::intern(&caller.file.name.to_string()),
caller.line as u32,
caller.col_display as u32 + 1,
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// was used.
fn find_mir_or_eval_fn(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
span: Span,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Self::PointerTag>],
ret: Option<(PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
| ty::InstanceDef::CloneShim(..)
| ty::InstanceDef::Item(_) => {
// We need MIR for this fn
let body = match M::find_mir_or_eval_fn(self, instance, args, ret, unwind)? {
let body = match M::find_mir_or_eval_fn(self, span, instance, args, ret, unwind)? {
Some(body) => body,
None => return Ok(()),
};
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {

fn find_mir_or_eval_fn(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
_span: Span,
_instance: ty::Instance<'tcx>,
_args: &[OpTy<'tcx>],
_ret: Option<(PlaceTy<'tcx>, BasicBlock)>,
Expand Down
1 change: 1 addition & 0 deletions src/libstd/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@
#![feature(thread_local)]
#![feature(toowned_clone_into)]
#![feature(trace_macros)]
#![feature(track_caller)]
#![feature(try_reserve)]
#![feature(unboxed_closures)]
#![feature(untagged_unions)]
Expand Down
18 changes: 16 additions & 2 deletions src/libstd/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! library. Each macro is available for use when linking against the standard
//! library.

#[cfg(bootstrap)]
#[doc(include = "../libcore/macros/panic.md")]
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
Expand All @@ -19,8 +20,21 @@ macro_rules! panic {
$crate::panic!($msg)
});
($fmt:expr, $($arg:tt)+) => ({
$crate::rt::begin_panic_fmt(&$crate::format_args!($fmt, $($arg)+),
&($crate::file!(), $crate::line!(), $crate::column!()))
$crate::rt::begin_panic_fmt(&$crate::format_args!($fmt, $($arg)+))
});
}

#[cfg(not(bootstrap))]
#[doc(include = "../libcore/macros/panic.md")]
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unstable(libstd_sys_internals)]
macro_rules! panic {
() => ({ $crate::panic!("explicit panic") });
($msg:expr) => ({ $crate::rt::begin_panic($msg) });
($msg:expr,) => ({ $crate::panic!($msg) });
($fmt:expr, $($arg:tt)+) => ({
$crate::rt::begin_panic_fmt(&$crate::format_args!($fmt, $($arg)+))
});
}

Expand Down
43 changes: 17 additions & 26 deletions src/libstd/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,17 +313,15 @@ pub fn panicking() -> bool {
#[cold]
// If panic_immediate_abort, inline the abort call,
// otherwise avoid inlining because of it is cold path.
#[cfg_attr(not(feature = "panic_immediate_abort"), track_caller)]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
pub fn begin_panic_fmt(msg: &fmt::Arguments<'_>, file_line_col: &(&'static str, u32, u32)) -> ! {
pub fn begin_panic_fmt(msg: &fmt::Arguments<'_>) -> ! {
if cfg!(feature = "panic_immediate_abort") {
unsafe { intrinsics::abort() }
}

// Just package everything into a `PanicInfo` and continue like libcore panics.
let (file, line, col) = *file_line_col;
let location = Location::internal_constructor(file, line, col);
let info = PanicInfo::internal_constructor(Some(msg), &location);
let info = PanicInfo::internal_constructor(Some(msg), Location::caller());
begin_panic_handler(&info)
}

Expand Down Expand Up @@ -356,6 +354,9 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {

unsafe impl<'a> BoxMeUp for PanicPayload<'a> {
fn take_box(&mut self) -> *mut (dyn Any + Send) {
// We do two allocations here, unfortunately. But (a) they're required with the current
// scheme, and (b) we don't handle panic + OOM properly anyway (see comment in
// begin_panic below).
let contents = mem::take(self.fill());
Box::into_raw(Box::new(contents))
}
Expand All @@ -365,15 +366,9 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
}
}

// We do two allocations here, unfortunately. But (a) they're
// required with the current scheme, and (b) we don't handle
// panic + OOM properly anyway (see comment in begin_panic
// below).

let loc = info.location().unwrap(); // The current implementation always returns Some
let msg = info.message().unwrap(); // The current implementation always returns Some
let file_line_col = (loc.file(), loc.line(), loc.column());
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), &file_line_col);
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), loc);
anp marked this conversation as resolved.
Show resolved Hide resolved
}

/// This is the entry point of panicking for the non-format-string variants of
Expand All @@ -386,19 +381,13 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
// bloat at the call sites as much as possible
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cold]
pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u32)) -> ! {
#[track_caller]
pub fn begin_panic<M: Any + Send>(msg: M, #[cfg(bootstrap)] _: &(&str, u32, u32)) -> ! {
if cfg!(feature = "panic_immediate_abort") {
unsafe { intrinsics::abort() }
}

// Note that this should be the only allocation performed in this code path.
// Currently this means that panic!() on OOM will invoke this code path,
// but then again we're not really ready for panic on OOM anyway. If
// we do start doing this, then we should propagate this allocation to
// be performed in the parent of this thread instead of the thread that's
// panicking.

rust_panic_with_hook(&mut PanicPayload::new(msg), None, file_line_col);
rust_panic_with_hook(&mut PanicPayload::new(msg), None, Location::caller());

struct PanicPayload<A> {
inner: Option<A>,
Expand All @@ -412,6 +401,11 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u3

unsafe impl<A: Send + 'static> BoxMeUp for PanicPayload<A> {
fn take_box(&mut self) -> *mut (dyn Any + Send) {
// Note that this should be the only allocation performed in this code path. Currently
// this means that panic!() on OOM will invoke this code path, but then again we're not
// really ready for panic on OOM anyway. If we do start doing this, then we should
// propagate this allocation to be performed in the parent of this thread instead of the
// thread that's panicking.
let data = match self.inner.take() {
Some(a) => Box::new(a) as Box<dyn Any + Send>,
None => process::abort(),
Expand All @@ -436,10 +430,8 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u3
fn rust_panic_with_hook(
payload: &mut dyn BoxMeUp,
message: Option<&fmt::Arguments<'_>>,
file_line_col: &(&str, u32, u32),
location: &Location<'_>,
) -> ! {
let (file, line, col) = *file_line_col;

let panics = update_panic_count(1);

// If this is the third nested call (e.g., panics == 2, this is 0-indexed),
Expand All @@ -456,8 +448,7 @@ fn rust_panic_with_hook(
}

unsafe {
let location = Location::internal_constructor(file, line, col);
let mut info = PanicInfo::internal_constructor(message, &location);
let mut info = PanicInfo::internal_constructor(message, location);
HOOK_LOCK.read();
match HOOK {
// Some platforms (like wasm) know that printing to stderr won't ever actually
Expand Down
1 change: 1 addition & 0 deletions src/test/mir-opt/retain-never-const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#![feature(const_panic)]
#![feature(never_type)]
#![warn(const_err)]

struct PrintName<T>(T);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
//[thin]compile-flags: -C lto=thin
//[fat]compile-flags: -C lto=fat

#![feature(core_panic, panic_internals)]
#![feature(core_panic)]

// (For some reason, reproducing the LTO issue requires pulling in std
// explicitly this way.)
Expand Down Expand Up @@ -50,9 +50,7 @@ fn main() {
}

let _guard = Droppable;
let s = "issue-64655-allow-unwind-when-calling-panic-directly.rs";
let location = core::panic::Location::internal_constructor(s, 17, 4);
core::panicking::panic("???", &location);
core::panicking::panic("???");
});

let wait = handle.join();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@

#[inline(never)]
#[track_caller]
fn defeat_const_prop() -> &'static core::panic::Location<'static> {
fn codegen_caller_loc() -> &'static core::panic::Location<'static> {
core::panic::Location::caller()
}

macro_rules! caller_location_from_macro {
() => (defeat_const_prop());
() => (codegen_caller_loc());
}

fn main() {
let loc = defeat_const_prop();
let loc = codegen_caller_loc();
assert_eq!(loc.file(), file!());
assert_eq!(loc.line(), 16);
assert_eq!(loc.column(), 15);
Expand Down
Loading