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

MinGW: move unwind linkage to libunwind #71001

Closed

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 10, 2020

Also adds test to make sure binaries can run without external libraries (like libgcc*) in the PATH.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2020
@mati865
Copy link
Contributor Author

mati865 commented Apr 10, 2020

cc @Amanieu

@nikomatsakis
Copy link
Contributor

@mati865 can you summarize what the changes in this PR are trying to do exactly? (i.e., which platforms are affected, etc)

@Amanieu
Copy link
Member

Amanieu commented Apr 10, 2020

If I understand the semantics of static-nobundle correctly then this change is incorrect.

src/librustc_codegen_ssa/back/link.rs contains this comment:

// Link "static-nobundle" native libs only if the crate they originate from
// is being linked statically to the current crate.  If it's linked dynamically
// or is an rlib already included via some other dylib crate, the symbols from
// native libs will have already been included in that dylib.

When we build rustc, we will create a std-XXX.dll which includes the panic-unwind crate. Based on the above comment, this means that the std-XXX.dll will statically link against libgcc-eh.a. This is wrong since it means that _Unwind_RaiseException from libgcc_eh.a will be used to throw panics but the main compiler code in rustc_driver-XXX.dll will use the unwinding runtime in libgcc_s-dw2-1.dll.

Have you tested these changes on Windows with the compiler testsuite? I would expect the compiler to crash when failing to unwind on some of the tests.

@mati865
Copy link
Contributor Author

mati865 commented Apr 10, 2020

@mati865 can you summarize what the changes in this PR are trying to do exactly? (i.e., which platforms are affected, etc)

Seems like it doesn't matter any more, sorry for the noise.

Have you tested these changes on Windows with the compiler testsuite? I would expect the compiler to crash when failing to unwind on some of the tests.

Passed all the tests (including new one) natively on x86_64-pc-windows-gnu and I was about to check i686 but accidentally opened the PR too soon.
Finally after solving x.py bug where builds were returning success after ~3 seconds since start without actually running tests I see i686 tests are failing.

When we build rustc, we will create a std-XXX.dll which includes the panic-unwind crate. Based on the above comment, this means that the std-XXX.dll will statically link against libgcc-eh.a. This is wrong since it means that _Unwind_RaiseException from libgcc_eh.a will be used to throw panics but the main compiler code in rustc_driver-XXX.dll will use the unwinding runtime in libgcc_s-dw2-1.dll.

That's really unfortunate, I was hoping to easily replace libgcc_eh with libunwind when linking statically.
I suppose there is no point in keeping code from this PR for x86_64 only. Do you think the test is useful enough to commit it separately?

@mati865 mati865 closed this Apr 10, 2020
@Amanieu
Copy link
Member

Amanieu commented Apr 10, 2020

Yes, the test seems pretty useful on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants