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

ConstEval/ConstProp: get rid of Panic error kind #66902

Closed
RalfJung opened this issue Nov 30, 2019 · 4 comments · Fixed by #68969
Closed

ConstEval/ConstProp: get rid of Panic error kind #66902

RalfJung opened this issue Nov 30, 2019 · 4 comments · Fixed by #68969
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 30, 2019

With the Miri engine now having support for properly executing panics, a panicking program does not really constitute an "interpreter error" any more. So we should get rid of the InterpError::Panic variant.

  • We'll need to decide what else to do with the throw_panic! that still exist. This one I think should be throw_ub! instead; same for the "division/remainder by zero" in this file. With well-formed MIR I think those are all unreachable, but I see no harm in letting Miri also support some "reasonable" MIR that rustc would never emit (such as omitting the bounds check on an array access, or the div-by-zero check). Overflowing pointer arithmetic should also be UB I think.
  • ConstEval should probably format an error message directly when a panic occurs (here or in the new assert_panic hook added by Miri engine: proper support for Assert MIR terminators #66874). This could be propagated outwards via a new MachineError(String) variant of InterpError, if nothing else fits.
  • I am not sure what ConstProp should do. The throw_panic! mentioned above are, I think, currently actually hit by ConstProp -- but maybe those same errors can be better shown by determining that the condition of an Assert terminator is constant, and indeed that might explain why we currently sometimes have duplicate error messages. It also contains a throw_panic! here.

Cc @oli-obk @wesleywiser

@jonas-schievink jonas-schievink added A-const-eval Area: Constant evaluation (MIR interpretation) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 30, 2019
@wesleywiser
Copy link
Member

ConstProp -- but maybe those same errors can be better shown by determining that the condition of an Assert terminator is constant

Unless I'm not understanding what you're saying, that's exactly how it currently works in ConstProp:

TerminatorKind::Assert { expected, ref msg, ref mut cond, .. } => {
if let Some(value) = self.eval_operand(&cond, source_info) {
trace!("assertion on {:?} should be {:?}", value, expected);
let expected = ScalarMaybeUndef::from(Scalar::from_bool(*expected));
let value_const = self.ecx.read_scalar(value).unwrap();
if expected != value_const {

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2019

that's exactly how it currently works in ConstProp

Yeah I think that is what I imagined. So I will start by experimenting with replacing the throw_panic! that we have with throw_ub!.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2019

I will start by experimenting with replacing the throw_panic! that we have with throw_ub!.

PR opened at #66927.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2019

With well-formed MIR I think those are all unreachable

Turns out that assumption is wrong: the MIR of promoteds actually is not well-formed in this sense; it can contain unchecked array/slice accesses.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2019
add reusable MachineStop variant to Miri engine error enum

Replace the Miri-tool-specific `Exit` error variant with something dynamically typed that all clients of the Miri engine can use.

r? @oli-obk
Cc rust-lang#66902
Centril added a commit to Centril/rust that referenced this issue Dec 3, 2019
Miri core engine: use throw_ub instead of throw_panic

See rust-lang#66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
Centril added a commit to Centril/rust that referenced this issue Dec 7, 2019
Miri core engine: use throw_ub instead of throw_panic

See rust-lang#66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
bors added a commit that referenced this issue Dec 7, 2019
Miri core engine: use throw_ub instead of throw_panic

See #66902 for context: panicking is not really an "interpreter error", but just part of a normal Rust execution. This is a first step towards removing the `InterpError::Panic` variant: the core Miri engine does not use it any more.

ConstProp and ConstEval still use it, though. This will be addressed in future PRs.

From what I can tell, all the error messages this removes are actually duplicates.

r? @oli-obk @wesleywiser
@bors bors closed this as completed in d538b80 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants