Use #[track_caller] for no_threads.rs for Mutex

[colelawrence]

Sorry for the lack of context, but I'm experiencing a terrible issue which is made significantly harder in browser WASM to debug as a result of this not being marked as #[tracked_caller].

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[cuviper]

Have you tested this? I think it will just point you to the caller here instead:

rust/library/std/src/sync/mutex.rs

Line 317 in c1dba09

self.inner.lock();

[colelawrence]

I have not had the opportunity to test this. I haven't set up Rust to compile on my set up. I haven't done it before, but I wanted to get the conversation started on this since it dramatically impacts the understandability of errors in my runtime environment.

If we put track_caller only on the parent, will we be able to truncate all nested stack traces? How would you recommend making these choices for the project?

[cuviper]

If we put track_caller only on the parent, will we be able to truncate all nested stack traces?

I don't think #[track_caller] will affect the stack trace at all, only the location in that first "panicked at file:line:col" message. We can push that up the stack with a chain of #[track_caller] attributes, but it adds some cost for hidden location arguments, which I don't think we want for other targets.

We usually manage that backtrace report ourselves, and RUST_BACKTRACE=1 does print a more limited range by default, with RUST_BACKTRACE=full available for everything. But for WebAssembly, this is coming from your runtime (chrome, wasmtime, etc.), and I don't know if we have any influence over that.

[colelawrence]

If we simply put track_caller into these two mentioned places, then the WASM debugging experience becomes much better for this mutex issue.

I discussed this exact issue with another member in my Rust community, and they recognized this as helpful for the same kind of issues I experienced. So, how can this tradeoff be evaluated?

Am I correct that the options are:

1. ask the WASM runtime team for V8/Chromium dev tools to improve their usage of full backtrace info
2. adding track_caller into the stdlib like this so the stack starts at the place in user code
3. More deeply learn if there is a way to make Rust WASM + V8 debug tools communicate their stacks better and apply that to my codebase only
4. When cases come up I can try to reverse map the debug symbols in the stack trace provided by v8 into my build artifacts somehow (possible, I think, but I will need to understand much more about the compiler)

Thanks for the insights so far! I figured out that in my codebase, when I compile in release mode, there is no lock contention, so that's my current solution, as it's the easiest way.

[cuviper]

The panic message and the stack trace are orthogonal, even though they're both dealing with code locations.

2. adding track_caller into the stdlib like this so the stack starts at the place in user code

Again, this won't change the stack trace at all, only the location given in the panic message.

Here's a playground you can try with this code:

fn main() {
    foo();
}

#[track_caller]
fn foo() {
    bar();
}

#[track_caller]
fn bar() {
    panic!();
}

If you change those track_caller attributes and run it, you'll see it change in thread 'main' panicked at src/main.rs:2:5:, but the stack will still look something like:

stack backtrace:
   0: rust_begin_unwind
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_fmt
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:72:14
   2: core::panicking::panic_display
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:263:5
   3: core::panicking::panic_explicit
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:236:5
   4: playground::bar::panic_cold_explicit
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panic.rs:87:13
   5: playground::bar
             at ./src/main.rs:12:5
   6: playground::foo
             at ./src/main.rs:7:5
   7: playground::main
             at ./src/main.rs:2:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5

Your WASM runtime is even further divorced from this. Does it let you see any further in the stack than your screenshot showed? It did show rust_begin_unwind and panic_fmt similar to above, at least...

---

That said, improving the panic message may still be worthwhile, as long as we don't hurt performance in general. Can you go ahead and add the second location? Then I'll queue a test build and perf run.

[cuviper]

I figured out that in my codebase, when I compile in release mode, there is no lock contention, so that's my current solution, as it's the easiest way.

Also, this seems weird. Release mode shouldn't have such visible effects on lock recursion, unless you have some code that's explicitly only grabbing a lock in debug mode.