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

Miri: make backtrace function names and spans match up #70590

Merged
merged 7 commits into from
Apr 1, 2020

Conversation

RalfJung
Copy link
Member

Currently, Miri backtraces are a bit confusing:

error: Undefined Behavior: entering unreachable code
  --> tests/compile-fail/never_transmute_void.rs:10:11
   |
10 |     match v {} //~ ERROR  entering unreachable code
   |           ^ entering unreachable code
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
note: inside call to `f` at tests/compile-fail/never_transmute_void.rs:17:5
  --> tests/compile-fail/never_transmute_void.rs:17:5
   |
17 |     f(v); //~ inside call to `f`
   |     ^^^^
   = note: inside call to `main` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
   = note: inside call to closure at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
   = note: inside call to closure at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:130:5

When reading this like a normal backtrace, one would expect that e.g. the backrace involves the "main" function at "libstd/rt.rs:67:34". But that is not actually where we are in the main function, that is where the main function is called.

This is not how backtraces are usually rendered (including e.g. with RUST_BACKTRACE=1). Usually we print next to each function name where inside that function the frame is currently executing, not where the parent frame is executing. With this PR and the Miri side at rust-lang/miri#1283, the backtrace now looks as follows:

error: Undefined Behavior: entering unreachable code
  --> tests/compile-fail/never_transmute_void.rs:10:11
   |
10 |     match v {} //~ ERROR entering unreachable code
   |           ^ entering unreachable code
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: inside `f` at tests/compile-fail/never_transmute_void.rs:10:11
note: inside `main` at tests/compile-fail/never_transmute_void.rs:17:5
  --> tests/compile-fail/never_transmute_void.rs:17:5
   |
17 |     f(v); //~ inside `main`
   |     ^^^^
   = note: inside closure at /home/r/src/rust/rustc/src/libstd/rt.rs:67:34
   = note: inside closure at /home/r/src/rust/rustc/src/libstd/rt.rs:52:73
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6034 ~ std[87db]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/r/src/rust/rustc/src/libstd/sys_common/backtrace.rs:130:5

Now function name and printed line numbers match up in the notes.

This code is partially shared with const-eval, so the change also affects const-eval: instead of printing what is being called at some span, we print which function/constant this span is inside.

With this, we can also remove the span field from Miri's stack frames (which used to track the caller span of that frame, quite confusing), and then get of a whole lot of span arguments that ultimately just served to fill that field (and as a fallback for caller_location, which however was never actually used).

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2020
@@ -65,12 +64,12 @@ impl<'tcx> fmt::Display for FrameInfo<'tcx> {
if tcx.def_key(self.instance.def_id()).disambiguated_data.data
== DefPathData::ClosureExpr
{
write!(f, "inside call to closure")?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original intention was to mean "inside this call to", but I guess that's obvious from the call site being displayed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that text was always attached to the call site, not the inside of the call.

@RalfJung
Copy link
Member Author

Okay I made the caller_location ICE when things are "weird", and stopped skipping the first frame on const-backtraces.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Apr 1, 2020

📌 Commit 96deb95 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Apr 1, 2020

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

@bors 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 Apr 1, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#70535 (Track the finalizing node in the specialization graph)
 - rust-lang#70590 (Miri: make backtrace function names and spans match up)
 - rust-lang#70616 (rustc_target::abi: rename FieldPlacement to FieldsShape.)
 - rust-lang#70626 (cargotest: remove webrender)
 - rust-lang#70649 (clean up E0468 explanation)
 - rust-lang#70662 (compiletest: don't use `std::io::stdout()`, as it bypasses `set_print`.)

Failed merges:

r? @ghost
@eddyb
Copy link
Member

eddyb commented Apr 1, 2020

Why are some frames not rendering code snippets?
Is miri checking is_imported by any chance?

IMO for miri specifically, a full-source backtrace, no matter the crates each frame comes from (only requirement being that we can load the sources), would be good.

@bors bors merged commit b919df2 into rust-lang:master Apr 1, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Apr 2, 2020

IMO for miri specifically, a full-source backtrace, no matter the crates each frame comes from (only requirement being that we can load the sources), would be good.

We used to do that. It took multiple screens worth of height and was horrible.

The check is not is_imported though, it is this one.

@RalfJung RalfJung deleted the miri-backtrace branch April 2, 2020 07:13
bors added a commit to rust-lang/miri that referenced this pull request Apr 2, 2020
Make backtrace function names and spans match up

This is the Miri side of rust-lang/rust#70590.
Fixes #521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants