Implement Win64 eh_personality natively. #27210

vadimcn · Jul 22, 2015

After this change, the only remaining symbol we are pulling from libgcc on Win64 is __chkstk_ms - the stack probing routine.

rust-highfive · Jul 22, 2015

r? @brson

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

vadimcn · Jul 22, 2015

I am not quite happy about the way rust_eh_unwind_resume get linked. It should have probably been a lang item, but I couldn't quite figure out how to plumb it through to LLVMRustHookUnwindResume...

vadimcn · Jul 22, 2015

r? @alexcrichton

bors · Jul 22, 2015

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

nagisa · Jul 22, 2015

nagisa reviewed Jul 22, 2015

View changes

nagisa · Jul 22, 2015

src/doc/trpl/no-stdlib.md

-[libstd implementation](../std/rt/unwind/index.html) for more
+The second and third of these functions, `eh_personality` and `rust_eh_unwind_resume`,
+are used by the failure mechanisms of the compiler. These are often mapped to GCC's
+personality function and libgcc's `_Unwind_Resume` respectively


Wording of this sentence somehow seems to suggest GCC personality function and _Unwind_Resume come from two different locations, while in practice they both come from libgcc.

nagisa · Jul 22, 2015

nagisa reviewed Jul 22, 2015

View changes

nagisa · Jul 22, 2015

src/librustc_trans/back/write.rs

@@ -512,6 +517,13 @@ unsafe fn optimize_and_codegen(cgcx: &CodegenContext,
        None => {},
    }

+    // Inject a local-scoped `_Unwind_Resume` function, which redirects to `rust_eh_unwind_resume`.
+    // This must be done after optimizations, because until DwarfEHPrepare pass has been run,
+    // `_Unwind_Resume` is not referenced by any live code and would have been eliminated by opt.


Wouldn’t OptimizeNone prevent it from being eliminated?

This may also be able to prevent elimination through the llvm.used global

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/doc/trpl/no-stdlib.md

@@ -38,6 +38,7 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
 // provided by libstd.
 #[lang = "stack_exhausted"] extern fn stack_exhausted() {}
 #[lang = "eh_personality"] extern fn eh_personality() {}
+#[no_mangle] pub extern fn rust_eh_unwind_resume() {}


It looks like this only needs to be defined on 64-bit windows targets, so can this be omitted? I suspect 99% of #![no_std] users aren't working on Windows.

Without it doctests fail on Windows. Also, if we ever drop libgcc on other platforms, we'll need it.

Can you hide this by default? (e.g. put a # in front)

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/librustc_back/target/x86_64_pc_windows_gnu.rs

@@ -16,6 +16,7 @@ pub fn target() -> Target {
    // On Win64 unwinding is handled by the OS, so we can link libgcc statically.
    base.pre_link_args.push("-static-libgcc".to_string());
    base.pre_link_args.push("-m64".to_string());
+    base.custom_unwind_resume = true;


Should this also set the hook for x86_64-pc-windows-msvc?

I was thinking of a separate PR.

Ok, sounds good to me

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/dwarf/eh.rs

+}
+
+#[inline(never)]
+pub unsafe fn find_landing_pad(lsda: *const u8, context: EHContext) -> Option<usize> {


A few notes on this function:

Is there public documentation available for this protocol? I'd love to reference this against a known implementation or description to verify it, but also so we can understand what this is doing months from now.

Can you document why this is inline(never)?

Is there public documentation available for this protocol?

Not really. Gcc LSDA format is defined by, well, gcc implementation. Aside from that, there are a few blog posts... I'll link them here.

inline(never) isn't needed, just forgot to delete it

Yeah as many links as possible would be nice, even if they're just links to code in other repos. Surely this implementation came from somewhere, and it'd be good to have a trail saying how so!

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/dwarf/mod.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+//! General DWARF format parsing utilities


Similarly as above, it'd be great to have a lot more documentation on this module. For example the read_uleb128 function would be nice to have a hyperlink to the definition of the format, and some overall docs about what this module's doing would be nice.

Now these are defined in the DWARF standard. I'll add a section reference here.

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/gcc.rs

+            unsafe {
+                __gcc_personality_v0(version, actions, exception_class, ue_header,
+                                     context)
+            }


Just to clarify, was this a drive-by fix?

Oh hm ok I think I see what's going on here, it's because this is legit running in something other than the search phase, so we probably need to do something more than just returning a constant.

This is basically a roll-back of the rust_try_inner magic, which was created because of the way libgcc's C personality routine works on win64. I thought it'd be good to get rid of unneeded complications. This is the way this code used to be a year ago.

Right, thanks!

I'll try to make the equivalent simplification in your trans_gnu_try.

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/gcc.rs

@@ -99,8 +92,6 @@ pub mod eabi {
    }

    #[lang="eh_personality"]
-    #[no_mangle] // referenced from rust_try.ll
-    #[allow(private_no_mangle_fns)]


Does this pass all the tests? I vaguely remember seeing the rust_eh_personality symbol in a list of reachable symbols which was necessary for LTO working, but I may be remembering incorrectly.

It did, on Windows. Might be worth pushing this into try?

Ah well, if it passes all the bots then it's fine by me!

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/mod.rs

@@ -47,6 +47,9 @@ pub mod args;
 mod at_exit_imp;
 mod libunwind;

+#[cfg(all(windows, target_arch="x86_64", target_env="gnu"))]


It may be worth having this just be compiled for all platforms and it'll just end up being optimized away on most. That way we can catch breakage to it on all platforms instead of just 64-bit windows.

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+// Since these assumptions do not generally hold true for foreign exceptions (system faults,
+// C++ exceptions, etc), we make no attempt to invoke our landing pads (and, thus, destructors!)
+// for anything other than RUST_PANICs. This is considered acceptable, because the behavior of
+// throwing exceptions through a C ABI boundary is undefined.


Could you format this to 80-chars wide? (what the rest of the runtime is formatted to)

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+
+    if er.ExceptionFlags & EXCEPTION_UNWIND == 0 { // we are in the dispatch phase
+        if er.ExceptionCode == RUST_PANIC {
+            if let Some(lpad) = find_landing_pad(dc) {


This may be a bit clearer as:

let lpad = find_landing_pad(dc).unwrap_or_else(|| { rtabort!(...) });

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+
+#[lang="eh_personality"]
+#[cfg(not(test))]
+unsafe extern "C" fn rust_eh_personality(


Is this just the same as rust_eh_personality_catch? These two functions look the same except for the expectation of the landing pad existing...

Overall I'd find it at least helpful to have a nice big doc comment at the top of the module explaining what's going on here. I have a little understanding of what mechanisms are in play, but I know why these are structured the way they are. For example it's not clear to my why the catch personality is catching but this one isn't?

Is this just the same as rust_eh_personality_catch? These two functions look the same except for the expectation of the landing pad existing...

Yes, that the only difference at the moment. There used to be more when I tried to support running destructors on foreign exceptions.

Interesting... I guess this shows how little I know about the ABI of these exceptions on windows! I guess whether a landing pad is a "catch" landing pad is encoded in the landing pad itself and not the personality function?

Overall it basically may mean that we can merge these two functions and only have one?

alexcrichton · Jul 22, 2015

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton · Jul 22, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+    ExceptionContinueSearch
+}
+
+#[no_mangle] // referenced from landing pads


Can you expand this to a comment explaining what this function is doing? It'd be nice to mention that this is called from all landing pads and is injected by LLVM, and it's also why it's pub + #[no_mangle].

Also, can you remove the "C" part from the header?

alexcrichton · Jul 22, 2015

Nice work @vadimcn!

liigo · Jul 23, 2015

liigo reviewed Jul 23, 2015

View changes

liigo · Jul 23, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+//    [24:27] = type
+//    [0:23]  = magic
+const ETYPE: DWORD = 0b1110_u32 << 28;
+const MAGIC: DWORD = 0x525354; // "RST"


RST also refers to reStructuredText. How about we use .rs here?

const MAGIC: DWORD = 0x2e7273; // ".rs"

brson · Jul 23, 2015

Epic patch.

vadimcn · Jul 24, 2015

Addressed comments and rebased on top of master. r?

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/libstd/rt/unwind/mod.rs

 pub mod imp;
-#[cfg(not(target_env = "msvc"))] #[path = "gcc.rs"] #[doc(hidden)]
+
+// x86-pc-windows-gnu and all others


s/x86/i686/

alexcrichton · Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton · Jul 24, 2015

src/libstd/rt/unwind/seh64_gnu.rs

+// `_Unwind_Resume` routine.  To avoid confusion with the same symbol exported
+// from libgcc, we redirect it to `rust_eh_unwind_resume`.
+// Since resolution of this symbol is done by the linker, `rust_eh_unwind_resume`
+// must be marked `pub` + `#[no_mangle]`.  (Can we make it a lang item?)


Unfortunately I don't think this can be a lang item because libraries like libcore/libcollections need to reference this (they're generating landing pads) so it needs to have a well known symbol name (e.g. it's used before it's defined).

vadimcn · Aug 2, 2015

Works now. r?

alexcrichton · Aug 2, 2015

@bors: r+

bors · Aug 2, 2015

📌 Commit e493027 has been approved by alexcrichton

bors · Aug 2, 2015

⌛️ Testing commit e493027 with merge 2c60a05...

bors · Aug 2, 2015

💔 Test failed - auto-linux-64-x-android-t

Gankro · Aug 2, 2015

Seems legit:


../src/libstd/rt/unwind/gcc.rs:231:38: 231:43 error: use of moved
value: `state` [E0382]
../src/libstd/rt/unwind/gcc.rs:231
__gcc_personality_v0(state, ue_header, context)
                                                                        ^~~~~
../src/libstd/rt/unwind/gcc.rs:225:13: 225:18 note: `state` moved here
because it has type `rt::libunwind::_Unwind_State`, which is
non-copyable
../src/libstd/rt/unwind/gcc.rs:225         if (state as c_int &
uw::_US_ACTION_MASK as c_int)
                                               ^~~~~
error: aborting due to previous error
make: *** [x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib/stamp.std]
Error 101
make: *** Waiting for unfinished jobs....

On Sun, Aug 2, 2015 at 12:08 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-x-android-t
http://buildbot.rust-lang.org/builders/auto-linux-64-x-android-t/builds/5916

—
Reply to this email directly or view it on GitHub
#27210 (comment).

vadimcn · Aug 3, 2015

Fixed. Let's try again?

alexcrichton · Aug 3, 2015

@bors: r+ 96d1db2

bors · Aug 3, 2015

⌛️ Testing commit 96d1db2 with merge d99b3c5...

bors · Aug 3, 2015

💔 Test failed - auto-mac-64-opt

alexcrichton · Aug 3, 2015

@bors: retry

On Mon, Aug 3, 2015 at 10:37 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/5957

—
Reply to this email directly or view it on GitHub
#27210 (comment).

bors · Aug 3, 2015

⌛️ Testing commit 96d1db2 with merge ceded6a...

bors · Aug 3, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

alexcrichton · Aug 3, 2015

🎊

rust-highfive assigned brson Jul 22, 2015

rust-highfive assigned alexcrichton and unassigned brson Jul 22, 2015

brson added the relnotes label Jul 23, 2015

vadimcn force-pushed the vadimcn:win64-eh-pers branch 2 times, most recently from 502a189 to 59d5b19 Jul 24, 2015

vadimcn force-pushed the vadimcn:win64-eh-pers branch from 59d5b19 to 63049c7 Jul 24, 2015

bors added a commit that referenced this pull request Aug 2, 2015

Auto merge of #27210 - vadimcn:win64-eh-pers, r=alexcrichton
After this change, the only remaining symbol we are pulling from libgcc on Win64 is `__chkstk_ms` - the stack probing routine.

2c60a05

Fix compile errors for ARM.

Loading status checks…

96d1db2

bors added a commit that referenced this pull request Aug 3, 2015

Auto merge of #27210 - vadimcn:win64-eh-pers, r=alexcrichton
After this change, the only remaining symbol we are pulling from libgcc on Win64 is `__chkstk_ms` - the stack probing routine.

d99b3c5

bors added a commit that referenced this pull request Aug 3, 2015

Auto merge of #27210 - vadimcn:win64-eh-pers, r=alexcrichton
After this change, the only remaining symbol we are pulling from libgcc on Win64 is `__chkstk_ms` - the stack probing routine.

Loading status checks…

ceded6a

bors merged commit 96d1db2 into rust-lang:master Aug 3, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

This was referenced Aug 3, 2015

Merged

rustc: Allow changing the default allocator #27400

Merged

Improve handling of dllexport #27416

Closed

Port Rust to PNaCl/JS (rebase #1) #26505

alexcrichton referenced this pull request Aug 11, 2015
Merged
trans: Re-enable unwinding on 64-bit MSVC #27676

vadimcn deleted the vadimcn:win64-eh-pers branch Oct 9, 2015

rust-lang/rust

Join GitHub today

Implement Win64 eh_personality natively. #27210

Conversation

vadimcn commented Jul 22, 2015

rust-highfive assigned brson Jul 22, 2015

This comment has been minimized.

rust-highfive commented Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 22, 2015

rust-highfive assigned alexcrichton and unassigned brson Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

nagisa reviewed Jul 22, 2015 View changes

This comment has been minimized.

nagisa reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 22, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Jul 22, 2015

liigo reviewed Jul 23, 2015 View changes

This comment has been minimized.

brson added the relnotes label Jul 23, 2015

This comment has been minimized.

brson commented Jul 23, 2015

vadimcn force-pushed the vadimcn:win64-eh-pers branch 2 times, most recently from 502a189 to 59d5b19 Jul 24, 2015

This comment has been minimized.

vadimcn commented Jul 24, 2015

vadimcn force-pushed the vadimcn:win64-eh-pers branch from 59d5b19 to 63049c7 Jul 24, 2015

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 24, 2015 View changes

This comment has been minimized.

This comment has been minimized.

vadimcn commented Aug 2, 2015

This comment has been minimized.

alexcrichton commented Aug 2, 2015

This comment has been minimized.

bors commented Aug 2, 2015

This comment has been minimized.

nagisa reviewed Jul 22, 2015

View changes

nagisa reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

alexcrichton reviewed Jul 22, 2015

View changes

liigo reviewed Jul 23, 2015

View changes

vadimcn force-pushed the vadimcn:win64-eh-pers branch 2 times, most recently from `502a189` to `59d5b19` Jul 24, 2015

vadimcn force-pushed the vadimcn:win64-eh-pers branch from `59d5b19` to `63049c7` Jul 24, 2015

alexcrichton reviewed Jul 24, 2015

View changes

alexcrichton reviewed Jul 24, 2015

View changes

bors merged commit `96d1db2` into rust-lang:master Aug 3, 2015
2 checks passed