-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Don't dynamically link LLVM tools unless rustc is too #76810
Conversation
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
@bors rollup=never I suspect the original PR had essentially no effect as a result of this "bug" so we'll want the no-rolling policy here too. |
@bors r+ |
📌 Commit 7a8fedf313e05e43208144a35bde20861a59f441 has been approved by |
⌛ Testing commit 7a8fedf313e05e43208144a35bde20861a59f441 with merge 50a9d19a155b90d56767ece8b126a9477b703091... |
💔 Test failed - checks-azure |
|
r? @alexcrichton -- for context, this is "just" a continuation of #76708 which essentially is just a re-implementation of that PR. The first commit fixes the enabling of link-shared and the second fixes builds with link-shared on macOS. My impression is that before we landed #76349 (e.g., on current stable) rustc_driver and rustc in general statically linked to LLVM, but we still shipped a libLLVM.dylib, and tools (like rust-lld) linked to that. After #76349 changed the logic to say "if llvm.link-shared is false, don't put the LLVM dylib in the sysroot," this broke tools like rust-lld on macOS. This PR, then, enables shared linking of LLVM to the rustc binary/rustc_driver, and fixes the LLVM build on macOS to support shared linking. This provides us with two benefits:
We could explore an alternative which tries to statically link llvm tools, like we do on Windows, but we'd then likely not be able to support the CI-built LLVM locally, which seems unfortunate. This PR is not something I'm personally a fan of -- I guess the "proper" thing to do is probably to try and talk to LLVM build system maintainers to try and work out why the macOS shared linking story is so oddly constructed with various parts of LLVM creating and doing different things (see commit message and comments for details). I tried to do some git history searching and googling but wasn't able to find anything that hinted at answers. |
📌 Commit 546fb5189f95393316cb84552cf7d8c522145b28 has been approved by |
⌛ Testing commit 546fb5189f95393316cb84552cf7d8c522145b28 with merge 384fb80f1c96953f5e787a17d20fe5695416a020... |
💔 Test failed - checks-actions |
and a whole slew of other linker errors following. |
35c7927
to
03e5e45
Compare
r? @alexcrichton since the latest update represents a pretty major change to this PR I've given up on trying to make link-shared work well across the board -- see PR description and the last few CI failures -- it seems link LLVM is just not ready for a shared link on macOS or when cross-compiling, which excludes almost all of our builders :) That said, this took the approach of disabling the shared LLVM link if we're not doing so with rustc, but it could also take the approach of editing dist.rs to distribute libLLVM.dylib (or the platform equivalent) whenever we're dynamically linking LLVM tools. I think I personally prefer to statically link -- we're likely to have slightly smaller aritfacts as a result and overall statically linking things is just much simpler. But I'm happy to do whichever, though I've not tested. I'm currently building this in docker and on a mac, but given that this approach is much simpler, I'm pretty confident in it working out. |
Previously we would have some platforms where LLVM was linked to rustc statically, but to the LLVM tools dynamically. That meant we were distributing two copies of LLVM: one as a separate dylib and one statically linked in to librustc_driver.
03e5e45
to
389b7ff
Compare
I've confirmed that the latest commit seems to fix things on macOS (link-shared still errors, now explicitly rather than opaquely). |
@Mark-Simulacrum Is there a temporary solution? I tried to compile with the stable version and the same problem occurred. This problem has been bothering me for a week, our team cannot proceed to the next step. |
Stable and beta branches should not be affected by this bug: if you're seeing a bug there, please file a new issue with a description of how to reproduce what you're seeing and cc me. |
@Mark-Simulacrum sorry, my bad. i hadn't restart the terminal. now i'm using the stable version. but some api just run on nightly version. |
@bors: r+ Longer-term I do think it's best to get back to a point where we're consistently doing the same thing for LLVM across all our platforms. It's been the case for quite some time that LLVM is built with ThinLTO on Linux but nowhere else. Differing in linkage is also unfortunate across the major platforms too. It seems good though to get things working again at least... |
📌 Commit 389b7ff has been approved by |
☀️ Test successful - checks-actions, checks-azure |
This PR initially tried to support link-shared on all of our target platforms (other than Windows), but ran into a number of difficulties:
So, this PR has now been revised such that we don't attempt to dynamically link LLVM tools (even if that would, otherwise, be supported) on targets where LLVM is statically linked to rustc. Currently that's basically everything except for x86_64-unknown-linux-gnu (where we dynamically link to avoid rerunning ThinLTO in each stage).
Follow-up to #76708.
Fixes #76698.