interpret: do not prune requires_caller_location stack frames quite so early #98549
Conversation
rustbot
added
the
T-compiler
|
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ oh yea, I was wondering where the detailed UB trace when recently |
|
📌 Commit bbcd2e2be94dfd5f13a170940cde31558e691db4 has been approved by |
bors
added
S-waiting-on-bors
| --> $SRC_DIR/core/src/mem/maybe_uninit.rs:LL:COL | ||
| | | ||
| LL | intrinsics::assert_inhabited::<T>(); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | aborted execution: attempted to instantiate uninhabited type `!` | ||
| | inside `MaybeUninit::<!>::assume_init` at $SRC_DIR/core/src/mem/maybe_uninit.rs:LL:COL | ||
| | inside `_BAD1` at $DIR/assert-type-intrinsics.rs:14:9 | ||
| | | ||
| ::: $DIR/assert-type-intrinsics.rs:13:5 | ||
| | | ||
| LL | / const _BAD1: () = unsafe { | ||
| LL | | MaybeUninit::<!>::uninit().assume_init(); | ||
| | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!` | ||
| LL | | }; |
There was a problem hiding this comment.
So you think this is better than before?
I honestly think hiding these frames here (as we did before) makes the error look better, and was wondering if it would make sense to do this in Miri, too (unless MIRI_BACKTRACE=full is set), because the user rarely wants to see stdlib frames.
There was a problem hiding this comment.
hmm... I did not consider what happens when libstd source is not available. Likely that will make the first block of this diagnostic just disappear silently, along with the backtrace. So users will be lacking most of the information they need in order to debug the issue. That's an orthogonal issue to what you're asking though.
so... we don't prune regular backtraces either... and that has been a frequent issue imo. All those iterator, option and whatnot paths in the backtrace are really not helping anyone.
There was a problem hiding this comment.
Even with std sources, I feel like pointing at assume_init is better than pointing at its insides.
so... we don't prune regular backtraces either...
True. But it is much easier for CTFE and Miri to do that so we might as well?
There was a problem hiding this comment.
Even with std sources, I feel like pointing at assume_init is better than pointing at its insides.
I very much agree. Otherwise the error points users at code that they've never even seen, when they make a simple error. That's exactly why we're using #[track_caller] in so many places in std, to make sure a useful location is reported.
There was a problem hiding this comment.
we don't prune regular backtraces either...
This isn't really a regular backtrace, though. This is part of a compiler diagonstic, where we are (and can be) much friendlier.
There was a problem hiding this comment.
Okay, I took out the last commit.
@oli-obk is that fine for you?
|
@bors r- |
bors
added
S-waiting-on-author
RalfJung
force-pushed
the
interpret-stacktraces
branch
from
bbcd2e2
to
852a111
Compare
RalfJung
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 852a111 has been approved by |
bors
added
S-waiting-on-bors
…li-obk interpret: do not prune requires_caller_location stack frames quite so early rust-lang#87000 made the interpreter skip `caller_location` frames for its stacktraces and `cur_span`. However, those functions are used for much more than just panic reporting, and e.g. when Miri reports UB somewhere, it probably wants to point inside `caller_location` frames. (And if it did not, it would want to have its own logic to decide that, not be forced into it by the core interpreter engine.) This fixes some rare ICEs in Miri that say "we should never pop more than one frame at once". So let's remove all `caller_location` logic from the core interpreter, and instead move it to CTFE error reporting. This does not change user-visible behavior. That's the first commit. We might additionally want to change CTFE error reporting to treat panics differently from other errors: only prune `caller_location` frames for panics. The second commit does that. But honestly I am not sure if this is an improvement. r? `@oli-obk`
Rollup of 7 pull requests Successful merges: - rust-lang#97423 (Simplify memory ordering intrinsics) - rust-lang#97542 (Use typed indices in argument mismatch algorithm) - rust-lang#97786 (Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths) - rust-lang#98277 (Fix trait object reborrow suggestion) - rust-lang#98525 (Add regression test for rust-lang#79224) - rust-lang#98549 (interpret: do not prune requires_caller_location stack frames quite so early) - rust-lang#98603 (Some borrowck diagnostic fixes) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Rustup Fix our stacktrace after rust-lang/rust#98549. Now we can control whether `caller_location` should be pruned!
Rustup Fix our stacktrace after rust-lang/rust#98549. Now we can control whether `caller_location` should be pruned!
#87000 made the interpreter skip
caller_locationframes for its stacktraces andcur_span. However, those functions are used for much more than just panic reporting, and e.g. when Miri reports UB somewhere, it probably wants to point insidecaller_locationframes. (And if it did not, it would want to have its own logic to decide that, not be forced into it by the core interpreter engine.) This fixes some rare ICEs in Miri that say "we should never pop more than one frame at once".So let's remove all
caller_locationlogic from the core interpreter, and instead move it to CTFE error reporting. This does not change user-visible behavior. That's the first commit.We might additionally want to change CTFE error reporting to treat panics differently from other errors: only prune
caller_locationframes for panics. The second commit does that. But honestly I am not sure if this is an improvement.r? @oli-obk