improve backtrace rendering

[RalfJung]

There are several things I don't like about our current backtrace rendering:

• the first span it mentions is always the same span that the errors occurs at
• we do not say in which thread the error happened if the stack has just a single frame
• = note: BACKTRACE: looks clunky

So here I am trying to improve the situation. The first commit adjusts rendering for the common case to something like this:

error: Undefined Behavior: entering unreachable code
  --> tests/fail/never_transmute_void.rs:LL:CC
   |
LL |         match v.0 {}
   |               ^^^ Undefined Behavior occurred here
   |
   = 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: this is inside `m::f`
note: which got called inside `main`
  --> tests/fail/never_transmute_void.rs:LL:CC
   |
LL |     m::f(v);
   |     ^^^^^^^

The line that says "this is inside" will also show the name of the thread if there is more than 1 thread. That line is shown if there is more than 1 stackframe or more than 1 thread.

The most tricky part is what to do with errors that point are more than one span. If we just add a = note: this is inside m::f``, that looks like it refers to the last span we showed, when it actually refers to the first. We'll want a full note-with-span here, not just a = note. I decided to have that span point at the surrounding function, e.g.:

error: Undefined Behavior: trying to retag from <317> for SharedReadWrite permission at alloc197[0x0], but that tag does not exist in the borrow stack for this location
  --> tests/fail/box-cell-alias.rs:9:14
   |
 9 |     unsafe { (*ptr).set(20) }; //~ ERROR: does not exist in the borrow stack
   |              ^^^^^^ this error occurs as part of retag at alloc197[0x0..0x1]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <317> was created by a SharedReadWrite retag at offsets [0x0..0x1]
  --> tests/fail/box-cell-alias.rs:15:32
   |
15 |     let ptr: *const Cell<u8> = &*val;
   |                                ^^^^^
help: <317> was later invalidated at offsets [0x0..0x1] by a Unique retag
  --> tests/fail/box-cell-alias.rs:16:22
   |
16 |     let res = helper(val, ptr);
   |                      ^^^
note: error occurred inside `helper`
  --> tests/fail/box-cell-alias.rs:7:1
   |
 7 | fn helper(val: Box<Cell<u8>>, ptr: *const Cell<u8>) -> u8 {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: which got called inside `main`
  --> tests/fail/box-cell-alias.rs:16:15
   |
16 |     let res = helper(val, ptr);
   |               ^^^^^^^^^^^^^^^^

However, I am not at all tied to this approach and would be happy about better suggestions.

The second commit adds one more span to the bottom of the backtrace for all stacks except the one of main. That span indicates why this particular function got called:

• For non-main threads, it points at the user-visible span corresponding to thread creation. This helps with Backtraces stop at thread boundaries #4066, though I am not sure if we should consider it to resolve that issue since we only show one span, not a full backtrace (but that may be a better default as full cross-thread backtraces will be huge). The example in the issue now looks as follows:

error: unsupported operation: can't call foreign function `unknown` on OS `linux`
 --> thread.rs:5:21
  |
5 | fn bad() { unsafe { unknown() } }
  |                     ^^^^^^^^^ unsupported operation occurred here
  |
  = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program
  = note: this is on thread `unnamed-1`, inside `bad`
note: which got called indirectly due to this code
 --> thread.rs:8:5
  |
8 |     std::thread::spawn(bad);
  |     ^^^^^^^^^^^^^^^^^^^^^^^

• For global constructors and destructors, it points at the static item that carries the magic linker annotation that turns into a constructor/desstructor. See the new ctor_ub test for an example.
• For TLS destructors it points at the user-visible span corresponding to where the destructor was registered.

@rust-lang/miri please let me know what you think.

[rustbot]

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[saethlin]

The most tricky part is what to do with errors that point are more than one span.

What I've always wanted here is to do the multi-span diagnostics like the borrow checker uses. If we can pull that off, it may make it clearer because the help spans wouldn't be inserted between the top two components of the backtrace.

[saethlin]

The second commit adds one more span to the bottom of the backtrace for all stacks except the one of main. That span indicates why this particular function got called:

I think this change is definitely good, the information is important and if people read every word of the output text it's clear, but we may discover that some additional clarity is needed. I wouldn't rush it though. I like this part of the PR as-is.

[RalfJung]

What I've always wanted here is to do the multi-span diagnostics like the borrow checker uses. If we can pull that off, it may make it clearer because the help spans wouldn't be inserted between the top two components of the backtrace.

We can fairly easily just add multiple labels to the main diagnostic, yeah. However, in my brief experiments, we then lose control over the order in which they are rendered -- they are always shown in source code order. That's not ideal for us, the aliasing model errors work better if we show them in execution order.

[RalfJung]

So as a concrete example, if I just replace the span_help by span_label, here are a data race error and an aliasing model error:

error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-1` and (2) non-atomic write on thread `unnamed-2` at alloc101
  --> tests/fail/data_race/write_write_race.rs:24:13
   |
19 |             *c.0 = 32;
   |             --------- and (1) occurred earlier here
...
24 |             *c.0 = 64; //~ ERROR: Data race detected between (1) non-atomic write on thread `unnamed-1` and (2) non-atomic write on thread `unnamed-2`
   |             ^^^^^^^^^ (2) just happened here
   |
   = 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: this is on thread `unnamed-2`, inside closure
note: which got called indirectly due to this code
  --> tests/fail/data_race/write_write_race.rs:22:18
   |
22 |           let j2 = spawn(move || {
   |  __________________^
23 | |             let c = c; // avoid field capturing
24 | |             *c.0 = 64; //~ ERROR: Data race detected between (1) non-atomic write on thread `unnamed-1` and (2) non-atomic write on thread `unnamed-2`
25 | |         });
   | |__________^

error: Undefined Behavior: trying to retag from <317> for SharedReadWrite permission at alloc197[0x0], but that tag does not exist in the borrow stack for this location
  --> tests/fail/box-cell-alias.rs:9:14
   |
 9 |     unsafe { (*ptr).set(20) }; //~ ERROR: does not exist in the borrow stack
   |              ^^^^^^ this error occurs as part of retag at alloc197[0x0..0x1]
...
15 |     let ptr: *const Cell<u8> = &*val;
   |                                ----- <317> was created by a SharedReadWrite retag at offsets [0x0..0x1]
16 |     let res = helper(val, ptr);
   |                      --- <317> was later invalidated at offsets [0x0..0x1] by a Unique retag
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
   = note: this is inside `helper`
note: which got called inside `main`
  --> tests/fail/box-cell-alias.rs:16:15
   |
16 |     let res = helper(val, ptr);
   |               ^^^^^^^^^^^^^^^^

In this example, the "was later invalidated" works out, but this may be shown above the other two labels.
Furthermore, in general, these multiple spans might not be in the same file; I am not sure what that would look like when rendered.

[rustbot]

☔ The latest upstream changes (possibly #4763) made this pull request unmergeable. Please resolve the merge conflicts.