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

Statically link libstdc++ on windows-gnu #65911

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Oct 28, 2019

Fixes #61561 by not shipping libstdc++-6.dll which can conflict with the GCC.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Oct 28, 2019
@mati865
Copy link
Contributor Author

mati865 commented Oct 28, 2019

r? @alexcrichton

@mati865
Copy link
Contributor Author

mati865 commented Oct 29, 2019

Cc @Amanieu who found the cause and could be interested in this PR.

@@ -279,7 +279,11 @@ fn main() {
let path = PathBuf::from(s);
println!("cargo:rustc-link-search=native={}",
path.parent().unwrap().display());
println!("cargo:rustc-link-lib=static={}", stdcppname);
if target.contains("windows") {
println!("cargo:rustc-link-lib=static-nobundle={}", stdcppname);
Copy link
Member

Choose a reason for hiding this comment

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

This may cause distribution issues since we ship rustc_llvm and users like clippy/plugins will link to rustc_llvm transitively through rustc_driver, was there a reason though static didn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think quite nasty or I'm just misunderstanding.

First let's take a look at static libstdc++:

nm /mingw64/lib/gcc/x86_64-w64-mingw32/9.2.0/libstdc++.a | grep pthread_mutex_init
                 U __imp_pthread_mutex_init
                 U pthread_mutex_init
                 U pthread_mutex_init
                 [..]

There are references to both static and dynamic symbols!

Now using static failed undefined with 2 undefined symbols: __imp_pthread_mutex_{destroy,init}. I honestly have no idea why picked dynamic symbols, though I haven't chased that rabbit.
It could be mitigated by dynamically linking to winpthread library (by removing static in line 298), windows-gnu ships libwinpthread-1.dll anyway because. But I felt it's just replacing one problem with another.

With some guidance I could try to investigate why picks dynamic pthread symbols when using static but static-nobundle seems to fit here (I asked for try build in OP to be sure).

Copy link
Member

Choose a reason for hiding this comment

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

I had the same issue in #65646, with a similar workaround: a505735

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok, I'm not really sure what's going on with static vs static-nobundle, but now that I think about this we're linking this into librustc_driver.so where the "nobundle" part won't keep propagating, so this doesn't actually affect consumers, and as a result should be fine to land.

@alexcrichton
Copy link
Member

Want to revert the CI configuration back to the default? Otherwise r=me?

@mati865
Copy link
Contributor Author

mati865 commented Oct 30, 2019

I thought you might want to give it @bors try run first to do more testing but if you are fine with landing this right away I'll revert CI change.

@mati865
Copy link
Contributor Author

mati865 commented Oct 30, 2019

Removed CI changing commit.

@alexcrichton
Copy link
Member

Oh sorry I misread the ci changes, can this run the mingw test on this pr to ensure they pass before we send to bors?

@mati865 mati865 force-pushed the static-libstdcxx-mingw branch 5 times, most recently from c8b551a to b20d8d3 Compare October 31, 2019 10:40
@mati865
Copy link
Contributor Author

mati865 commented Oct 31, 2019

All mingw tests are green: https://dev.azure.com/rust-lang/rust/_build/results?buildId=12392&view=logs&jobId=5ac1211a-cbe7-52be-dcc2-57e3991fd20c
Dist builds were not tested but I believe they will be fine.

@alexcrichton
Copy link
Member

👍

Sounds good to me!

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 31, 2019

📌 Commit b20d8d3 has been approved by alexcrichton

@bors 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 Oct 31, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 1, 2019
…lexcrichton

Statically link libstdc++ on windows-gnu

Fixes rust-lang#61561 by not shipping `libstdc++-6.dll` which can conflict with the GCC.
tmandry added a commit to tmandry/rust that referenced this pull request Nov 1, 2019
…lexcrichton

Statically link libstdc++ on windows-gnu

Fixes rust-lang#61561 by not shipping `libstdc++-6.dll` which can conflict with the GCC.
@bors
Copy link
Contributor

bors commented Nov 5, 2019

⌛ Testing commit b20d8d3 with merge d2185f6...

bors added a commit that referenced this pull request Nov 5, 2019
Statically link libstdc++ on windows-gnu

Fixes #61561 by not shipping `libstdc++-6.dll` which can conflict with the GCC.
@bors
Copy link
Contributor

bors commented Nov 5, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing d2185f6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2019
@bors bors merged commit b20d8d3 into rust-lang:master Nov 5, 2019
@mati865 mati865 deleted the static-libstdcxx-mingw branch November 5, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

couldn't load codegen backend on windows-gnu
6 participants