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

Implement unwinding for i686 MSVC #30448

Merged
merged 3 commits into from
Jan 30, 2016
Merged

Implement unwinding for i686 MSVC #30448

merged 3 commits into from
Jan 30, 2016

Conversation

alexcrichton
Copy link
Member

These commits perform a few high-level changes with the goal of enabling i686 MSVC unwinding:

  • LLVM is upgraded to pick up the new exception handling instructions and intrinsics for MSVC. This puts us somewhere along the 3.8 branch, but we should still be compatible with LLVM 3.7 for non-MSVC targets.
  • All unwinding for MSVC targets (both 32 and 64-bit) are implemented in terms of this new LLVM support. I would like to also extend this to Windows GNU targets to drop the runtime dependencies we have on MinGW, but I'd like to land this first.
  • Some tests were fixed up for i686 MSVC here and there where necessary. The full test suite should be passing now for that target.

In terms of landing this I plan to have this go through first, then verify that i686 MSVC works, then I'll enable make check on the bots for that target instead of just make as-is today.

Closes #25869

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@alexcrichton
Copy link
Member Author

cc @rust-lang/compiler, there's some pretty significant trans changes here!
cc @vadimcn, lots of windows changes
cc @brson, build system changes! (and llvm changes)

@alexcrichton
Copy link
Member Author

Oh and I think it's important to wait for travis before r+'ing this b/c that make sure that we're still compatible with LLVM 3.7 (which I believe is important for now)

@sanxiyn
Copy link
Member

sanxiyn commented Dec 18, 2015

Travis failed compiling RustWrapper.cpp on LLVM_VERSION_MINOR < 8 paths.

@bors
Copy link
Contributor

bors commented Dec 18, 2015

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

@@ -686,5 +686,8 @@ extern "rust-intrinsic" {
/// Rust's "try catch" construct which invokes the function pointer `f` with
/// the data pointer `data`, returning the exception payload if an exception
/// is thrown (aka the thread panics).
#[cfg(not(stage0))]
pub fn try(f: fn(*mut u8), data: *mut u8, local_ptr: *mut u8) -> i32;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe update the comment with what the heck local_ptr is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Surely!

@vadimcn
Copy link
Contributor

vadimcn commented Dec 19, 2015

Nice work, @alexcrichton!

After the first pass this looks fine to me.
I'll try to find some time to build this branch and play with it. I might have more insights then.

@bors
Copy link
Contributor

bors commented Dec 21, 2015

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

@brson
Copy link
Contributor

brson commented Dec 22, 2015

Impressive.

@alexcrichton
Copy link
Member Author

@nikomatsakis brought up some interesting points on IRC that I didn't realize here. It turns out that today we actually cache cleanups for landing pads, for example with code like:

pub fn foo() {
    let _a = Foo;
    bar();
    let _b = Foo;
    bar();
}

There are two landing pads for this function from the two calls to bar, and both landing pads need a cleanup of the local variable _a. Today we only translate the cleanup of _a once, however, and the landing pad of _b will branch to the cleanup of _a. This is visible in the generated IR.

A problem here, however, is that with cleanuppad this is not possible. The same basic block (e.g. the drop of _a) is being shared between two cleanuppad instances, so it doesn't clearly belong to either one to add a funclet bundle to it. This branch currently works accidentally by basically breaking the cache. For example the IR of this PR re-translates the drop of _a twice.

I just remembered, however, that the cleanupret instruction can take a label to continue unwinding to. I believe we can use this to chain together cleanup pads in a way similar to how landingpad/GNU works.

This'll take some work to get working, but seems critical enough to get it done before landing.

@alexcrichton
Copy link
Member Author

Ok, I've now pushed an updated version which handles caching better on MSVC. The caching for GNU/landingpad exceptions should be the same as it was before, and the IR for the snippet above in MSVC shows the caching being used as well.

Currently the caching isn't as aggressively used on MSVC as it is on GNU b/c on MSVC you can't jump to the middle of a set of cleanup scopes, only the front. Perhaps that only matters for corner cases though?

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 11698cb

@bors
Copy link
Contributor

bors commented Jan 29, 2016

⌛ Testing commit 11698cb with merge 1813eca...

@bors
Copy link
Contributor

bors commented Jan 29, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis f9b267a force

(gonna see if we can get the bots to not build LLVM a million times)

@bors
Copy link
Contributor

bors commented Jan 29, 2016

⌛ Testing commit f9b267a with merge d853d8a...

@bors
Copy link
Contributor

bors commented Jan 30, 2016

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

This brings some routine upgrades to the bundled LLVM that we're using, the most
notable of which is a bug fix to the way we handle range asserts when loading
the discriminant of an enum. This fix ended up being very similar to f9d4149
where we basically can't have a range assert when loading a discriminant due to
filling drop, and appropriate flags were added to communicate this to
`trans::adt`.
This commit transitions the compiler to using the new exception handling
instructions in LLVM for implementing unwinding for MSVC. This affects both 32
and 64-bit MSVC as they're both now using SEH-based strategies. In terms of
standard library support, lots more details about how SEH unwinding is
implemented can be found in the commits.

In terms of trans, this change necessitated a few modifications:

* Branches were added to detect when the old landingpad instruction is used or
  the new cleanuppad instruction is used to `trans::cleanup`.
* The return value from `cleanuppad` is not stored in an `alloca` (because it
  cannot be).
* Each block in trans now has an `Option<LandingPad>` instead of `is_lpad: bool`
  for indicating whether it's in a landing pad or not. The new exception
  handling intrinsics require that on MSVC each `call` inside of a landing pad
  is annotated with which landing pad that it's in. This change to the basic
  block means that whenever a `call` or `invoke` instruction is generated we
  know whether to annotate it as part of a cleanuppad or not.
* Lots of modifications were made to the instruction builders to construct the
  new instructions as well as pass the tagging information for the call/invoke
  instructions.
* The translation of the `try` intrinsics for MSVC has been overhauled to use
  the new `catchpad` instruction. The filter function is now also a
  rustc-generated function instead of a purely libstd-defined function. The
  libstd definition still exists, it just has a stable ABI across architectures
  and leaves some of the really weird implementation details to the compiler
  (e.g. the `localescape` and `localrecover` intrinsics).
@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 58f1b9c force

bors added a commit that referenced this pull request Jan 30, 2016
These commits perform a few high-level changes with the goal of enabling i686 MSVC unwinding:

* LLVM is upgraded to pick up the new exception handling instructions and intrinsics for MSVC. This puts us somewhere along the 3.8 branch, but we should still be compatible with LLVM 3.7 for non-MSVC targets.
* All unwinding for MSVC targets (both 32 and 64-bit) are implemented in terms of this new LLVM support. I would like to also extend this to Windows GNU targets to drop the runtime dependencies we have on MinGW, but I'd like to land this first.
* Some tests were fixed up for i686 MSVC here and there where necessary. The full test suite should be passing now for that target.

In terms of landing this I plan to have this go through first, then verify that i686 MSVC works, then I'll enable `make check` on the bots for that target instead of just `make` as-is today.

Closes #25869
@bors
Copy link
Contributor

bors commented Jan 30, 2016

⌛ Testing commit 58f1b9c with merge 303892e...

@alexcrichton
Copy link
Member Author

🌙

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.

MSVC: Implement 32-bit Unwinding
9 participants