Skip to content

/rust

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

msvc: Enable unwinding, improve finding link.exe #26741

Merged
merged 7 commits into from Jul 6, 2015

Conversation

Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@alexcrichton @rust-highfive @jroesch @vadimcn @retep998 @brson @bors @nikomatsakis
@alexcrichton
Copy link
Member

alexcrichton commented Jul 2, 2015

This PR was originally going to be a "let's start running tests on MSVC" PR, but it didn't quite get to that point. It instead gets us ~80% of the way there! The steps taken in this PR are:

  • Landing pads are turned on by default for 64-bit MSVC. The LLVM support is "good enough" with the caveat the destructor glue is now marked noinline. This was recommended on the associated bug as a stopgap until LLVM has a better representation for exception handling in MSVC. The consequence of this is that MSVC will have a bit of a perf hit, but there are possible routes we can take if this workaround sticks around for too long.
  • The linker (link.exe) is now looked up in the Windows Registry if it's not otherwise available in the environment. This improves using the compiler outside of a VS shell (e.g. in a MSYS shell or in a vanilla cmd.exe shell). This also makes cross compiles via Cargo "just work" when crossing between 32 and 64 bit!
  • TLS destructors were fixed to start running on MSVC (they previously weren't running at all)
  • A few assorted run-pass tests were fixed.
  • The dependency on the rust_builtin library was removed entirely for MSVC to try to prevent any cl.exe compiled objects get into the standard library. This should help us later remove any dependence on the CRT by the standard library.
  • I re-added rust_try_msvc_32.ll for 32-bit MSVC and ensured that landing pads were turned off by default there as well.

Despite landing pads being enabled, there are still many failing tests on MSVC. The two major classes I've identified so far are:

  • Spurious aborts. It appears that when optimizations are enabled that landing pads aren't always lined up properly, and sometimes an exception being thrown can't find the catch block down the stack, causing the program to abort. I've been working to reduce this test case but haven't been met with great success just yet.
  • Parallel codegen does not work on MSVC. Our current strategy is to take the N object files emitted by the N codegen threads and use ld -r to assemble them into one object file. The MSVC linker, however, does not have this ability, and this will need to be rearchitected to work on MSVC.

I will fix parallel codegen in a future PR, and I'll also be watching LLVM closely to see if the aborts... disappear!

alexcrichton added some commits Jun 29, 2015

@alexcrichton
msvc: Enable landing pads by default 
This commit turns on landing pads for MSVC by default, which means that we'll
now be running cleanups for values on the stack when an exception is thrown.
This commit "fixes" the previously seen LLVM abort by attaching the `noinline`
attribute to all generated drop glue to prevent landing pads from being inlined
into other landing pads.

The performance of MSVC is highly likely to decrease from this commit, but there
are various routes we can taken in the future if this ends up staying for quite
a while, such as generating a shim function only called from landing pads which
calls the actual drop glue, and this shim is marked noinline.

For now, however, this patch enables MSVC to successfully bootstrap itself!
f9de964
@alexcrichton
msvc: Lookup linker in windows registry 
This commit alters the compiler to no longer "just run link.exe" but instead
probe the system's registry to find where the linker is located. The default
library search path (normally found through LIB) is also found through the
registry. This also brings us in line with the default behavior of Clang, and
much of the logic of where to look for information is copied over from Clang as
well. Finally, this commit removes the makefile logic for updating the
environment variables for the compiler, except for stage0 where it's still
necessary.

The motivation for this change is rooted in two positions:

* Not having to set up these environment variables is much less hassle both for
  the bootstrap and for running the compiler itself. This means that the
  compiler can be run outside of VS shells and be run inside of cmd.exe or a
  MSYS shell.

* When dealing with cross compilation, there's not actually a set of environment
  variables that can be set for the compiler. This means, for example, if a
  Cargo compilation is targeting 32-bit from 64-bit you can't actually set up
  one set of environment variables. Having the compiler deal with the logic
  instead is generally much more convenient!
91c22b6
@alexcrichton
msvc: Fix TLS destructors 
Just like the original article our Windows TLS support is based on predicted,
this symbol must be linked in on MSVC to pull in the necessary support for TLS
variables. This commit fixes a number of unit tests which require that TLS
destructors are run.
ae0eb67
@alexcrichton
msvc: Ignore extern-pass-empty on MSVC 
The MSVC compiler doesn't like empty structs, so this test won't link on MSVC,
so it's ignored.
407fb29
@alexcrichton
msvc: Fix the link name for the lgamma 
The function is apparently just called lgamma on MSVC
18c39e1
@alexcrichton
windows: Don't link rust_builtin 
This library has no shims which are actually needed on Windows now, so translate
that last easy one into Rust and then don't link it at all on Windows.
83ee47b
@alexcrichton
rustc_trans: Disable landing pads on 32-bit MSVC 
This is currently quite buggy in LLVM from what I can tell, so just disable it
entirely. This commit also adds preliminary support, however, to actually
target 32-bit MSVC by making sure the `rust_try_msvc_32.ll` file exists and
wiring up exceptions to `_except_handler3` instead of `__C_specific_handler`
(which doesn't exist on 32-bit).
Loading status checks…
3e26e56

@rust-highfive rust-highfive assigned nikomatsakis Jul 2, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jul 2, 2015

r? @nikomatsakis

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 2, 2015

r? @brson

cc @retep998 -- lots of windows registry stuff
cc @epdtry -- do you remember if there's a fundamental reason we use ld -r? I'm under the impression it's just convenience
cc @vadimcn -- also about registry stuff, hopefully lots of the stuff in the configure script can be removed eventually now!

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Jul 2, 2015

@jroesch

This comment has been minimized.

Sign in to view
Copy link
Member

jroesch commented Jul 2, 2015

@alexcrichton: 👍
@vadimcn

This comment has been minimized.

Sign in to view
Copy link
Contributor

vadimcn commented Jul 2, 2015

Trawling the registry to find linker and SDK libraries is... unorthodox and is likely to break with future versions, but I guess you already know that. Otherwise, lgtm. Sadly, I cannot propose a better way of detecting VC's install location.
@vadimcn

This comment has been minimized.

Sign in to view
Copy link
Contributor

vadimcn commented Jul 2, 2015

Regarding unwinding: if the goal is to get rid of libgcc dependency, we could implement Itanium-style LSDA parser in rust_eh_personality and thus avoid all the trouble associated with MSVC-style landing pads.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 2, 2015

Yeah I was a little uneasy probing the registry, but it's what clang does so I didn't feel too too bad about it (ish)

I also actually started a branch a long time ago to just write the necessary portions of libgcc in Rust, but it ended up getting super hairy super quickly. I want to believe that LLVM support will become more robust/rock solid over the next few months, but if it ends up languishing too much then we can definitely look again into doing the libgcc-like stuff.
@vadimcn

This comment has been minimized.

Sign in to view
Copy link
Contributor

vadimcn commented Jul 3, 2015

it ended up getting super hairy super quickly

Which bits of it got hairy? CPU context management? Did you try to do it just for Windows or for all platforms?
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 3, 2015

Oh no I was only trying for 64-bit MSVC Windows. I found that the rabbit hole kept getting deeper and deeper and look liked it was going to be quite a chunk of code, so I bailed out. This is when I was initially adding MSVC support though so I wasn't expecting it to be a large dive. In isolation it may not be so bad today?
@retep998

This comment has been minimized.

Sign in to view
Copy link
Member

retep998 commented Jul 3, 2015

I checked over the registry ffi definitions and they seem correct.

@retep998 retep998 referenced this pull request Jul 5, 2015

Closed

Compiler on windows 8.1 issues #26647

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Jul 6, 2015

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 6, 2015

📌 Commit 3e26e56 has been approved by brson
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 6, 2015

⌛️ Testing commit 3e26e56 with merge 2ceaa77...

bors added a commit that referenced this pull request Jul 6, 2015

@bors
Auto merge of #26741 - alexcrichton:noinline-destructors, r=brson 
This PR was originally going to be a "let's start running tests on MSVC" PR, but it didn't quite get to that point. It instead gets us ~80% of the way there! The steps taken in this PR are:

* Landing pads are turned on by default for 64-bit MSVC. The LLVM support is "good enough" with the caveat the destructor glue is now marked noinline. This was recommended [on the associated bug](https://llvm.org/bugs/show_bug.cgi?id=23884) as a stopgap until LLVM has a better representation for exception handling in MSVC. The consequence of this is that MSVC will have a bit of a perf hit, but there are possible routes we can take if this workaround sticks around for too long.
* The linker (`link.exe`) is now looked up in the Windows Registry if it's not otherwise available in the environment. This improves using the compiler outside of a VS shell (e.g. in a MSYS shell or in a vanilla cmd.exe shell). This also makes cross compiles via Cargo "just work" when crossing between 32 and 64 bit!
* TLS destructors were fixed to start running on MSVC (they previously weren't running at all)
* A few assorted `run-pass` tests were fixed.
* The dependency on the `rust_builtin` library was removed entirely for MSVC to try to prevent any `cl.exe` compiled objects get into the standard library. This should help us later remove any dependence on the CRT by the standard library.
* I re-added `rust_try_msvc_32.ll` for 32-bit MSVC and ensured that landing pads were turned off by default there as well.

Despite landing pads being enabled, there are still *many* failing tests on MSVC. The two major classes I've identified so far are:

* Spurious aborts. It appears that when optimizations are enabled that landing pads aren't always lined up properly, and sometimes an exception being thrown can't find the catch block down the stack, causing the program to abort. I've been working to reduce this test case but haven't been met with great success just yet.
* Parallel codegen does not work on MSVC. Our current strategy is to take the N object files emitted by the N codegen threads and use `ld -r` to assemble them into *one* object file. The MSVC linker, however, does not have this ability, and this will need to be rearchitected to work on MSVC.

I will fix parallel codegen in a future PR, and I'll also be watching LLVM closely to see if the aborts... disappear!
Loading status checks…
2ceaa77
@bors

@bors bors merged commit 3e26e56 into rust-lang:master Jul 6, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:noinline-destructors branch Jul 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.