Ignore return of SymInitializeW on Windows
#231
Conversation
This commit updates the behavior of backtraces on Windows to execute `SymInitializeW` globally once-per-process and ignore the return value as to whether it succeeded or not. This undoes previous work in this crate to specifically check the return value, and this is a behavior update for the standard library when this goes into the standard library. The `SymInitializeW` function is required to be called on MSVC before any backtraces can be captured or before any addresses can be symbolized. This function is quite slow. It can only be called once-per-process and there's a corresponding `SymCleanup` to undo the work of `SymInitializeW`. The behavior of what to do with `SymInitializeW` has changed a lot over time in this crate. The very first implementation for Windows paired with `SymCleanup`. Then reports of slowness at rust-lang/rustup#591 led to ac8c6d2 where `SymCleanup` was removed. This led to #165 where because the standard library still checked `SymInitializeW`'s return value and called `SymCleanup` generating a backtrace with this crate would break `RUST_BACKTRACE=1`. Finally (and expectedly) the performance report has come back as #229 for this crate. I'm proposing that the final nail is put in this coffin at this point where this crate will ignore the return value of `SymInitializeW`, initializing symbols once per process globally. This update will go into the standard library and get updated on the stable channel eventually, meaning both libstd and this crate will be able to work with one another and only initialize the process's symbols once globally. This does mean that there will be a period of "breakage" where we will continue to make `RUST_BACKTRACE=1` not useful if this crate is used on MSVC, but that's sort of the extension of the status quo as of a month or so ago. This will eventually be fixed once the stable channel of Rust is updated.
|
@sfackler mind taking a look at this and lending your opinion to this? This will be a behavior change for the standard library on MSVC, so I figured it's good to double check with at least one other libs member to ensure it's ok. |
|
Under what conditions does SymInitializeW fail? Being called when initialized? |
|
Unfortunately that's not 100% clear to me. I know for certain of one failure case, but I don't know of others. The for-certain failure case is that if you call Presumably we could also detect cases of like "actual symbol initialization failed" and ignore the "you initialized twice" errors and actually report "symbol initialization failed" failures, but I don't currently know of a way to do so. I believe there are also not really any consequences for using symbolication/backtrace functions if |
|
Ok, and this is going to regress the standard library since it expects SymInitializeW to succeed before taking each backtrace? It might make sense to backport a backtrace upgrade to beta in that case. |
|
It sort of depends on how you look at it. The standard library has always behaved as it does today, which is to pair initialize/cleanup calls and skip generating a backtrace if initialization fails. This crate itself was changed a month or so ago to also pair initialize/cleanup calls to enusre that if you use this crate In that sense the standard library if you don't use this crate doesn't change, it just continues to work. The libstd from January will be as "broken" as the libstd when this PR hits crates.io for the I would classify this more as a temporary regression for this crate than for the standard library myself. |
|
Maybe someday we'll just be able to use Gimli :P |
This commit updates the behavior of backtraces on Windows to execute
SymInitializeWglobally once-per-process and ignore the return valueas to whether it succeeded or not. This undoes previous work in this
crate to specifically check the return value, and this is a behavior
update for the standard library when this goes into the standard
library.
The
SymInitializeWfunction is required to be called on MSVC beforeany backtraces can be captured or before any addresses can be
symbolized. This function is quite slow. It can only be called
once-per-process and there's a corresponding
SymCleanupto undo thework of
SymInitializeW.The behavior of what to do with
SymInitializeWhas changed a lot overtime in this crate. The very first implementation for Windows paired
with
SymCleanup. Then reports of slowness at rust-lang/rustup#591led to ac8c6d2 where
SymCleanupwas removed. This led to #165 wherebecause the standard library still checked
SymInitializeW's returnvalue and called
SymCleanupgenerating a backtrace with this cratewould break
RUST_BACKTRACE=1. Finally (and expectedly) the performancereport has come back as #229 for this crate.
I'm proposing that the final nail is put in this coffin at this point
where this crate will ignore the return value of
SymInitializeW,initializing symbols once per process globally. This update will go into
the standard library and get updated on the stable channel eventually,
meaning both libstd and this crate will be able to work with one another
and only initialize the process's symbols once globally. This does mean
that there will be a period of "breakage" where we will continue to make
RUST_BACKTRACE=1not useful if this crate is used on MSVC, but that'ssort of the extension of the status quo as of a month or so ago. This
will eventually be fixed once the stable channel of Rust is updated.