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

Miri: improve spans of required_const failures #75396

Merged
merged 6 commits into from
Aug 12, 2020
Merged
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: 8 additions & 4 deletions src/librustc_mir/const_eval/machine.rs
Original file line number Diff line number Diff line change
@@ -301,12 +301,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
Ok(())
}

fn after_stack_push(ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
// Enforce stack size limit.
if !ecx.tcx.sess.recursion_limit().value_within_limit(ecx.stack().len()) {
#[inline(always)]
fn init_frame_extra(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx>,
) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
// Enforce stack size limit. Add 1 because this is run before the new frame is pushed.
if !ecx.tcx.sess.recursion_limit().value_within_limit(ecx.stack().len() + 1) {
throw_exhaust!(StackFrameLimitReached)
} else {
Ok(())
Ok(frame)
}
}

53 changes: 33 additions & 20 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@ use rustc_middle::ty::layout::{self, TyAndLayout};
use rustc_middle::ty::{
self, query::TyCtxtAt, subst::SubstsRef, ParamEnv, Ty, TyCtxt, TypeFoldable,
};
use rustc_span::{source_map::DUMMY_SP, Pos, Span};
use rustc_span::{Pos, Span};
use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size, TargetDataLayout};

use super::{
@@ -83,9 +83,11 @@ pub struct Frame<'mir, 'tcx, Tag = (), Extra = ()> {
////////////////////////////////////////////////////////////////////////////////
// Current position within the function
////////////////////////////////////////////////////////////////////////////////
/// If this is `None`, we are unwinding and this function doesn't need any clean-up.
/// Just continue the same as with `Resume`.
pub loc: Option<mir::Location>,
/// If this is `Err`, we are not currently executing any particular statement in
/// this frame (can happen e.g. during frame initialization, and during unwinding on
/// frames without cleanup code).
/// We basically abuse `Result` as `Either`.
pub(super) loc: Result<mir::Location, Span>,
}

/// What we store about a frame in an interpreter backtrace.
@@ -189,7 +191,14 @@ impl<'mir, 'tcx, Tag> Frame<'mir, 'tcx, Tag> {
impl<'mir, 'tcx, Tag, Extra> Frame<'mir, 'tcx, Tag, Extra> {
/// Return the `SourceInfo` of the current instruction.
pub fn current_source_info(&self) -> Option<&mir::SourceInfo> {
self.loc.map(|loc| self.body.source_info(loc))
self.loc.ok().map(|loc| self.body.source_info(loc))
}

pub fn current_span(&self) -> Span {
match self.loc {
Ok(loc) => self.body.source_info(loc).span,
Err(span) => span,
}
}
}

@@ -324,11 +333,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

#[inline(always)]
pub fn cur_span(&self) -> Span {
self.stack()
.last()
.and_then(|f| f.current_source_info())
.map(|si| si.span)
.unwrap_or(self.tcx.span)
self.stack().last().map(|f| f.current_span()).unwrap_or(self.tcx.span)
}

#[inline(always)]
@@ -640,7 +645,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// first push a stack frame so we have access to the local substs
let pre_frame = Frame {
body,
loc: Some(mir::Location::START),
loc: Err(body.span), // Span used for errors caused during preamble.
return_to_block,
return_place,
// empty local array, we fill it in below, after we are inside the stack frame and
@@ -654,9 +659,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
for const_ in &body.required_consts {
let span = const_.span;
let const_ =
self.subst_from_current_frame_and_normalize_erasing_regions(const_.literal);
self.const_to_op(const_, None)?;
self.const_to_op(const_, None).map_err(|err| {
// If there was an error, set the span of the current frame to this constant.
// Avoiding doing this when evaluation succeeds.
self.frame_mut().loc = Err(span);
err
})?;
}

// Locals are initially uninitialized.
@@ -683,8 +694,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
// done
self.frame_mut().locals = locals;

M::after_stack_push(self)?;
self.frame_mut().loc = Ok(mir::Location::START);
Comment on lines 697 to +698
Copy link
Member Author

Choose a reason for hiding this comment

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

It's about the order of these two lines: for errors that happen during after_stack_push, so we want to use the span of the first statement, or the span of the entire body? IMO the latter makes more sense, the first statement has nothing to do with this.

For the stack overflow, probably the best span to use would be that of the call site.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, the body's span is good here, though it's actually the call site's fault. But lets leave such refactorings (using the call site span) to the future, I remember trying something similar a while back and it just complicated the span scheme even more

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this wouldn't even be that hard I think... we can just call cur_span before pushing the new frame, then we should get the call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just not clear to me that we always want the call site. The callee could also be responsible for the problem -- in fact I'd usually think they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

at runtime the decision whose job it is to create the stack frame is an ABI decision, but that doesn't really make sense for CTFE. But I also think your alluding to a different thing? If an required_const will fail regardless of the concrete generic parameters, then that's indeed the function's fault and not the caller's

Copy link
Member Author

Choose a reason for hiding this comment

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

If an required_const will fail regardless of the concrete generic parameters, then that's indeed the function's fault and not the caller's

Yes, that is one possible scenario I had in mind where blaming the caller seems wrong.

info!("ENTERING({}) {}", self.frame_idx(), self.frame().instance);

Ok(())
@@ -693,7 +704,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Jump to the given block.
#[inline]
pub fn go_to_block(&mut self, target: mir::BasicBlock) {
self.frame_mut().loc = Some(mir::Location { block: target, statement_index: 0 });
self.frame_mut().loc = Ok(mir::Location { block: target, statement_index: 0 });
}

/// *Return* to the given `target` basic block.
@@ -715,7 +726,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// If `target` is `None`, that indicates the function does not need cleanup during
/// unwinding, and we will just keep propagating that upwards.
pub fn unwind_to_block(&mut self, target: Option<mir::BasicBlock>) {
self.frame_mut().loc = target.map(|block| mir::Location { block, statement_index: 0 });
self.frame_mut().loc = match target {
Some(block) => Ok(mir::Location { block, statement_index: 0 }),
None => Err(self.frame_mut().body.span),
};
}

/// Pops the current frame from the stack, deallocating the
@@ -743,8 +757,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
assert_eq!(
unwinding,
match self.frame().loc {
None => true,
Some(loc) => self.body().basic_blocks()[loc.block].is_cleanup,
Ok(loc) => self.body().basic_blocks()[loc.block].is_cleanup,
Err(_) => true,
}
);

@@ -920,14 +934,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
pub fn generate_stacktrace(&self) -> Vec<FrameInfo<'tcx>> {
let mut frames = Vec::new();
for frame in self.stack().iter().rev() {
let source_info = frame.current_source_info();
let lint_root = source_info.and_then(|source_info| {
let lint_root = frame.current_source_info().and_then(|source_info| {
match &frame.body.source_scopes[source_info.scope].local_data {
mir::ClearCrossCrate::Set(data) => Some(data.lint_root),
mir::ClearCrossCrate::Clear => None,
}
});
let span = source_info.map_or(DUMMY_SP, |source_info| source_info.span);
let span = frame.current_span();

frames.push(FrameInfo { span, instance: frame.instance, lint_root });
}
3 changes: 1 addition & 2 deletions src/librustc_mir/interpret/intrinsics/caller_location.rs
Original file line number Diff line number Diff line change
@@ -30,8 +30,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Assert that there is always such a frame.
.unwrap();
// 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).
// (`loc` is `Err` when we are unwinding and the frame does not require cleanup).
let loc = frame.loc.unwrap();
// If this is a `Call` terminator, use the `fn_span` instead.
let block = &frame.body.basic_blocks()[loc.block];
8 changes: 0 additions & 8 deletions src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
@@ -409,12 +409,4 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
) -> Self::PointerTag {
()
}

#[inline(always)]
fn init_frame_extra(
_ecx: &mut InterpCx<$mir, $tcx, Self>,
frame: Frame<$mir, $tcx>,
) -> InterpResult<$tcx, Frame<$mir, $tcx>> {
Ok(frame)
}
}
6 changes: 3 additions & 3 deletions src/librustc_mir/interpret/step.rs
Original file line number Diff line number Diff line change
@@ -47,8 +47,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

let loc = match self.frame().loc {
Some(loc) => loc,
None => {
Ok(loc) => loc,
Err(_) => {
// We are unwinding and this fn has no cleanup code.
// Just go on unwinding.
trace!("unwinding: skipping frame");
@@ -283,7 +283,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

self.eval_terminator(terminator)?;
if !self.stack().is_empty() {
if let Some(loc) = self.frame().loc {
if let Ok(loc) = self.frame().loc {
info!("// executing {:?}", loc.block);
}
}
8 changes: 8 additions & 0 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
@@ -281,6 +281,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
Ok(())
}

#[inline(always)]
fn init_frame_extra(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
frame: Frame<'mir, 'tcx>,
) -> InterpResult<'tcx, Frame<'mir, 'tcx>> {
Ok(frame)
}

#[inline(always)]
fn stack(
ecx: &'a InterpCx<'mir, 'tcx, Self>,
10 changes: 5 additions & 5 deletions src/test/ui/consts/const-err-multi.stderr
Original file line number Diff line number Diff line change
@@ -24,17 +24,17 @@ error: any use of this value will cause an error
--> $DIR/const-err-multi.rs:7:19
|
LL | pub const C: u8 = A as u8;
| ------------------^^^^^^^-
| ------------------^-------
| |
| referenced constant has errors

error: any use of this value will cause an error
--> $DIR/const-err-multi.rs:9:19
--> $DIR/const-err-multi.rs:9:24
|
LL | pub const D: i8 = 50 - A;
| ------------------^^^^^^-
| |
| referenced constant has errors
| -----------------------^-
| |
| referenced constant has errors

error: aborting due to 4 previous errors

4 changes: 2 additions & 2 deletions src/test/ui/consts/const-eval/erroneous-const.rs
Original file line number Diff line number Diff line change
@@ -8,8 +8,8 @@ impl<T> PrintName<T> {
}

const fn no_codegen<T>() {
if false { //~ERROR evaluation of constant value failed
let _ = PrintName::<T>::VOID;
if false {
let _ = PrintName::<T>::VOID; //~ERROR evaluation of constant value failed
}
}

8 changes: 3 additions & 5 deletions src/test/ui/consts/const-eval/erroneous-const.stderr
Original file line number Diff line number Diff line change
@@ -25,12 +25,10 @@ LL | #![warn(const_err, unconditional_panic)]
| ^^^^^^^^^

error[E0080]: evaluation of constant value failed
--> $DIR/erroneous-const.rs:11:5
--> $DIR/erroneous-const.rs:12:17
|
LL | / if false {
LL | | let _ = PrintName::<T>::VOID;
LL | | }
| |_____^ referenced constant has errors
LL | let _ = PrintName::<T>::VOID;
| ^^^^^^^^^^^^^^^^^^^^ referenced constant has errors

error[E0080]: could not evaluate static initializer
--> $DIR/erroneous-const.rs:16:22
4 changes: 2 additions & 2 deletions src/test/ui/consts/uninhabited-const-issue-61744.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// build-fail

pub const unsafe fn fake_type<T>() -> T {
hint_unreachable() //~ ERROR evaluation of constant value failed
hint_unreachable()
}

pub const unsafe fn hint_unreachable() -> ! {
fake_type()
fake_type() //~ ERROR evaluation of constant value failed
}

trait Const {
9 changes: 4 additions & 5 deletions src/test/ui/consts/uninhabited-const-issue-61744.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
error[E0080]: evaluation of constant value failed
--> $DIR/uninhabited-const-issue-61744.rs:4:5
--> $DIR/uninhabited-const-issue-61744.rs:8:5
|
LL | hint_unreachable()
| ^^^^^^^^^^^^^^^^^^
| ------------------
| |
| reached the configured maximum number of stack frames
| inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
| inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
| inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
| inside `fake_type::<!>` at $DIR/uninhabited-const-issue-61744.rs:4:5
@@ -72,8 +70,9 @@ LL | hint_unreachable()
| inside `fake_type::<i32>` at $DIR/uninhabited-const-issue-61744.rs:4:5
...
LL | fake_type()
| -----------
| ^^^^^^^^^^^
| |
| reached the configured maximum number of stack frames
| inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5
| inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5
| inside `hint_unreachable` at $DIR/uninhabited-const-issue-61744.rs:8:5