remove Panic variant from InterpError #68969

RalfJung · 2020-02-08T21:37:33Z

The interpreter engine itself does not raise Panic errors any more, so remove them from the error enum. Instead, const-prop and const-eval have to do their own handling of panics.

I used the opportunity to refactor the const-eval error handling a bit to use the MachineStop variant.

Also, in const-prop I could do some cleanup as now, no more lints are being reported in use_ecx. However, I am quite puzzled by why exactly the linting there works the way it does -- the code can certainly be cleaned up more, but I don't know enough of the intent to do that. I left some questions for the most confusing parts, but for now behavior should be unchanged by this PR (so, all that weirdness I am asking about is pre-existing and merely maintained here). Cc @wesleywiser

Fixes #66902

r? @oli-obk

RalfJung · 2020-02-08T21:38:07Z

src/librustc/mir/interpret/error.rs

@@ -242,6 +251,9 @@ impl<'tcx> From<InterpError<'tcx>> for InterpErrorInfo<'tcx> {
    }
 }

+/// Information about a panic.
+///
+/// FIXME: this is not actually an InterpError, and should probably be moved to another module.


@oli-obk any proposals where to put this? The Debug impl of this is still used by const-eval and const-prop.

In that case you can move it to src/librustc_mir/interpret. Maybe create an error handling submodule that for now just contains this type.

Ah I should have been clearer, the type itself is also used in MIR.^^ So the default place would be rustc/mir/mod.rs I think. But should the Debug impl also be there?

oh... well, if it's part of the MIR, you need to keep the Debug impl around for debug printing, as most MIR datastructures can be debug printed.

rustc/mir/mod.rs seems fine to me

@oli-obk All right, I moved it. But I also realized the name is kind of bad... this is not about panics in general, it is about assertions in particular. It's just that AssertMessage is already a type alias for the specific PanicInfo<Operand> that actually gets embedded into MIR.

So what would be a better name? Maybe AssertInfo? Though the distinction between AssertInfo and AssertMessage is entirely not obvious.

src/librustc_mir/transform/const_prop.rs

RalfJung · 2020-02-08T21:40:47Z

src/librustc_mir/transform/const_prop.rs

+    fn report_panic_as_lint(&self, source_info: SourceInfo, panic: PanicInfo<u64>) -> Option<()> {
+        // Somewhat convoluted way to re-use the CTFE error reporting code.
+        let lint_root = self.lint_root(source_info)?;
+        let error = InterpError::MachineStop(Box::new(format!("{:?}", panic)));
+        let mut diagnostic = error_to_const_error(&self.ecx, error.into());
+        diagnostic.span = source_info.span; // fix the span
+        diagnostic.report_as_lint(
+            self.tcx.at(source_info.span),
+            "this expression will panic at runtime",
+            lint_root,
+            None,
+        );
+        None
+    }


So this is quite a mess. I basically took the error reporting code from use_ecx, and glued it directly together with the code that actually raises the error -- but at that point there is not really any reason left to even create an InterpError, except that I don't know what to do instead.

And "this expression will panic at runtime" is factually wrong; run-time panics are caused by Assert terminators which do not go through this code-path. But that confusion is pre-existing.

Probably, this should not use ConstEvalErr at all, but I have no idea which part of ConstEvalErr::struct_generic is geared specifically towards const-prop and should be put here (and removed in its original source).

Probably the best thing would be to take the error reporting code from visit_terminator, put it into a helper function, and use that. That would also make things more consistent in terms of errors being reported on MIR with and without overflow checks.

I am holding off on this though because I fear that might create some dead code in ConstEvalErr::struct_generic -- was some code there added just to handle const-prop? If yes, that should be removed then.

And "this expression will panic at runtime" is factually wrong; run-time panics are caused by Assert terminators which do not go through this code-path. But that confusion is pre-existing.

right, it used to be true at the start. Maybe we could just go with a generic "this expression contains erroneous code"?

Probably, this should not use ConstEvalErr at all, but I have no idea which part of ConstEvalErr::struct_generic is geared specifically towards const-prop and should be put here (and removed in its original source).

You can just report a lint directly via self.tcx.sess.span_lint. After that you should be able to make MachineError a bug! in report_as_lint again.

All this should probably be in separate PRs though, at least the lint and the message change are likely to be noisy.

I am holding off on this though because I fear that might create some dead code in ConstEvalErr::struct_generic -- was some code there added just to handle const-prop? If yes, that should be removed then.

There shouldn't be, we ignore everything but panic-likes in const prop

There shouldn't be, we ignore everything but panic-likes in const prop

I was referring to this mess:

rust/src/librustc/mir/interpret/error.rs

Lines 146 to 167 in 4d1241f

let mut err = if let (Some(lint_root), false) = (lint_root, must_error) {

let hir_id = self

.stacktrace

.iter()

.rev()

.filter_map(|frame| frame.lint_root)

.next()

.unwrap_or(lint_root);

tcx.struct_span_lint_hir(

rustc_session::lint::builtin::CONST_ERR,

hir_id,

tcx.span,

message,

)

} else if must_error {

struct_error(tcx, &self.error.to_string())

} else {

struct_error(tcx, message)

};

if !must_error {

err.span_label(self.span, self.error.to_string());

}

Is some of this const-prop hackery, or is this all still needed even if const-prop uses another code path?

I was more thinking of leaving CTFE with const_err and making const-prop emit some other error... that error is not necessarily about "consts" anyway.

the relevant rfc rust-lang/rfcs#1229 mentions "erroneous code", though that's very vague.

(I am also not sure if the const-prop errors really should be deny-by-default but that is a separate concern.)

They are only deny-by-default because they share the same lint name, after splitting it making it warn-by-default seems ok. Also promoteds should share that lint.

Also promoteds should share that lint.

Aaaahhh, but then it'll become messy again because this means the ConstEvalError thing will also emit that lint.^^

Also, I think even ignoring that concern I only agree for promoteds that were promoted from a function, not those promoted from a const.

So, what about this mess, can anyone add comments to explain it and maybe remove what is not needed any more when const-prop switches away?

Also, I think even ignoring that concern I only agree for promoteds that were promoted from a function, not those promoted from a const.

yes that makes sense

So, what about this mess, can anyone add comments to explain it and maybe remove what is not needed any more when const-prop switches away?

We can't change anything here as far as I can tell. This is all needed for CTFE. const prop just made use of the "lint only" case since it only ever fed panic errors into this function.

Whatever "lint only case" means...

I added a commit that (a) cleans up the control flow a bit and (b) adds some FIXMEs fur stuff that is weird and has no comment explaining it.

src/librustc_mir/transform/const_prop.rs

src/librustc_mir/const_eval/machine.rs

RalfJung · 2020-02-10T14:11:31Z

src/librustc_mir/transform/const_prop.rs

-        // then we'll already catch overflow when we evaluate the `Assert` statement
-        // in MIR. However, if overflow checking is disabled, then there won't be any
-        // `Assert` statement and so we have to do additional checking here.
-        if !overflow_check {


I also just realized this entire !overflow_check makes no sense...

It gets initialized via

let overflow_check = self.tcx.sess.overflow_checks();

but whether this MIR has overflow checks is not a global property! In release builds, function bodies have no overflow checks but const bodies still have overflow checks.

Ah, but maybe const-prop does not run on const bodies? At least with associated const though, one could conceivably still want to optimize the MIR even though it cannot be executed... if we don't that will at the very least mean that exceeding_bitshifts does not happen there. (EDIT: nope, this really does lead to duplicates: #69020 (comment))

And the other part that makes no sense is causing #69020: even with !overflow_check, division and remainder still get overflow checks.

And of course, for addition, subtraction and multiplication in MIR with overflow checks, check_binary_op will anyway not be called because those will use CheckedBinOp, not BinOp! So, the check is mostly dead code anyway.

We could keep piling up more and more special cases, but this is getting ridiculous. I feel this requires a fundamentally different approach.

So... my gut feeling is that we should stop emitting errors at the Assert terminators (which only sometimes exist), and always use the arithmetic operation itself as the source of the error (as that will always exist in one form or another, for debug or release builds). Does that sound sensible? (Yes that is a separate PR.)

hmm... That will also solve the situation where we have code paths only run by const prop, because const eval will now also take them. So essentially assert terminators become gotos for miri? Can this work for length checks and such, too?

I have no idea what you mean... I was talking only about const-prop not emitting errors at Assert terminators, not Miri in general. Miri in general should implement the Rust Abstract Machine, and there arithmetic is wrapping.

Ok then: const-prop skips assert terminators and treats them like Goto?

Ok then: const-prop skips assert terminators and treats them like Goto?

But doesn't that make "this may panic at runtime" messaging a complete lie?
Only Assert terminators are the ultimate source of truth for that.

I guess there are two categories of Assert terminators:

always emitted (indexing, division by 0, etc.)

overflow checks, only in debug mode or with a flag enabled etc.

I don't care too much about what happens with the latter, but I think the former should still be relied upon.

But doesn't that make "this may panic at runtime" messaging a complete lie?
Only Assert terminators are the ultimate source of truth for that.

Fun story: currently, we do not write "this will panic at runtime" when we get our info from Assert terminators. We only write that when we do not get it from assert terminators.

I don't care too much about what happens with the latter, but I think the former should still be relied upon.

I can see that. However, then we need some other way to fix #61821. Though now that I am thinking about this... considering that const-prop currently stays silent when evaluating a div-by-0 (relying on the panic instead), it probably makes most sense to also make const-prop stay silent when evaluating an out-of-bounds access (relying on the panic instead).

Fun story: currently, we do not write "this will panic at runtime" when we get our info from Assert terminators. We only write that when we do not get it from assert terminators.

That's... the wrongest it can be, wow!

considering that const-prop currently stays silent when evaluating a div-by-0 (relying on the panic instead), it probably makes most sense to also make const-prop stay silent when evaluating an out-of-bounds access (relying on the panic instead).

Yes, all "Assert guards UB" situations should be treated the same.

On a second thought, this makes no sense... we already don't show out-of-bounds indexing in const-prop. The error there is duplicated because the MIR got duplicated into the promoted. The fix will have to be something promoted-specific.

src/librustc_mir/transform/const_prop.rs

oli-obk · 2020-02-10T15:02:23Z

@bors r+

bors · 2020-02-10T15:02:25Z

📌 Commit 4e0cecea5090d9ba0df4a8963247df5282d1aa1a has been approved by oli-obk

RalfJung · 2020-02-10T15:06:17Z

@bors r-

I'd like to resolve #68969 (comment) before landing.

src/librustc/mir/mod.rs

src/librustc/mir/interpret/error.rs

bors · 2020-02-11T17:48:22Z

☔ The latest upstream changes (presumably #68725) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2020-02-12T18:49:00Z

@oli-obk I updated the comments with what you said, and rebased. Please check.

bors · 2020-02-13T01:42:43Z

☔ The latest upstream changes (presumably #69105) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2020-02-13T08:10:52Z

r=me after another rebase

- comment for special handling of Shl/Shr - reference a PR

RalfJung · 2020-02-13T10:24:25Z

@bors r=oli-obk

bors · 2020-02-13T10:24:26Z

📌 Commit ed2f22c has been approved by oli-obk

src/librustc_mir/transform/const_prop.rs

RalfJung · 2020-02-13T10:26:30Z

@bors r=oli-obk

bors · 2020-02-13T10:26:31Z

📌 Commit 33ba83c has been approved by oli-obk

bors · 2020-02-13T12:53:51Z

⌛ Testing commit 33ba83c with merge d538b80...

@wesleywiser

remove Panic variant from InterpError The interpreter engine itself does not raise `Panic` errors any more, so remove them from the error enum. Instead, const-prop and const-eval have to do their own handling of panics. I used the opportunity to refactor the const-eval error handling a bit to use the `MachineStop` variant. Also, in const-prop I could do some cleanup as now, no more lints are being reported in `use_ecx`. However, I am quite puzzled by why exactly the linting there works the way it does -- the code can certainly be cleaned up more, but I don't know enough of the intent to do that. I left some questions for the most confusing parts, but for now behavior should be unchanged by this PR (so, all that weirdness I am asking about is pre-existing and merely maintained here). Cc @wesleywiser Fixes #66902 r? @oli-obk

bors · 2020-02-13T15:56:26Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing d538b80 to master...

rust-highfive · 2020-02-13T15:57:54Z

📣 Toolstate changed by #68969!

Tested on commit d538b80.
Direct link to PR: #68969

💔 miri on windows: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

@oli-obk

Tested on commit rust-lang/rust@d538b80. Direct link to PR: <rust-lang/rust#68969> 💔 miri on windows: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra). 💔 miri on linux: test-fail → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra). 🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

@oli-obk

miri: fix exact_div Turns out `exact_div` was relying on the broken behavior of `Rem` for `int_min % -1` that was fixed in rust-lang#69002. This PR fixes `exact_div`. Inside rustc, `exact_div` is only used in a single place where the divisor is always positive (in `ptr_offset_from`), so we cannot test the fix in rustc. The Miri test suite covers this through the `exact_div` intrinsic, though (and it is how I found out). One step to rust-lang#69117 (then we also need to address build failures introduced by rust-lang#68969) r? @oli-obk

rust-highfive assigned oli-obk Feb 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2020

RalfJung commented Feb 8, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Show resolved Hide resolved

RalfJung commented Feb 8, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

RalfJung commented Feb 8, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

wesleywiser reviewed Feb 9, 2020

View reviewed changes

src/librustc_mir/const_eval/machine.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request Feb 9, 2020

miri: improve and simplify overflow detection #69002

Merged

RalfJung commented Feb 10, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 10, 2020

oli-obk reviewed Feb 11, 2020

View reviewed changes

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved

oli-obk reviewed Feb 11, 2020

View reviewed changes

src/librustc/mir/interpret/error.rs Outdated Show resolved Hide resolved

oli-obk reviewed Feb 11, 2020

View reviewed changes

src/librustc/mir/interpret/error.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request Feb 11, 2020

"exceeding_bitshifts" lint does not work in associated consts #69021

Closed

RalfJung force-pushed the dont-panic branch from 9550de2 to c06ad27 Compare February 12, 2020 18:41

RalfJung added 3 commits February 13, 2020 10:54

remove Panic variant from InterpError

0633a0e

const-prop: handle overflow_check consistently for all operators

c5709ff

remove PanicInfo::Panic variant that MIR does not use or need

f3ff02b

RalfJung added 5 commits February 13, 2020 10:56

no need for hook_panic_fn to return a bool

17a8cfd

move PanicInfo to mir module

6457b29

small cleanup in ConstEvalErr::struct_generic

55339f2

improve comments:

7e7d1c3

- comment for special handling of Shl/Shr - reference a PR

rename PanicInfo -> AssertKind

ed2f22c

RalfJung force-pushed the dont-panic branch from d2d47e1 to ed2f22c Compare February 13, 2020 10:06

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 13, 2020

RalfJung commented Feb 13, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

fix typo

33ba83c

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 13, 2020

bors merged commit 33ba83c into rust-lang:master Feb 13, 2020

RalfJung mentioned this pull request Feb 13, 2020

miri: fix exact_div #69126

Merged

RalfJung deleted the dont-panic branch February 13, 2020 16:35

RalfJung mentioned this pull request Feb 15, 2020

Unify and improve const-prop lints #69185

Merged

	let mut err = if let (Some(lint_root), false) = (lint_root, must_error) {
	let hir_id = self
	.stacktrace
	.iter()
	.rev()
	.filter_map(\|frame\| frame.lint_root)
	.next()
	.unwrap_or(lint_root);
	tcx.struct_span_lint_hir(
	rustc_session::lint::builtin::CONST_ERR,
	hir_id,
	tcx.span,
	message,
	)
	} else if must_error {
	struct_error(tcx, &self.error.to_string())
	} else {
	struct_error(tcx, message)
	};
	if !must_error {
	err.span_label(self.span, self.error.to_string());
	}

remove Panic variant from InterpError #68969

remove Panic variant from InterpError #68969

Conversation

RalfJung commented Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Feb 10, 2020

bors commented Feb 10, 2020

RalfJung commented Feb 10, 2020

bors commented Feb 11, 2020

RalfJung commented Feb 12, 2020

bors commented Feb 13, 2020

oli-obk commented Feb 13, 2020

RalfJung commented Feb 13, 2020

bors commented Feb 13, 2020

RalfJung commented Feb 13, 2020

bors commented Feb 13, 2020

bors commented Feb 13, 2020

bors commented Feb 13, 2020

rust-highfive commented Feb 13, 2020

RalfJung commented Feb 8, 2020 •

edited

Loading

RalfJung Feb 8, 2020 •

edited

Loading

RalfJung Feb 10, 2020 •

edited

Loading

RalfJung Feb 10, 2020 •

edited

Loading