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

std: Move pc-windows-gnu to SEH-based unwinding #31313

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

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.

@alexcrichton
Copy link
Member Author

r? @brson

cc @vadimcn

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@nagisa
Copy link
Member

nagisa commented Jan 31, 2016

すてき :shipit:

@retep998
Copy link
Member

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!

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 -gnu stack traces to still be useful we'd need code capable of parsing dwarf debug info to resolve symbol names.

@bors
Copy link
Contributor

bors commented Jan 31, 2016

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

@alexcrichton
Copy link
Member Author

It looks like MSVC backtraces via RUST_BACKTRACE are basically fixed by #31319 (they just require compiling with -g as @retep998 mentions). I tried that PR with the GNU triples and got weird results and haven't pushed further just yet.

@hanna-kruppe
Copy link
Contributor

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 link.exe to turn it into PDB debuginfo.

edit: see Clang documentation

@alexcrichton
Copy link
Member Author

@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. _Unwind_Backtrace). One of the major purposes of this PR is to get away from MinGW's need to unwind the stack (as it comes in multiple flavors). I think that a few things can be done to help mitigate the loss of backtraces, though:

  • We should probably stop shipping MinGW-host compilers. I think that a MSVC host compiler is already dependency-free and does everything we need (and will have nice backtraces now). The *-pc-windows-gnu targets are in theory just that, separate targets, and they should be handled through the upcoming cross-compilation support we're working on.
  • I hope to update the backtrace crate to work more-well on MinGW. I think that compiling that at build-time (vs shipping a binary) will allow much greater flexibility to work in more situations. That should at least help provide the ability to get quality back traces on GNU windows.
  • Overall, this signifies a shift away from the pc-windows-gnu triples towards the MSVC ones. This shift has basically always been inevitable, and in some sense if there are very few users of pc-windows-gnu the need for backtraces may not be as high.
  • We could perhaps figure out how to jerry-rig libbacktrace to use the standard Windows stack unwinding API but use the libbacktrace support to convert function pointers to symbol names. I think that the backtraces themselves on Windows have always been accurate, it's just the matching-up-to-symbol names that have sometimes been bogus.

I am curious what others think though! My personal priorities may be a bit backwards relative to others' :)

@retep998
Copy link
Member

retep998 commented Feb 1, 2016

I think that the backtraces themselves on Windows have always been accurate, it's just the matching-up-to-symbol names that have sometimes been bogus.

This is correct. We already rely on Windows to walk the stack to get a backtrace for -gnu targets and then just ask libbacktrace to resolve the address into a symbol for us. If libbacktrace could be jury rigged to only have the symbol lookup stuff without depending on _Unwind_Backtrace and friends, that could work fairly well.

If we did transition the -gnu compilers to use -msvc as their host, would we continue to support and test the -gnu hosts? I personally don't mind if we drop that support.

@alexcrichton
Copy link
Member Author

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!

@hanna-kruppe
Copy link
Contributor

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 *-windows-gnu triples.

@nagisa
Copy link
Member

nagisa commented Feb 1, 2016

We should probably stop shipping MinGW-host compilers

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 :)

@alexcrichton
Copy link
Member Author

Sorry I don't mean to sidetrack discussion here, I don't think this is an appropriate location to discuss switching between *-pc-windows-gnu and/or MSVC. I was just trying to describe how I thought the two interacted over time.

@retep998
Copy link
Member

retep998 commented Feb 1, 2016

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 :)

I believe the point is that there will still be compilers that target *-windows-gnu, it's just their host would be *-windows-msvc, you wouldn't need MSVC to use that Rust. You'd only need MSVC when using the *-windows-msvc targets or building rustc itself.

@vadimcn
Copy link
Contributor

vadimcn commented Feb 1, 2016

I don't think this is an appropriate location to discuss switching between *-pc-windows-gnu and/or MSVC

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?
Meanwhile, 2c from me: A major downside of *-windows-msvc is that the toolchain is not self-contained. You cannot just install it and compile Rust programs, you need to install MSVC too.
IMHO, we cannot start deprecating *-windows-gnu until we have a replacement for linker that we can bundle with rust toolchain.

@retep998
Copy link
Member

retep998 commented Feb 1, 2016

IMHO, we cannot start deprecating *-windows-gnu until we have a replacement for linker that we can bundle with rust toolchain.

Nobody is suggesting to deprecate *-windows-gnu. It would still be a very valid supported target and use MinGW for the linker. The idea is that its host would be *-windows-msvc which would not require you to have MSVC installed to use it. At worst you'd need the VC++ redistributable which is tiny and practically everyone has it anyway.

@bors
Copy link
Contributor

bors commented Feb 1, 2016

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

@brson
Copy link
Contributor

brson commented Feb 2, 2016

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.

@alexcrichton
Copy link
Member Author

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...

@petrochenkov
Copy link
Contributor

After the fix in #30908 libbacktrace still doesn't work properly with PE/COFF. It does show function names instead of <unknown>, but those names can often be incorrect. So, I wouldn't mind dropping it if some replacement is provided soon enough.

@vadimcn
Copy link
Contributor

vadimcn commented Feb 2, 2016

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?

@bors
Copy link
Contributor

bors commented Feb 8, 2016

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

@brson
Copy link
Contributor

brson commented Feb 8, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2016

📌 Commit 3299609 has been approved by brson

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 8, 2016
@bors
Copy link
Contributor

bors commented Feb 8, 2016

⌛ Testing commit 3299609 with merge 31377ef...

@bors
Copy link
Contributor

bors commented Feb 8, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member Author

@bors: r=brson b2a53fe1a5f226d2cc88688bb4ddf07170591a9b

@bors
Copy link
Contributor

bors commented Feb 9, 2016

⌛ Testing commit b2a53fe with merge 6b784f3...

@bors
Copy link
Contributor

bors commented Feb 9, 2016

💔 Test failed - auto-win-gnu-32-nopt-t

@alexcrichton
Copy link
Member Author

@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.
@retep998
Copy link
Member

retep998 commented Feb 9, 2016

You appear to have r+'d the wrong commit

@nagisa
Copy link
Member

nagisa commented Feb 9, 2016

@bors r=brson

@bors
Copy link
Contributor

bors commented Feb 9, 2016

📌 Commit 0487292 has been approved by brson

@bors
Copy link
Contributor

bors commented Feb 9, 2016

🔒 Merge conflict

@bors
Copy link
Contributor

bors commented Feb 9, 2016

☔ 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@alexcrichton
Copy link
Member Author

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 x86_64-pc-windows-gnu, but the major triple being changed here is i686-pc-windows-gnu, which was left untested.

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 i686-pc-windows-gnu. I tried to retrofit some support into LLVM and was also unable to produce working executables locally (maybe ld.exe went crazy somewhere?).

Basically it looks like this just isn't supported at this time for i686-pc-windows-gnu, and that was really the only major benefit here unfortunately. Today the x86_64-pc-windows-gnu triple already doesn't have many extra toolchain specific dependencies, it's largely just the i686 one (as we're tied to a specific exception handling model). As a result, I don't think that we're going to get much benefit from taking this PR for just x86_64-pc-windows-gnu and not the 32-bit version. Consequently, I'm going to close this for now.

Alas!

@alexcrichton
Copy link
Member Author

Note that my threshold for "this doesn't work" is:

  • Get LLVM to compile this IR, probably with some local modifications to LLVM as it currently triggers an assert:
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(...)
  • Compile that file with gcc.exe or with ld.exe, try running the output:
$ llc foo.ll -filetype=obj -o foo.o
$ gcc ./foo.o
$ ./a.exe
bash: ./a.exe: cannot execute binary file: Exec format error
  • Try the same in an MSVC prompt:
$ link.exe /out:foo.exe foo.o msvcrt.lib ../path/to/libgcc.a
$ foo.exe
$

Note that the MSVC link.exe works just fine where the MinGW ld.exe fails. Not really sure why...

@alexcrichton alexcrichton deleted the gnu-seh branch February 9, 2016 19:48
@tamird
Copy link
Contributor

tamird commented Feb 9, 2016

Is there an upstream issue? Should there be?

@alexcrichton
Copy link
Member Author

In theory, yes. I chatted briefly in #llvm about this and was met with "wait what are you doing?!" so I don't think it's expected to work, and the fact that MinGW produces a somehow invalid binary is even more worrisome. Those two combined make me think this is a dead end for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.