-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
std: Move pc-windows-gnu to SEH-based unwinding #31313
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
821c985
to
4fa32ee
Compare
すてき |
Windows back traces being bad have very little to do with how unwinding is done, but rather with debug info. Symbol names are only preserved in the debug info and Windows is incapable of reading dwarf debug info and I highly doubt mingw is capable of emitting PDB/codeview debug info. If we want |
☔ The latest upstream changes (presumably #31298) made this pull request unmergeable. Please resolve the merge conflicts. |
I only got working backtraces two weeks ago (#30908) 😭 If possible, please don't land this before it works (i.e., gives decent backtraces) on gnu. Unfortunately, that may not be possible. AFAIK #31319 won't help gnu since CodeView debuginfo is (again AFAIK) useless without an MSVC edit: see Clang documentation |
@rkruppe yeah I was hoping to leave in libbacktrace for GNU for now, but it unfortunately would still require using MinGW's support for unwinding the stack (e.g.
I am curious what others think though! My personal priorities may be a bit backwards relative to others' :) |
This is correct. We already rely on Windows to walk the stack to get a backtrace for If we did transition the |
If we did end up fully transitioning to only MSVC compilers we'd probably consider dropping support for the GNU ones yeah, but I should emphasize that these are just thoughts in my head and would certainly be very far out. Not necessarily any consensus on this strategy among anyone but me! |
To be frank I'm not attached to mingw, it's just what I've always been using to develop. I'm currently trying to build from source w/ MSVC and if that works, I will instantly stop caring about the |
MSVC is 10GB for link.exe. Its pain to install that instead of MinGW if you only need (to debug, in my case) Windows rustc, as opposed to MSVC Rust :) |
Sorry I don't mean to sidetrack discussion here, I don't think this is an appropriate location to discuss switching between |
I believe the point is that there will still be compilers that target |
Let's have a place then, so we can have a discussion before we start rolling in this direction. Should this be in the RFCs repo? |
Nobody is suggesting to deprecate |
☔ The latest upstream changes (presumably #30367) made this pull request unmergeable. Please resolve the merge conflicts. |
This looks like a good consolidation. I'm inclined to break stack traces again for it unless it's possible to rig up something to read the dwarf symbol info in short order. |
I checked briefly and there's no way to at least configure libbacktrace to only have symbol-demangling support, so I suspect there's nothing too too easy we could do to get a dwarf reader. In theory it's not too hard to write though... |
After the fix in #30908 libbacktrace still doesn't work properly with PE/COFF. It does show function names instead of |
Finally found the time to go through this PR. Impressive work! A note regarding rsbegin, rsend and LSDA parser: I wrote this code thinking that eventually we'd use them for Unix-y targets as well,- to reduce reliance on the GCC runtime. Is this something we'd want to do? |
☔ The latest upstream changes (presumably #31470) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r+ |
📌 Commit 3299609 has been approved by |
⌛ Testing commit 3299609 with merge 31377ef... |
💔 Test failed - auto-win-gnu-32-opt |
@bors: r=brson b2a53fe1a5f226d2cc88688bb4ddf07170591a9b |
⌛ Testing commit b2a53fe with merge 6b784f3... |
💔 Test failed - auto-win-gnu-32-nopt-t |
@bors: r=brson 7c7ecb4 |
This commit moves the `*-pc-windows-gnu` targets (e.g. the MinGW targets) to use SEH-based unwinding instead of libunwind-based unwinding. There are a number of ramifications on these targets as a result: * Binary distributions of the standard library are no longer tied to a particular compiler toolchain. The MinGW toolchains typically ship with either SEH, Dwarf, or SjLj based unwinding and are binary-incompatible, but with SEH unwinding we'll be able to link with any of the toolchains. * The GNU implementation is now much closer to the MSVC implementation, reducing the amount of duplicated code we'll have to maintain (yay!). * Due to the loss of the libunwind stack unwinder the libbacktrace library is no longer used on Windows. The same unwinding code for MSVC is now used for GNU as well, and unfortunately this has empirically led to worse stack traces in the past. In theory, though, this should be fixed for both MSVC and GNU at the same time! * Due to the lack of a need for frame unwind info registration, the `rsend.o` and `rsbegin.o` startup object files are no longer built. Additionally the `crt2.o` and `dllcrt2.o` files are no longer distributed. It's assumed that the linker in use will inject these as usual. The `-nostdlib` flag is no longer passed to the linker to indicate this. This change also opened up the possibility to reorganize a few modules, so the following changes were also made: * The `custom_unwind_resume` option and all support code was removed from trans as this is no longer necessary. * The `sys_common::unwind` module was refactored to have the platform-specific portions live in `sys::unwind`. * A similar refactoring was applied to backtrace writing (just shuffling some files around). * Documentation was updated in shuffled modules to reflect the current state of affairs.
You appear to have r+'d the wrong commit |
@bors r=brson |
📌 Commit 0487292 has been approved by |
🔒 Merge conflict |
☔ The latest upstream changes (presumably #31282) made this pull request unmergeable. Please resolve the merge conflicts. |
/// This is only true for MSVC targets, and even then the 64-bit MSVC target | ||
/// currently uses SEH-ish unwinding with DWARF info tables to the side (same as | ||
/// 64-bit MinGW) instead of "full SEH". | ||
/// This is currentlyt true for all Windows targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
Well it appears I was a little too eager to land this, and I didn't thoroughly test it enough locally before doing so. My testing was all on It turns out that if LLVM has assertions enabled, they specifically have a check against this which means that we can't actually use funclets (the new exception handling things) on Basically it looks like this just isn't supported at this time for Alas! |
Note that my threshold for "this doesn't work" is:
target triple = "i686-pc-windows-gnu"
define void @bar() {
ret void
}
define void @main() personality i32 (...)* @_except_handler3 {
entry-block:
invoke void @bar()
to label %exit unwind label %bad
exit:
ret void
bad:
%pad = cleanuppad within none []
cleanupret from %pad unwind to caller
}
declare i32 @_except_handler3(...)
Note that the MSVC |
Is there an upstream issue? Should there be? |
In theory, yes. I chatted briefly in |
This commit moves the
*-pc-windows-gnu
targets (e.g. the MinGW targets) touse SEH-based unwinding instead of libunwind-based unwinding. There are a number
of ramifications on these targets as a result:
particular compiler toolchain. The MinGW toolchains typically ship with either
SEH, Dwarf, or SjLj based unwinding and are binary-incompatible, but with SEH
unwinding we'll be able to link with any of the toolchains.
the amount of duplicated code we'll have to maintain (yay!).
longer used on Windows. The same unwinding code for MSVC is now used for GNU
as well, and unfortunately this has empirically led to worse stack traces in
the past. In theory, though, this should be fixed for both MSVC and GNU at the
same time!
rsend.o
and
rsbegin.o
startup object files are no longer built. Additionally thecrt2.o
anddllcrt2.o
files are no longer distributed. It's assumed thatthe linker in use will inject these as usual. The
-nostdlib
flag is nolonger passed to the linker to indicate this.
This change also opened up the possibility to reorganize a few modules, so the
following changes were also made:
custom_unwind_resume
option and all support code was removed from transas this is no longer necessary.
sys_common::unwind
module was refactored to have the platform-specificportions live in
sys::unwind
.files around).
affairs.