-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
panic when an interpreter error gets unintentionally discarded #130885
Conversation
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annoying thing is that all the MIR transforms need to use discard_interp_error()
instead of ok()
now, and if they forget that that's a lurking ICE.
An alternative approach would be to use TLS to track whether the current interpreter has raised an error, and then on the next interpreter step complain if we are still running. But that means more global state, and we'll have to handle the situation of multiple interpreters on the same thread (they should be well-nested, at least) -- so it's not beautiful either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we configure whether InterpError
s are "explosive" in the InterpCx
? I'd ideally like to see MIR opts just do InterpCx::new(...).ignore_interp_errors()
or something when interfacing with const eval. Or would this still be violating some invariant you'd like to see enforced in MIR opts?
120e9f9
to
b742eda
Compare
This comment has been minimized.
This comment has been minimized.
Ah damn, a clippy failure... those are always so painful to fix due to #78717 :/ |
b742eda
to
4557061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change in theory, but I do fear that it introduces a new class of somewhat easy to introduce ICEs with much of a "guardrail" to avoid it...
@@ -573,7 +587,7 @@ impl<'a, 'tcx> Collector<'a, 'tcx> { | |||
map: &Map<'tcx>, | |||
) -> Option<Const<'tcx>> { | |||
let ty = place.ty(self.local_decls, self.patch.tcx).ty; | |||
let layout = ecx.layout_of(ty).ok()?; | |||
let layout = ecx.layout_of(ty).discard_interp_error()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should just call into self.tcx.layout_of
-- having a layout_of
query return an InterpResult
that needs to be defused seems odd, imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that but then we have to pass the ParamEnv by hand. It doesn't mak the code shorter, overall.
There's a lot of code that has TyCtx + ParamEnv but sadly no commonly used type to pack the two leading to a lot of issues like this. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we configure whether InterpError
s are "explosive" in the InterpCx
? I'd ideally like to see MIR opts just do InterpCx::new(...).ignore_interp_errors()
or something when interfacing with const eval. Or would this still be violating some invariant you'd like to see enforced in MIR opts?
TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(), | ||
TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(), | ||
TrackElem::Field(idx) => { | ||
self.ecx.project_field(op, idx.as_usize()).discard_interp_error() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed: If we can't get rid of all these calls in rustc_mir_transform, perhaps we could at least find a shorter name than discard_interp_error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name must be somewhat scary though or else people might use it in const-eval / miri and not realize what they are doing (and reviewers could easily miss it)...
@@ -557,7 +571,9 @@ impl<'a, 'tcx> TOFinder<'a, 'tcx> { | |||
// `SetDiscriminant` may be a no-op if the assigned variant is the untagged variant | |||
// of a niche encoding. If we cannot ensure that we write to the discriminant, do | |||
// nothing. | |||
let Ok(enum_layout) = self.ecx.layout_of(enum_ty) else { return }; | |||
let Some(enum_layout) = self.ecx.layout_of(enum_ty).discard_interp_error() else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems really easy to mix up by writing let Ok(_)
, as noted above.
I'm not certain the best way to avoid this, except for going way overkill and implementing some custom struct (like not a Result
but a struct wrapper that makes the Result
inaccessible) that implements Try
and forces you to deal with the error by either doing ?
(via Try
) or .discard_interp_error
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's the only type-based alternative I can think of.
Unfortunately not, since the code that creates InterpError has no access to the (Sorry for replying out-of-thread, github as usually has a terrible UI when a review adds a new comment to an existing thread and doesn't make it clear where to reply to a message like this.) |
I think this is just going to be a constant source of ICEs. I can already hear the humming of Matthias's fuzzers. I don't think TLS for a sanity assertion is a problem and would prefer that solution |
What about the "newtyped |
I like it most but assumed from your comments that it was not to your liking. |
Global state is also not to my liking ;) |
Argh I wish we had implicit Ok-wrapping, that would make this so much nicer... |
f07f8d3
to
afa5a66
Compare
Okay, I did that now. :D That made the PR... big... The new
Note that we panic on drop even if the operation succeeded, so while this does risk ICEs, the ICE should occur always when some code is executed, not just sometimes, making it easier to find with tests. And this has indeed found one discarded UB issue in Miri. |
@@ -135,7 +135,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |||
// The only supported flags are GRND_RANDOM and GRND_NONBLOCK, | |||
// neither of which have any effect on our current PRNG. | |||
// See <https://github.com/rust-lang/rust/pull/79196> for a discussion of argument sizes. | |||
let _flags = this.read_scalar(&args[3])?.to_i32(); | |||
let _flags = this.read_scalar(&args[3])?.to_i32()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the discarded UB issue that was now found -- the final ?
was missing.
This comment has been minimized.
This comment has been minimized.
afa5a66
to
59c4659
Compare
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
f5c5e1f
to
fc32fe9
Compare
☔ The latest upstream changes (presumably #131036) made this pull request unmergeable. Please resolve the merge conflicts. |
fc32fe9
to
dc0281f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dc0281f
to
80c5d99
Compare
r=me with or without any more applications of your last change if you want them. |
80c5d99
to
d46aa2a
Compare
d46aa2a
to
c4ce8c1
Compare
@bors r=oli-obk |
…li-obk panic when an interpreter error gets unintentionally discarded One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing. This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded. Fixes rust-lang/miri#3855
…kingjubilee Rollup of 3 pull requests Successful merges: - rust-lang#130885 (panic when an interpreter error gets unintentionally discarded) - rust-lang#131108 (Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags") - rust-lang#131121 (A couple of fixes for dataflow graphviz dumps) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#130885 - RalfJung:interp-error-discard, r=oli-obk panic when an interpreter error gets unintentionally discarded One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing. This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded. Fixes rust-lang/miri#3855
…=jieyouxu mark InterpResult as must_use This was forgotten in rust-lang#130885
Rollup merge of rust-lang#131596 - RalfJung:interp-result-must-use, r=jieyouxu mark InterpResult as must_use This was forgotten in rust-lang#130885
One important invariant of Miri is that when an interpreter error is raised (in particular a UB error), those must not be discarded: it's not okay to just check
foo().is_err()
and then continue executing.This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that if this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to
mem::forget
when an error is deliberately discarded.Fixes rust-lang/miri#3855