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

Allow backtraces to cross thread boundaries #4093

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

actuallyatoaster
Copy link

Fixes #4066

When a thread is spawned we store the ID of the spawning thread as well as a backtrace to the spawn location. When there's an error we can traverse upwards, displaying a backtrace up to the main thread.

For instance, with the following program:

extern "C" {
    fn unknown();
}

fn bad() {
    unsafe { unknown() }
}

fn main() {
    std::thread::spawn(bad);
}

Whereas previously we would see:

error: unsupported operation: can't call foreign function `unknown` on OS `linux`
 --> tests/temp.rs:6:14
  |
6 |     unsafe { unknown() }
  |              ^^^^^^^^^ can't call foreign function `unknown` on OS `linux`
  |
  = help: if this is a basic API commonly used on this target, please report an issue with Miri
  = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases
  = note: BACKTRACE on thread `unnamed-1`:
  = note: inside `bad` at tests/temp.rs:6:14: 6:23

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

We now have:

error: unsupported operation: can't call foreign function `unknown` on OS `linux`
  --> tests/temp.rs:6:14
   |
6  |     unsafe { unknown() }
   |              ^^^^^^^^^ can't call foreign function `unknown` on OS `linux`
   |
   = help: if this is a basic API commonly used on this target, please report an issue with Miri
   = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases
   = note: BACKTRACE on thread `unnamed-1`:
   = note: inside `bad` at tests/temp.rs:6:14: 6:23
   = note: thread `unnamed-1` was spawned by thread `main`
   = note: inside `std::sys::pal::unix::thread::Thread::new` at /home/reimu/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/pal/unix/thread.rs:84:19: 84:86
   = note: inside `std::thread::Builder::spawn_unchecked_::<'_, fn() {bad}, ()>` at /home/reimu/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:600:30: 600:64
   = note: inside `std::thread::Builder::spawn_unchecked::<fn() {bad}, ()>` at /home/reimu/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:467:32: 467:62
   = note: inside `std::thread::Builder::spawn::<fn() {bad}, ()>` at /home/reimu/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:400:18: 400:41
   = note: inside `std::thread::spawn::<fn() {bad}, ()>` at /home/reimu/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/mod.rs:730:5: 730:28
note: inside `main`
  --> tests/temp.rs:10:5
   |
10 |     std::thread::spawn(bad);
   |     ^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2024

You'll need to bless the windows tests, too. It should suffice to pass --target with a windows target, no windows host needed

@oli-obk oli-obk added this pull request to the merge queue Dec 14, 2024
@RalfJung RalfJung removed this pull request from the merge queue due to a manual request Dec 14, 2024
@RalfJung
Copy link
Member

I have some nits.

@@ -235,6 +235,9 @@ pub struct Thread<'tcx> {
/// The join status.
join_status: ThreadJoinStatus,

// ThreadId that spawned this thread and backtrace to where this thread was spawned
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ThreadId that spawned this thread and backtrace to where this thread was spawned
/// ThreadId that spawned this thread and backtrace to where this thread was spawned.

Comment on lines +606 to +608
// No span for non-local frames except the first frame (which is the error site).
if is_local && idx > 0 {
err.subdiagnostic(frame_info.as_note(machine.tcx));
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not right, the first frame is not the error site.

Comment on lines +604 to +614
for (idx, frame_info) in pruned_stacktrace.iter().enumerate() {
let is_local = machine.is_local(frame_info);
// No span for non-local frames except the first frame (which is the error site).
if is_local && idx > 0 {
err.subdiagnostic(frame_info.as_note(machine.tcx));
} else {
let sm = sess.source_map();
let span = sm.span_to_embeddable_string(frame_info.span);
err.note(format!("{frame_info} at {span}"));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicating existing logic. Can you move this into a shared helper fn?

Comment on lines +10 to +14
= note: inside `std::sys::pal::PLATFORM::thread::Thread::new` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC
= note: inside `std::thread::Builder::spawn_unchecked_::<'_, {closure@tests/fail-dep/concurrency/libc_pthread_mutex_deadlock.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/mod.rs:LL:CC
= note: inside `std::thread::Builder::spawn_unchecked::<{closure@tests/fail-dep/concurrency/libc_pthread_mutex_deadlock.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/mod.rs:LL:CC
= note: inside `std::thread::Builder::spawn::<{closure@tests/fail-dep/concurrency/libc_pthread_mutex_deadlock.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/mod.rs:LL:CC
= note: inside `std::thread::spawn::<{closure@tests/fail-dep/concurrency/libc_pthread_mutex_deadlock.rs:LL:CC}, ()>` at RUSTLIB/std/src/thread/mod.rs:LL:CC
Copy link
Member

Choose a reason for hiding this comment

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

It's not pretty to show this for every backtrace. We spent a bunch of effort making backtraces nice, i.e. they usually stop at main and don't go all the way to the start lang item. We should do something similar here to avoid drowning the user in noise.

I could imagine two options:

  • we mark these frames as #[caller_location] in std (we have done this for a bunch of other functions to make backtraces prettier)
  • when pruning the stack for another thread, we also remove all non-local frames at the top

I am leaning towards the first option since the second option might prune too much when the thread is spawned deep inside some other library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backtraces stop at thread boundaries
3 participants