-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 core engine: use throw_ub instead of throw_panic #66927
Conversation
This comment has been minimized.
This comment has been minimized.
This is really great work cleaning up those error messages! |
@bors r+ |
📌 Commit 15f159a has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
} | ||
|
||
#[inline] | ||
fn signed_offset<'tcx>(&self, val: u64, i: i64) -> InterpResult<'tcx, u64> { | ||
let (res, over) = self.overflowing_signed_offset(val, i128::from(i)); | ||
if over { throw_panic!(Overflow(mir::BinOp::Add)) } else { Ok(res) } | ||
if over { throw_ub!(PointerArithOverflow) } else { Ok(res) } |
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.
Ohh, so this is now specific enough to be equivalent to LLVM "inbounds GEP" overflowing, nice!
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.
Yes that is the intention.
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 rollup=never |
@bors rollup- |
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
⌛ Testing commit 15f159a with merge 517286a2264b49996c5c79e6a224250e0dd6ffc5... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
Actual error seems to be
Seems spurious? |
@bors retry |
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
☀️ Test successful - checks-azure |
Tested on commit rust-lang/rust@5c5c8eb. Direct link to PR: <rust-lang/rust#66927> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
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