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

make intern_const_alloc_recursive return error #78742

Merged
merged 2 commits into from
Nov 5, 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: 9 additions & 3 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ impl From<ErrorHandled> for InterpErrorInfo<'_> {
}
}

impl From<ErrorReported> for InterpErrorInfo<'_> {
fn from(err: ErrorReported) -> Self {
InterpError::InvalidProgram(InvalidProgramInfo::AlreadyReported(err)).into()
}
}

impl<'tcx> From<InterpError<'tcx>> for InterpErrorInfo<'tcx> {
fn from(kind: InterpError<'tcx>) -> Self {
let capture_backtrace = tls::with_opt(|tcx| {
Expand Down Expand Up @@ -115,8 +121,8 @@ pub enum InvalidProgramInfo<'tcx> {
/// Cannot compute this constant because it depends on another one
/// which already produced an error.
ReferencedConstant,
/// Abort in case type errors are reached.
TypeckError(ErrorReported),
/// Abort in case errors are already reported.
AlreadyReported(ErrorReported),
/// An error occurred during layout computation.
Layout(layout::LayoutError<'tcx>),
/// An invalid transmute happened.
Expand All @@ -129,7 +135,7 @@ impl fmt::Display for InvalidProgramInfo<'_> {
match self {
TooGeneric => write!(f, "encountered overly generic constant"),
ReferencedConstant => write!(f, "referenced constant has errors"),
TypeckError(ErrorReported) => {
AlreadyReported(ErrorReported) => {
write!(f, "encountered constants with type errors, stopping evaluation")
Copy link
Member

Choose a reason for hiding this comment

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

The error message text does not match the new variant name.

}
Layout(ref err) => write!(f, "{}", err),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/const_eval/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ impl<'tcx> ConstEvalErr<'tcx> {
err_inval!(Layout(LayoutError::Unknown(_))) | err_inval!(TooGeneric) => {
return ErrorHandled::TooGeneric;
}
err_inval!(TypeckError(error_reported)) => {
err_inval!(AlreadyReported(error_reported)) => {
return ErrorHandled::Reported(error_reported);
}
// We must *always* hard error on these, even if the caller wants just a lint.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
None => InternKind::Constant,
}
};
intern_const_alloc_recursive(ecx, intern_kind, ret);
intern_const_alloc_recursive(ecx, intern_kind, ret)?;

debug!("eval_body_using_ecx done: {:?}", *ret);
Ok(ret)
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_mir/src/const_eval/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ pub(crate) fn const_caller_location(
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all(), false);

let loc_place = ecx.alloc_caller_location(file, line, col);
intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place);
if intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place).is_err() {
bug!("intern_const_alloc_recursive should not error in this case")
}
ConstValue::Scalar(loc_place.ptr)
}

Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
if let Some(def) = def.as_local() {
if self.tcx.has_typeck_results(def.did) {
if let Some(error_reported) = self.tcx.typeck_opt_const_arg(def).tainted_by_errors {
throw_inval!(TypeckError(error_reported))
throw_inval!(AlreadyReported(error_reported))
}
}
}
Expand Down Expand Up @@ -527,8 +527,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(Some(instance)) => Ok(instance),
Ok(None) => throw_inval!(TooGeneric),

// FIXME(eddyb) this could be a bit more specific than `TypeckError`.
Err(error_reported) => throw_inval!(TypeckError(error_reported)),
// FIXME(eddyb) this could be a bit more specific than `AlreadyReported`.
Err(error_reported) => throw_inval!(AlreadyReported(error_reported)),
}
}

Expand Down
7 changes: 6 additions & 1 deletion compiler/rustc_mir/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

use super::validity::RefTracking;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::ErrorReported;
use rustc_hir as hir;
use rustc_middle::mir::interpret::InterpResult;
use rustc_middle::ty::{self, layout::TyAndLayout, Ty};
Expand Down Expand Up @@ -285,11 +286,13 @@ pub enum InternKind {
/// tracks where in the value we are and thus can show much better error messages.
/// Any errors here would anyway be turned into `const_err` lints, whereas validation failures
/// are hard errors.
#[tracing::instrument(skip(ecx))]
pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
ecx: &mut InterpCx<'mir, 'tcx, M>,
intern_kind: InternKind,
ret: MPlaceTy<'tcx>,
) where
) -> Result<(), ErrorReported>
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I had quite deliberately made interning infallible a while ago.^^ Is this really necessary or just yet another hack to work around #76064 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

well... infallible or not, it was reporting errors, so it wasn't really infallible, it just didn't report its failures.

The problem isn't #76064 in this case, even if the repro test would also be fixed by a fix for #76064 . This issue also occurs for

const FOO: *const u32 = {
    let x = 42;
    &x
};

which typechecks just fine. Basically any dangling pointer was causing this.

As you noted in #78655 (comment) that error should stop compilation. We could change the interning error to a delay_span_bug and have it get reported by validation instead, but then validation needs to have a look at relocations within padding, which it currently does not.

Copy link
Member

Choose a reason for hiding this comment

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

well... infallible or not, it was reporting errors, so it wasn't really infallible, it just didn't report its failures.

That's fair.

Basically any dangling pointer was causing this.

It was causing an error, but usually not an ICE -- IIRC we even had a testcase for that.

But I guess what I really care about is that interning does not return an InterpError, and that is still the case. Actually telling the outside world whether we did span_err seems reasonable and seems to be a more general pattern.

So, LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

then validation needs to have a look at relocations within padding, which it currently does not.

Interning is in a much better place to do this, so unless we really want the error to point to where in the value the problem occurs, I think the current approach works better.

where
'tcx: 'mir,
{
let tcx = ecx.tcx;
Expand Down Expand Up @@ -405,12 +408,14 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
// Codegen does not like dangling pointers, and generally `tcx` assumes that
// all allocations referenced anywhere actually exist. So, make sure we error here.
ecx.tcx.sess.span_err(ecx.tcx.span, "encountered dangling pointer in final constant");
return Err(ErrorReported);
} else if ecx.tcx.get_global_alloc(alloc_id).is_none() {
// We have hit an `AllocId` that is neither in local or global memory and isn't
// marked as dangling by local memory. That should be impossible.
span_bug!(ecx.tcx.span, "encountered unknown alloc id {:?}", alloc_id);
}
}
Ok(())
}

impl<'mir, 'tcx: 'mir, M: super::intern::CompileTimeMachine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Early-return cases.
let val_val = match val.val {
ty::ConstKind::Param(_) | ty::ConstKind::Bound(..) => throw_inval!(TooGeneric),
ty::ConstKind::Error(_) => throw_inval!(TypeckError(ErrorReported)),
ty::ConstKind::Error(_) => throw_inval!(AlreadyReported(ErrorReported)),
ty::ConstKind::Unevaluated(def, substs, promoted) => {
let instance = self.resolve(def, substs)?;
return Ok(self.eval_to_allocation(GlobalId { instance, promoted })?.into());
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/dangling-alloc-id-ice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ union Foo<'a> {
long_live_the_unit: &'static (),
}

const FOO: &() = { //~ ERROR it is undefined behavior to use this value
const FOO: &() = {
//~^ ERROR encountered dangling pointer in final constant
let y = ();
unsafe { Foo { y: &y }.long_live_the_unit }
Expand Down
15 changes: 1 addition & 14 deletions src/test/ui/consts/dangling-alloc-id-ice.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,5 @@ LL | | unsafe { Foo { y: &y }.long_live_the_unit }
LL | | };
| |__^

error[E0080]: it is undefined behavior to use this value
--> $DIR/dangling-alloc-id-ice.rs:9:1
|
LL | / const FOO: &() = {
LL | |
LL | | let y = ();
LL | | unsafe { Foo { y: &y }.long_live_the_unit }
LL | | };
| |__^ type validation failed: encountered a dangling reference (use-after-free)
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.

error: aborting due to 2 previous errors
error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
10 changes: 10 additions & 0 deletions src/test/ui/consts/issue-78655.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const FOO: *const u32 = { //~ ERROR encountered dangling pointer in final constant
let x;
&x //~ ERROR borrow of possibly-uninitialized variable: `x`
};

fn main() {
let FOO = FOO;
//~^ ERROR could not evaluate constant pattern
//~| ERROR could not evaluate constant pattern
}
30 changes: 30 additions & 0 deletions src/test/ui/consts/issue-78655.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
error[E0381]: borrow of possibly-uninitialized variable: `x`
--> $DIR/issue-78655.rs:3:5
|
LL | &x
| ^^ use of possibly-uninitialized `x`

error: encountered dangling pointer in final constant
--> $DIR/issue-78655.rs:1:1
|
LL | / const FOO: *const u32 = {
LL | | let x;
LL | | &x
LL | | };
| |__^

error: could not evaluate constant pattern
--> $DIR/issue-78655.rs:7:9
|
LL | let FOO = FOO;
| ^^^

error: could not evaluate constant pattern
--> $DIR/issue-78655.rs:7:9
|
LL | let FOO = FOO;
| ^^^

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0381`.