-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
Support LLD linker for Darwin #286
Conversation
I gave this a quick spin, it might need more work for darwin_arm? (With llvm 17.0.6)
|
It might. I did develop this on an M2 Mini, but I tested it only on the simple tests in this repo, without a custom sysroot. The params look like this for me:
|
I found my mistake. At least the repo tests should be passing now. |
I will wait for at least some people to say this change is working for them. This may not make it into the next release. |
@siddharthab I suspect this might be a llvm/lld version difference. Looks like the tests run on 16.x |
On my local machine, I checked with LLVM 17 as well. |
Anecdata, working fine on Bazel 7.1 on an M2 mac with up-to-date rules_rust, rules_go, llvm. |
I think that for @DavidZbarsky-at , it is either the I am thinking of landing this behind a feature flag so people can more easily try it within their projects. Most likely, it might just mean adding a few more flags to the invocation which can be iterated on as more people try. |
Made it optional, and added to the PR description instructions on how to use for a project. |
* 2024.03.12: bazel-contrib/toolchains_llvm#286: Support LLD linker for Darwin
* 2024.03.12: bazel-contrib/toolchains_llvm#286: Support LLD linker for Darwin
Clang [has custom code](https://github.com/llvm/llvm-project/blob/1193f7d6487d2d94009f8d8d27da3907136482b9/clang/lib/Driver/ToolChains/Darwin.cpp#L358-L360) to inject the `-platform_version` flag when invoking `lld`. [This takes effect ](https://github.com/llvm/llvm-project/blob/1193f7d6487d2d94009f8d8d27da3907136482b9/clang/lib/Driver/ToolChain.cpp#L919-L920)when we pass `-fuse-ld=lld`, which we currently don't do for `lld` because want to point at the hermetic one. Switch to using `--ld-path` instead, which is supported since Clang12. The current structure relies on `sanitize_option` generating 1 option per input, so I had to add the extra option check in the loop around it. This is why LLD didn't work for me [when we first added this support](#286 (comment)). Kudos to @keith for the analysis above :) I think we should follow-up soon and make LLD the default; that should allow a lot of simplification
Users can use with
--linkopt=-fuse-ld=ld64.lld
flag.Eventually, we should make this the default. But only after we hear from some users that it works for their projects. This PR will make it easy for them to test.