Print thread ID in panic message

[tgross35]

panic! does not print any identifying information for threads that are
unnamed. However, in many cases, the thread ID can be determined.

This changes the panic message from something like this:

thread '<unnamed>' panicked at src/main.rs:3:5: explicit panic 

To something like this:

thread '<unnamed>' (12345) panicked at src/main.rs:3:5: explicit panic 

Stack overflow messages are updated as well.

This change applies to both named and unnamed threads. The ID printed is
the OS integer thread ID rather than the Rust thread ID, which should
also be what debuggers print.

try-job: aarch64-apple
try-job: aarch64-gnu
try-job: dist-apple-various
try-job: dist-various-*
try-job: dist-x86_64-freebsd
try-job: dist-x86_64-illumos
try-job: dist-x86_64-netbsd
try-job: dist-x86_64-solaris
try-job: test-various
try-job: x86_64-gnu
try-job: x86_64-mingw-1
try-job: x86_64-msvc-1

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[tgross35]

I think this needs FCP since it's a visible change

@rustbot label -T-libs -T-compiler +T-libs-api
r? libs-api

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[tgross35]

cc @m-ou-se since you recently did the formatting in #112849

[bjorn3]

Maybe

thread '<unnamed 2>' panicked at src/main.rs:3:5: explicit panic 

or

thread '<unnamed#2>' panicked at src/main.rs:3:5: explicit panic 

? That fits more with the way named threads are shown.

[the8472]

Note that this is a rust-internal thread ID, which isn't the OS thread ID which can be relevant if you're also logging from C code or looking at things with GDB. And as_u64() is unstable and there are questions about stabilizing it (#110738).

[tgross35]

Maybe

thread '<unnamed 2>' panicked at src/main.rs:3:5: explicit panic 

or

thread '<unnamed#2>' panicked at src/main.rs:3:5: explicit panic 

I just proposed the syntax as a demo, figured libs-api would make the final choice - though between those two, I like the first one better

Note that this is a rust-internal thread ID, which isn't the OS thread ID which can be relevant if you're also logging from C code or looking at things with GDB. And as_u64() is unstable and there are questions about stabilizing it (#110738).

It is kind of unfortunate that Rust and C won't use the same thread IDs. But this seems worthwhile still - Rust-only applications printing the thread ID in logs is common (tracing even has a builtin way to do this), probably more common than Rust thread IDs silently mixing with C thread IDs in logs. And even in that case, it still gives you something more concrete to trace than "something failed".

This is all inspired by the below failure I recently had in an highly threaded system. The entire backtrace uselessly displayed one repeated function, with no hints about which preceding log events were relevant:

thread '<unnamed>' panicked at src/scan.rs:101:55: called `Result::unwrap()` on an `Err` value: () stack backtrace: 0: 0x1009661d0 - __mh_execute_header 1: 0x10088869b - __mh_execute_header 2: 0x10093ba0e - __mh_execute_header 4: 0x10096757c - __mh_execute_header 5: 0x100968632 - __mh_execute_header 6: 0x1009680f4 - __mh_execute_header 7: 0x100968059 - __mh_execute_header 8: 0x100968042 - __mh_execute_header 9: 0x100bdd913 - __mh_execute_header 

I don't think that stability of the thread ID is much of a concern since we will always have some integer ID for a thread, even if the semantics of that ID might change. The important thing is probably just that this panic message thread ID is the same as what shows up as Debug for ThreadId (or as_u64 as applicable) since that is what other libraries are directed to use. That is the case here

[tgross35]

Another display option - it looks a bit cleaner to not even call out that a thread is unnamed, and I don't think it adds much

thread with ID 2 panicked at src/main.rs:3:5: explicit panic 

Or we could use ThreadId's debug formatting which tracing and I think log do, but that seems unnecessarily clunky

thread with `ThreadId(2)` panicked at src/main.rs:3:5: explicit panic 

I also tried to poke some discussion at #67939

[bors]

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

[tgross35]

@joshtriplett do you have any thoughts on this?

[timClicks]

Sorry to add bike shedding to the thread. Here is a slight derivative of the last suggestion, which omits a "with":

thread `ThreadId(2)` panicked at src/main.rs:3:5: explicit panic 

Although it's uglier than using the thread identifier in a sentence, using the Debug form makes it look like a Rust value rather than something else. It's also consistent with other output from Rust, although the case can definitely be made that thread IDs deserve a special case.

[tgross35]

I know Josh has a pretty busy queue, so perhaps it is best to reroll

r? libs-api

[dtolnay]

I think this needs FCP since it's a visible change

@​rustbot label -T-libs -T-compiler +T-libs-api

I think it wouldn't need one. T-libs-api FCP is primarily for any time a permanent API commitment is being made by the standard library, and deprecations. In contrast, anything that we can just change back with minimal effort or disruption would not warrant FCP.

#112849 had a T-libs FCP but I am not familiar with their criteria for that. @rust-lang/libs would you want to handle this PR?

In any case, this looks good to me.

[thomcc]

I don't know that this needs FCP. I'm not opposed and there is precedent for it, but OTOH we wouldn't FCP a panic message change somewhere in the stdlib. So I don't feel strongly.

In any case, I think this change is a good idea, and am in favor.

[tgross35]

There is not any way currently. It's probably possible but that's just an extra knob that we should probably avoid. Can you preprocess the text before saving or before deduplicating, like compiletest does

rust/src/tools/compiletest/src/runtest.rs

Lines 2570 to 2573 in 321a89b

	// Normalize thread IDs in panic messages
	normalized = static_regex!(r"thread '(?P<name>.*?)' \((rtid )?\d+\) panicked")
	.replace_all(&normalized, "thread '$name' ($$TID) panicked")
	.into_owned();

?

[HKalbasi]

This makes snapshot testing of rust programs that panic very hard, since the id will change in each run. A way to disable this is much appreciated. If a specific knob for this is too much, I would suggest showing this only on RUST_BACKTRACE=1.

[tgross35]

Is there any reason the snapshots can't normalize the message? (e.g. #115746 (comment))

[HKalbasi]

It is not the end of the world, there are workarounds like that normalization. It's just a tradeoff between creating work for people working with the output of panicking Rust binaries, and annoying people who need this thread id feature by forcing them to use RUST_BACKTRACE=1. I guess the number of people in the first group are more, but I'm definitely biased.

[tgross35]

So, the main motivation for this change is when the thread ID is printed as part of log/tracing output and then seeing a panic, which allows tying the panic to the exact events that happened before it (previously not possible in multithreaded applications unless you set the thread name, which might also not be possible depending on what library spawned it). I think that requiring RUST_BACKTRACE would be a step backward because if you happen to forget to set it, that association is lost again. And it becomes something that you need to actually know about in order to benefit from.

I also don't understand what makes this a "very hard" thing to adjust to: it seems like it should be reasonably trivial, ours was a three line change. Of course it's unfortunate that all snapshot libraries need to handle this, but we don't guarantee panic output so infrequent changes are expected if you're doing a 1:1 comparison (which should be way less frequent than something like #[should_panic = "hi"]).

I don't mind discussing further, but it should be in a new issue (there are nearly 20 people on this thread and it's closed anyway).