Add `__CxxFrameHandler3` in `panic_abort` #83607

Berrysoft · 2021-03-28T13:15:16Z

I initially tried to forward to __CxxFrameHandler3 from rust_eh_personality, but later I found that LLVM uses handler names to distinguish exception type. Therefore I choose to add __CxxFrameHandler3 in panic_abort. Anyway it solves the link problem, and this function will never be called.

It seems that the original issue was solved, but still adding these tests.

rust-highfive · 2021-03-28T13:15:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Berrysoft · 2021-03-28T14:49:01Z

How to test for MSVC?

nagisa · 2021-03-28T16:59:35Z

You should add a test case under src/test as a regression test.

dtolnay

I don't know anything about the panic_abort crate. 😕
r? @nagisa -- to assess whether the new test is adequate

FYI @rylev in case this issue means anything to you or you know an appropriate reviewer for msvc.

rylev · 2021-03-31T08:12:50Z

Ping to @danielframpton for awareness.

It seems like #54137 is no longer an issue. The tests added in this PR all already pass even without the change adding the __CxxFrameHandler3 symbol. Now, in order to trigger the missing symbol issue, panic must be set to "unwind" not to "abort".

It also looks like this issue was introduced relatively recently. The code here does not reproduce the issue on nightlies older than 2021-03-11. I did a cargo bisect run and found that #76570 seems to be the culprit - perhaps LLVM can no longer prove that unwinding won't happen. Pinging @cratelyn for awareness as she was the author of that PR.

Additionally, it seems like the linker is expecting the _fltused symbol and fails to find one. This is why one of the tests implements _fltused. This very much seems like a bug.

So with this in mind, I believe the following is what needs to be done:

Close Missing symbol in no_std application on panic (windows-msvc) #54137 as the issue it is reporting on (namely a missing __CxxFrameHandler3 in no_std when panic="abort") no longer seems to be an issue.
Try to better understand why after Implement RFC 2945: "C-unwind" ABI #76570, the __CxxFrameHandler3 is needed in panic-unwind cases. The solution in this PR of defining that symbol does not seem to be the right one. Do we need to open a new issue or is this PR enough for tracking?
Make sure the tests that were added in this PR are not duplicates of existing tests since they already pass before the addition of __CxxFrameHandler3. If they aren't, it would be great to keep them even if they aren't addressing the issue at hand. We also need to add a test that does fail (namely, when panic = "unwind").
Add a new issue for the fact that the _fltused is expected. This seems to also be a bug and seems to impact any no_std application not linking to MSVCRT.

Berrysoft · 2021-03-31T09:25:47Z

I agree with most of @rylev 's points, but I don't think the requirement of __CxxFrameHandler3 when panic=unwind is a problem.

rust/compiler/rustc_codegen_llvm/src/context.rs

Lines 383 to 387 in 6e17a5c

    
           let name = if wants_msvc_seh(self.sess()) { 
        
               "__CxxFrameHandler3" 
        
           } else { 
        
               "rust_eh_personality" 
        
           };

LLVM distinguishes exception type with name, so hard coding __CxxFrameHandler3 is the only way to use SEH with MSVC toolchains.

src/test/ui/panic-runtime/abort-msvc-no-std-link-to-libcmt.rs

src/test/ui/panic-runtime/abort-msvc-no-std-link-to-msvcrt.rs

Berrysoft · 2021-03-31T14:36:58Z

It seems that there's no test for no_std under panic-runtime folder, thus I think these tests are necessary. And I don't think tests for no_std + panic_unwind are necessary, because unwind feature needs some libc features, which is conflict to what no_std for.

rylev · 2021-03-31T15:51:14Z

@Berrysoft no_std + panic_unwind definitely still makes sense. no_std simply means not linking to the Rust standard library. If that still imposes dependencies on DLLs like MSVCRT.dll, I don't think that's necessarily an issue.

Berrysoft · 2021-04-01T12:37:58Z

I added to tests for panic_unwind + msvcrt/libcmt, but didn't add test for panic_unwind without __CxxFrameHandler3, because it is hard to write full stderr output.

bjorn3 · 2021-04-01T12:57:55Z

src/test/ui/panic-runtime/abort-msvc-no-std.rs

+
+#[no_mangle]
+pub extern "C" fn mainCRTStartup() -> ! {
+    main()


Can you add a call to a function that may panic, drops a value on panic and is not inlined from a crate that is compiled with -Cpanic=unwind? In that case the personality function should be referenced.

~~A crate compiled with panic=abort can't import a crate compiled with panic=unwind.~~

Oh, I see. They could be statically linked.

But what's the expected result? When both crates use std, it wouldn't be a problem, and the executable will run normally without any error; but when both use no_std, there's no unwind runtime. Should the executable pass or abort?

The called function should not actually panic when running the executable. It should be added merely to check that there is no linking error when trying to call such a function.

Ok, you're right. __CxxFrameHandler3 is required in that case. Should I add back the empty implementation?

I think so.

Berrysoft · 2021-04-03T05:42:42Z

The three abort test is marked with build-pass instead of run-pass, as the executables call rust_begin_unwind, which is just a loop{} in panic_abort

nagisa · 2021-04-04T13:10:00Z

Perfectly fine to keep this as build-pass tests IMO.

r=me after the commits are squashed.

Berrysoft · 2021-04-05T15:22:30Z

@nagisa squashed.

nagisa · 2021-04-05T17:16:43Z

@bors r+

bors · 2021-04-05T17:16:44Z

📌 Commit 48acdb7 has been approved by nagisa

Add `__CxxFrameHandler3` in `panic_abort` Fix rust-lang#54137 <del>I initially tried to forward to `__CxxFrameHandler3` from `rust_eh_personality`, but later I found that LLVM uses handler names to distinguish exception type. Therefore I choose to add `__CxxFrameHandler3` in `panic_abort`. Anyway it solves the link problem, and this function will never be called.</del> It seems that the original issue was solved, but still adding these tests.

JohnTitor · 2021-04-05T21:15:34Z

Failed in rollup: #83897 (comment)
@bors r-

Berrysoft · 2021-04-06T04:43:53Z

Seems caused by i686-msvc, and I'll check later. Why there're so many CRT symbols required even in core?

Berrysoft · 2021-04-06T14:08:04Z

There are 3 more symbols needed for x86 msvc target: _aulldiv, _aullrem, and _load_config_used. _aulldiv and _aullrem are used for 64-bit int calculations, and _load_config_used is used for safe exception handlers.

The first two could be resolved by link against ntdll, but the last one needs some more works.

rust/compiler/rustc_target/src/spec/i686_pc_windows_msvc.rs

Lines 12 to 15 in 505ed7f

    
           // Ensure the linker will only produce an image if it can also produce a table of 
        
           // the image's safe exception handlers. 
        
           // https://docs.microsoft.com/en-us/cpp/build/reference/safeseh-image-has-safe-exception-handlers 
        
           "/SAFESEH".to_string(),

As we just abort when panicking, do we really need safe exception handlers?

And note that if /SAFESEH is not specified, safe exception handlers will still be used when all modules are compatible with that feature. It shouldn't be a big problem to remove that flag.

rylev · 2021-04-06T15:25:36Z

@Berrysoft @nagisa I'm a bit confused and unsure if this is the right thing to do. Presumably, while this fixes the linker error, the resulting binary won't work properly right? Shouldn't we either be forcing the user to link to the MSVC runtime for exception handling, or providing an error and saying that it's not possible to unwind on MSVC targets without linking to the runtime? The proposed change seems like a hack to just make the linker happy, but it doesn't actually produce working binaries.

rylev · 2021-04-06T15:28:10Z

src/test/ui/panic-runtime/unwind-msvc-no-std-link-to-libcmt.rs

+#[global_allocator]
+static ALLOC: DummyAllocator = DummyAllocator;
+
+#[alloc_error_handler]


What's the reason for bringing in alloc?

panic_unwind needs that.

Is there a reason we're using the panic_unwind feature and not #[panic_handler] with an eh_personality item? This would remove the need for the panic_unwind and alloc_error_handler features. I believe that's the more common and less verbose way to handle panics in a custom way.

I'm only testing a no_std crate with panic_unwind feature. Do you mean that panic_unwind doesn't need a panic_handler?

I guess I'm asking if testing the panic_unwind feature is the right test here. We want to test custom panic handling, and the panic_unwind feature is one way to do that. Another is simply adding a panic_handler and eh_personality. Perhaps we should add another test for that?

Thoughts here?

Do you mean adding a test with panic=unwind feature and a custom panic handler? It seems somehow complicated to write a robust unwind panic handler...

Berrysoft · 2021-04-06T15:34:01Z

@rylev Yes, linking a crate with panic=abort and a crate with panic=unwind won't produce working binaries, even if I remove the empty implemention of __CxxFrameHandler3, because they will hang at rust_begin_unwind.

After all these attempts, I'm agree with either of these solutions:

Don't support panic=unwind with no_std on Windows.
Don't support mixing panic=abort and panic=unwind with no_std on Windows.
Remove /SAFESEH requirement.
Link against vcruntime140 with no_std on Windows.

nagisa · 2021-04-06T15:59:19Z

@rylev my reasoning in approving this is that the change should not make anything worse. And it might help somebody to produce a working binary if they are careful enough.

I'm glad that you brought a different viewpoint to this though.

Don't support mixing panic=abort and panic=unwind with no_std on Windows.

Hm, you already can't mix panic=abort and panic=unwind. It seems, however, that -Cpanic=unwind isn't actually what is written on the tin, though. To really say that you want -Cpanic=unwind, no matter what, you must extern crate panic_unwind which is unstable.

rylev · 2021-04-07T11:41:01Z

I might be missing something, but it seems to me that panic=unwind without linking MSCRT doesn't make any sense, and it probably shouldn't be supported as unwinding on Windows requires MSCRT support.

I'm still not sure what should happen in the case of statically linking a crate compiled with panic=unwind to a crate with panic=abort. It seems like without including the MSCRT this should also be a linker error, no?

Are we stuck with just a linker errors or is there a way to provide the user with better error messages?

rylev · 2021-04-07T11:58:40Z

src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std-link-to-libcmt.rs

+// only-msvc
+// Test that `no_std` with `panic=abort` under MSVC toolchain
+// doesn't cause error when linking to libcmt.
+// We don't run this executable because it will hang in `rust_begin_unwind`


Do we expect that this should actually run successfully? If the MSCRT is linked in statically, then we should be able to handle linking another crate which is compiled with -Cpanic=unwind, no?

At first, I expect it fail because panic! is in a panic=abort crate, and unwind won't occur; but it hangs in rust_begin_unwind.

I solved this problem now. It should be able to run and fail.

This comment should be changed then.

rylev · 2021-04-07T11:59:26Z

src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std-link-to-msvcrt.rs

+// compile-flags: -C panic=abort
+// aux-build:exit-success-if-unwind-msvc-no-std.rs
+// only-msvc
+// Test that `no_std` with `panic=abort` under MSVC toolchain


Do we also expect this to run successfully (even though it doesn't currently)? Since MSCRT is linked in?

klensy · 2021-04-08T18:31:15Z

Btw, current rustc requires msvc 2019 to build (at least i can't build it with 2017 version, in the past), that have __CxxFrameHandler4 (and other ones, like 3, 2). __CxxFrameHandler3 from linked issue used msvc 2017.

Checked currently build exe file, it uses __CxxFrameHandler3, hmm.

Berrysoft · 2021-04-09T03:28:18Z

__CxxFrameHandler4 is only avaliable for x86_64 arch, and actually it up to LLVM support. Nowadays LLVM still distinguish exception handler type by name, or specifically, __CxxFrameHandler3.

nagisa · 2021-04-24T21:22:14Z

r? @rylev (but see my observations above)

JohnCSimon · 2021-05-09T19:28:21Z

looks like this is ready to review

rylev

I guess we can merge this since it shouldn't cause any issues, but this is definitely still not in a complete steady state.

There are a few tests that don't seem minimal in that they're testing multiple things at once. It would be good to break them up. For instance, there is one test that tests panic=unwind with a panic_unwind feature. We also need to test without using the panic_unwind feature and just using a custom panic_handler.

rylev · 2021-05-17T15:31:00Z

src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std-link-to-libcmt.rs

+// only-msvc
+// Test that `no_std` with `panic=abort` under MSVC toolchain
+// doesn't cause error when linking to libcmt.
+// We don't run this executable because it will hang in `rust_begin_unwind`


This comment should be changed then.

rylev · 2021-05-17T15:32:06Z

src/test/ui/panic-runtime/auxiliary/exit-success-if-unwind-msvc-no-std.rs

@@ -0,0 +1,32 @@
+// compile-flags:-C panic=unwind


It would be good to add a comment explaining what this helper crate does at a high level.

rylev · 2021-05-17T15:35:24Z

src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std-link-to-libcmt.rs

@@ -0,0 +1,25 @@
+// run-fail
+// compile-flags: -C panic=abort -C target-feature=+crt-static


I'm slightly confused about -C target-feature=+crt-static. We're statically linking the crt but also dynamically linking libcmt. Shouldn't we be testing on or the other.

libcmt is the static C runtime library. It is a static library, though it should be linked dynamically. I just cannot tell why it needs dynamic linking. It should make no difference between dynamic linking and static linking.

+crt-static will already link libcmt so there should be no reason to declare it manually.

That's right when using libc crate, but here we need to test under no_std.

rylev · 2021-05-17T15:35:49Z

src/test/ui/panic-runtime/abort-link-to-unwind-msvc-no-std.rs

+// aux-build:exit-success-if-unwind-msvc-no-std.rs
+// no-prefer-dynamic
+// only-msvc
+// We don't run this executable because it will hang in `rust_begin_unwind`


This comment is wrong now

rylev · 2021-05-17T15:37:34Z

src/test/ui/panic-runtime/unwind-msvc-no-std-link-to-libcmt.rs

+#[global_allocator]
+static ALLOC: DummyAllocator = DummyAllocator;
+
+#[alloc_error_handler]


Thoughts here?

rylev · 2021-05-19T09:57:39Z

@Berrysoft I think what's missing is a good overview of what each test is supposed to be covering. For instance, +crt-static seems to be a noop in no-std. Is that intentional?

I wonder if it's better for us to close this PR, and open individual ones for each type of test we're adding explaining what the test is for. Right now it's hard for me to get an overview of what use cases we're trying to cover, especially when the linked issue is actually not being addressed here (because it's no longer an issue).

Berrysoft · 2021-05-20T05:13:20Z

I do think this PR is somehow miscellaneous. Anyway the issue I originally wanted to address had been solved, and we found so many different problems here, which made me a little bit tired. I'd like to close this PR, and will consider to open individual ones for each type of test in the future.

rust-highfive assigned dtolnay Mar 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2021

Berrysoft changed the title ~~Forward to __CxxFrameHandler3 in rust_eh_personality.~~ Add __CxxFrameHandler3 in panic_abort Mar 29, 2021

dtolnay reviewed Mar 30, 2021

View reviewed changes

dtolnay assigned nagisa and unassigned dtolnay Mar 30, 2021

bjorn3 reviewed Mar 31, 2021

View reviewed changes

src/test/ui/panic-runtime/abort-msvc-no-std-link-to-libcmt.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 31, 2021

View reviewed changes

src/test/ui/panic-runtime/abort-msvc-no-std-link-to-msvcrt.rs Outdated Show resolved Hide resolved

Berrysoft changed the title ~~Add __CxxFrameHandler3 in panic_abort~~ Add tests for no_std panic with MSVC toolchain Apr 1, 2021

bjorn3 reviewed Apr 1, 2021

View reviewed changes

Berrysoft changed the title ~~Add tests for no_std panic with MSVC toolchain~~ Add __CxxFrameHandler3 in panic_abort Apr 1, 2021

Berrysoft added 3 commits April 4, 2021 22:50

Add empty __CxxFrameHandler3 impl in panic_abort

af7c540

Add tests for abort link to unwind.

9cdaea3

Add tests for unwind no_std.

48acdb7

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021

Dylan-DPC-zz mentioned this pull request Apr 5, 2021

Rollup of 5 pull requests #83897

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 5, 2021

Fix ExitProcess linkage.

fc4bf6d

Link to ntdll to fix some crt symbols

c162f8a

rylev reviewed Apr 6, 2021

View reviewed changes

rylev reviewed Apr 7, 2021

View reviewed changes

Run test for abort-link-to-unwind.

7a54baa

rust-highfive assigned rylev and unassigned nagisa Apr 24, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2021

rylev requested changes May 17, 2021

View reviewed changes

rylev mentioned this pull request May 19, 2021

Missing symbol in no_std application on panic (windows-msvc) #54137

Closed

Berrysoft closed this May 20, 2021

		@@ -0,0 +1,25 @@
		// run-fail
		// compile-flags: -C panic=abort -C target-feature=+crt-static

Add __CxxFrameHandler3 in panic_abort #83607

Add __CxxFrameHandler3 in panic_abort #83607

Conversation

Berrysoft commented Mar 28, 2021 • edited Loading

rust-highfive commented Mar 28, 2021

Berrysoft commented Mar 28, 2021

nagisa commented Mar 28, 2021

dtolnay left a comment

Choose a reason for hiding this comment

rylev commented Mar 31, 2021

Berrysoft commented Mar 31, 2021

Berrysoft commented Mar 31, 2021 • edited Loading

rylev commented Mar 31, 2021

Berrysoft commented Apr 1, 2021

Choose a reason for hiding this comment

Berrysoft Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Berrysoft commented Apr 3, 2021

nagisa commented Apr 4, 2021

Berrysoft commented Apr 5, 2021

nagisa commented Apr 5, 2021

bors commented Apr 5, 2021

JohnTitor commented Apr 5, 2021

Berrysoft commented Apr 6, 2021

Berrysoft commented Apr 6, 2021 • edited Loading

rylev commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Berrysoft commented Apr 6, 2021

nagisa commented Apr 6, 2021

rylev commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy commented Apr 8, 2021 • edited Loading

Berrysoft commented Apr 9, 2021

nagisa commented Apr 24, 2021

JohnCSimon commented May 9, 2021

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented May 19, 2021

Berrysoft commented May 20, 2021

Add `__CxxFrameHandler3` in `panic_abort` #83607

Add `__CxxFrameHandler3` in `panic_abort` #83607

Berrysoft commented Mar 28, 2021 •

edited

Loading

Berrysoft commented Mar 31, 2021 •

edited

Loading

Berrysoft Apr 1, 2021 •

edited

Loading

Berrysoft commented Apr 6, 2021 •

edited

Loading

klensy commented Apr 8, 2021 •

edited

Loading