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

Enable link-shared for LLVM on non-Windows targets #76708

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Sep 14, 2020

Even when LLVM is not generally participating in a shared link with rustc, we
will likely still link to the shared dylib from rust-lld, so we still need to
promote it.

Hopefully fixes #76698.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors try

This has a second commit which enables macOS and Windows try builders so that we can hopefully make sure that things work without waiting for a full local build.

@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Trying commit 1ffb3c170e223980509ec8c8a7ec734e8c4ddf10 with merge 2d7e7d3397bb4d7fda21c806662de7a714cdc4c1...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-msvc of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Could not determine the LLVM submodule commit hash. Assuming that an LLVM rebuild is not necessary.
To force LLVM to rebuild, remove the file `D:\a\rust\rust\build\x86_64-pc-windows-msvc\llvm\llvm-finished-building`
llvm-config: error: component libraries and shared library

llvm-config: error: missing: D:\a\rust\rust\build\x86_64-pc-windows-msvc\llvm\build\lib\gtest.lib
llvm-config: error: missing: D:\a\rust\rust\build\x86_64-pc-windows-msvc\llvm\build\lib\gtest_main.lib
llvm-config: error: missing: D:\a\rust\rust\build\x86_64-pc-windows-msvc\llvm\build\lib\LLVMTestingSupport.lib
thread 'main' panicked at 'command did not execute successfully: "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\llvm\\build\\bin\\llvm-config.exe" "--libfiles"
expected success, got: exit code: 1', src\build_helper\lib.rs:139:9
failed to run: D:\a\rust\rust\build\bootstrap\debug\bootstrap dist
Build completed unsuccessfully in 0:16:12
== clock drift check ==
  local time: Mon Sep 14 13:59:28 CUT 2020
  local time: Mon Sep 14 13:59:28 CUT 2020
  network time: Mon, 14 Sep 2020 13:59:28 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (5908) (python)
Terminate orphan process: pid (7528) (sccache)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Sep 14, 2020

💔 Test failed - checks-actions

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Trying commit a6394d2b8167631e8bff8946da6c2b64bdb34383 with merge 8a552637288ac264194163f7294832fbd89f7836...

@alexcrichton
Copy link
Member

Hm I may be a bit confused. IIRC there is weirdness of how the dynamic library is named on macos compared to other platforms, but I think all both Linux and mac are dynamically linked? I thought it was just windows that was static

@Mark-Simulacrum
Copy link
Member Author

We don't set link-shared on macOS, and on linux it's only implicitly set by enabling ThinLTO with LLVM.

It seems like on macOS though the compiler itself isn't linked dynamically to LLVM, as otherwise current nightly wouldn't work, as it lacks any libLLVM.dylib. It seems like the libLLVM.dylib on macOS today is just for rust-lld and other LLVM tools.

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8a552637288ac264194163f7294832fbd89f7836 (8a552637288ac264194163f7294832fbd89f7836)

@Mark-Simulacrum
Copy link
Member Author

I have confirmed that on latest nightly RUSTFLAGS=-Clinker=rust-lld cargo run fails on macOS with the "libLLVM.dylib not found" error but does get farther (failing on missing system libraries, but that's because I'm invoking it as rust-lld instead of through a driver like clang, I think) on the toolchain produced by this PR.

@alexcrichton
Copy link
Member

Hm ok, I don't really understand what's going on here then. That feels like a bug that macOS is not linked dynamically (but only rust-lld is?), we ideally want all the platforms to be consistent. IIRC the reason Windows doesn't do this is that LLVM's build system doesn't really support generating a shard library (I don't know why though).

Assuming though that this bug is set aside and we try to just fix nightlies, the previous conditional check I believe is correct. The comment makes sense and it I think is what we want, I believe that the bug should probably be fixed elsewehere in the configuration step of rustbuild to ensure that rustbuild doesn't erroneously think that on Windows and macOS LLVM is dynamically linked.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 14, 2020
@Mark-Simulacrum
Copy link
Member Author

I don't personally have a Windows machine to test on -- I'd be interested in finding out if/how the llvm tools there work (or not) on current nightly. Judging by their size, they definitely don't include a full LLVM, but we don't ship a libLLVM dll anywhere that I can see. Presumably they work though? Maybe not?

That issue aside (since we should probably fix nightlies before tackling Windows, unless there's a solution covering both)...

It looks like rust-lld (and presumably all the other LLVM tools) are just broken on every target right now except x86_64-unknown-linux-gnu (and Windows, if they ever worked), because that's the only one that enables a shared LLVM in CI I think (and even then, indirectly through enabling LLVM ThinLTO).

We could just try to enabling link-shared on all platforms except for Windows in CI, which presumably would fix this? I'm not sure if that's precisely the right fix. I've pushed up a commit that does so (see the commit message for some explanation as well as this comment).

Interested to hear what you think!

@Mark-Simulacrum
Copy link
Member Author

@bors try

Thought I'd kick off a try build so we can at least see if this is just completely not going to work...

@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Trying commit 3384b515966f9186cc75952d23305ccf42dd5cc1 with merge 5faa03252d364b6903d28d52397c0a22a5c91f9b...

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 5faa03252d364b6903d28d52397c0a22a5c91f9b (5faa03252d364b6903d28d52397c0a22a5c91f9b)

@phil-opp
Copy link
Contributor

I don't have a Windows machine either, but for my blog_os project I'm running scheduled CI builds on Linux, Windows, and macOS for each nighly. The build failure seems to happen only on macOS, on Windows is appears to work without problems. Apart from rust-lld, the build process includes llvm-size, llvm-objcopy, and llvm-ar.

@alexcrichton
Copy link
Member

I don't personally have a Windows machine to test on -- I'd be interested in finding out if/how the llvm tools there work (or not) on current nightly

FWIW they're statically linked with LLVM, and they work for me at least locally.

We could just try to enabling link-shared on all platforms except for Windows in CI, which presumably would fix this?

I believe this is corret, yeah, and I think ideally we'd just generate an error in rustbuild trying to set link-shared on Windows since I don't think it works at all.

In any case r=me with the tweak to CI config since I think that's what we want to do on macOS anyway

Windows doesn't quite support dynamic linking to LLVM yet, but on other
platforms we do. In rust-lang#76708, it was discovered that we dynamically link to LLVM
from the LLVM tools (e.g., rust-lld), so we need the shared LLVM library to link
against. That means that if we do not have a shared link to LLVM, and want LLVM
tools to work, we'd be shipping two copies of LLVM on all of these platforms:
one in librustc_driver and one in libLLVM.

Also introduce an error into rustbuild if we do end up configured for shared
linking on Windows.
@Mark-Simulacrum
Copy link
Member Author

FWIW they're statically linked with LLVM, and they work for me at least locally.

Hm, interesting. I guess they are quite a bit bigger than on linux targets, so that makes sense. I was sort of expecting ~80 MB each but I guess they're not linking to a good part of LLVM so that's why they're much smaller.

Dropped the try build commits and added a panic! on windows and link-shared configured, so that should be ready to go.

@bors r=alexcrichton rollup=never

This is somewhat likely to bounce I suspect once or twice as we work out any bugs, and in any case is nice to be able to bisect to in case of regressions.

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit f001a0c 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2020
@alexcrichton
Copy link
Member

Ah yeah the tools that bring in codegen libraries are bigger (e.g. llc, opt, lld, etc), but the binary inspection tools benefit from gc in linkers and are pretty small (they don't benefit much from dynamic linking)

@mati865
Copy link
Contributor

mati865 commented Sep 15, 2020

I believe this is corret, yeah, and I think ideally we'd just generate an error in rustbuild trying to set link-shared on Windows since I don't think it works at all.

FWIW LLVM does support it in MinGW mode (and it works fine) but we cannot use it since linking to that shared libLLVM.dll would result in hangs because of old mingw-w64 version used on the CI (winpthreads bug).

@bors
Copy link
Contributor

bors commented Sep 15, 2020

⌛ Testing commit f001a0c with merge 27a45d0...

@Mark-Simulacrum Mark-Simulacrum changed the title Always try to promote shared LLVM to the sysroot Enable link-shared for LLVM on non-Windows targets Sep 15, 2020
@bors
Copy link
Contributor

bors commented Sep 15, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: alexcrichton
Pushing 27a45d0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2020
@bors bors merged commit 27a45d0 into rust-lang:master Sep 15, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 15, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the lld-macos branch September 16, 2020 21:54
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2020
…crichton

Don't dynamically link LLVM tools unless rustc is too

This PR initially tried to support link-shared on all of our target platforms (other than Windows), but ran into a number of difficulties:
 * LLVM doesn't really support a shared link on macOS (llvm-config runs into problems with the version suffix)
 * LLVM doesn't seem to support a shared link when cross-compiling (the libLLVM.so ends up empty and symbols are not found)

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 rust-lang#76708.
Fixes rust-lang#76698.
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-lld broken on latest x86_64-apple-darwin nightly
9 participants