-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Always use LLD on darwin #380
base: master
Are you sure you want to change the base?
Conversation
5068712
to
780b391
Compare
@rrbutani are you the right person to take a look at this? I tried to keep the cmd lines generated the same as we did before this change (when running with |
@@ -228,7 +210,19 @@ def cc_toolchain_config( | |||
"-Bdynamic", | |||
"-L{}lib".format(toolchain_path_prefix), | |||
]) | |||
|
|||
else: | |||
# For single-platform builds, we can statically link the bundled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is preexisting but I don't think it's correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it actually might be right because of this bit here:
toolchains_llvm/toolchain/cc_toolchain_config.bzl
Lines 188 to 189 in 6edc43b
if stdlib == "builtin-libc++" and is_xcompile: | |
stdlib = "stdc++" |
i.e. builtin-libc++
implies not is_xcompile
, and this arm has exec_os != "darwin"
implying linux (exec) -> linux (target)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely no objection to rewording that comment though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay — unfortunately I haven't really kept up with the state of lld
on darwin and no longer have a machine to test this on.
However, everything in this PR looks good to me. Appreciate the cleanup as well.
@@ -228,7 +210,19 @@ def cc_toolchain_config( | |||
"-Bdynamic", | |||
"-L{}lib".format(toolchain_path_prefix), | |||
]) | |||
|
|||
else: | |||
# For single-platform builds, we can statically link the bundled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it actually might be right because of this bit here:
toolchains_llvm/toolchain/cc_toolchain_config.bzl
Lines 188 to 189 in 6edc43b
if stdlib == "builtin-libc++" and is_xcompile: | |
stdlib = "stdc++" |
i.e. builtin-libc++
implies not is_xcompile
, and this arm has exec_os != "darwin"
implying linux (exec) -> linux (target)
@@ -228,7 +210,19 @@ def cc_toolchain_config( | |||
"-Bdynamic", | |||
"-L{}lib".format(toolchain_path_prefix), | |||
]) | |||
|
|||
else: | |||
# For single-platform builds, we can statically link the bundled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely no objection to rewording that comment though
780b391
to
1a49d04
Compare
It sounds like a few folks have tried the
-fuse-ld=lld
option and it works for them, so let's try to roll it out more broadly.