search

rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
530 stars 245 forks source link

Added tracing of threads other than the calling thread on Windows #473

Closed aminya closed 6 months ago

aminya commented 2 years ago

This is a continuation of the work by @Jardynq that makes it possible to trace threads other than the current one. This can be used to profile Windows applications.

https://github.com/tikv/pprof-rs/issues/9#issuecomment-1114222675

https://github.com/tikv/pprof-rs/pull/122

aminya commented 2 years ago

This is ready to go. Let me know if there is any feedback.

The MIR test is irrelevant to this pull request, and it is also happening on the master branch.

aminya commented 2 years ago

I'm wary of adding platform-specific functionality to this crate so this may best live elsewhere. The dbghelp.rs file alone isn't the most complicated to copy for something like that.

This only adds Windows for now because it is Rust cannot be profiled on Windows, and we need this to remove a platform-specific restriction. In the future, if other operating systems have such functionality, people can suggest implementations for those too.

I think it is more important to allow people to profile Rust on Windows. This is way more limiting than not having the implementation of trace_thread for other operating systems.

Otherwise this is not a safe implementation because the thread is resumed after the register context is captured. That means the stack walking may not work because the stack could change while the stack was being walked.

Based on the official Windows docs, the only possible way to get a valid context for a running thread is to suspend it. https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadcontext#remarks

As I mentioned in this comment, ResumeThread just increments the counter back to the initial state of the thread. https://github.com/rust-lang/backtrace-rs/pull/473#discussion_r917625960

bjorn3 commented 2 years ago

This only adds Windows for now because it is Rust cannot be profiled on Windows

backtrace-rs is not magic. You can use the same api's as for C/C++ stack walking to walk the stack for Rust. This means that you can copy dbghelp.rs from backtrace-rs into a separate crate and implement walking the stack of a different thread in this copy. No need to change backtrace-rs.

and we need this to remove a platform-specific restriction.

I fail to see how it is a platform-specific restriction as backtrace-rs has no support for stack walking of a different thread on any platform at all.

Based on the official Windows docs, the only possible way to get a valid context for a running thread is to suspend it. https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadcontext#remarks

I think @alexcrichton means that you need to keep the running thread suspended while walking the stack to prevent corruption of the resulting backtrace.

CraftSpider commented 1 year ago

Are the objections to this PR more about the feature of walking other threads in general, or about it being added for Windows in particular? I'm coming here from pprof as well, and interested in trying to move this work forwards in some way, and I have access to both a Windows and Linux environment, so could try to implement similar functionality on both if that would make this work more appealing.

The talk of 'platform specific restriction' is that Windows doesn't support profiling from a single running thread, so the lack of this feature prevents profiling on Windows entirely without forking the library, which I think is reasonable for the pprof maintainers to be wary of taking on - it would be a significant maintenance burden to maintain what is effectively just 'backtrace with this code patched in'.

workingjubilee commented 6 months ago

This has fallen behind the current work on dbghelp, so I am closing it. Apologies. Please feel free to reopen and rebase this (in that order) if you wish to pursue this again.