Link with ld.gold by default #29974
Link with ld.gold by default #29974
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -199,6 +200,24 @@ impl<'a> Linker for GnuLinker<'a> { | |||
| fn export_symbols(&mut self, _: &Session, _: &CrateTranslation, _: &Path) { | |||
| // noop, visibility in object files takes care of this | |||
| } | |||
|
|
|||
| fn try_gold_linker(&mut self) { | |||
| let gold_exists = Path::new("/usr/bin/ld.gold").exists(); | |||
There was a problem hiding this comment.
You should check for it in all pathes listed in $PATH, not hard-code /usr/bin.
There was a problem hiding this comment.
This is very important on, e.g. NixOS, where /usr/bin only contains env.
EDIT: Seems to have been brought up by @nagisa already.
bluss
added
the
relnotes
|
r=me with probing all of |
|
I'm not sure I agree with probing Edit: Ah, I'll just do it |
|
In MinGW |
|
I'll conditionalize it on linux too then. |
|
I figured that if you go out of your way to install |
Some distribution/OSes/package managers install to /usr/local/bin by default (and do not link /usr/bin/), then there’s NixOS based things which install to random-looking pathes and may link things into arbitrary pathes. |
| @@ -515,6 +515,8 @@ options! {CodegenOptions, CodegenSetter, basic_codegen_options, | |||
| "explicitly enable the cfg(debug_assertions) directive"), | |||
| inline_threshold: Option<usize> = (None, parse_opt_uint, | |||
| "set the inlining threshold for"), | |||
| disable_gold: bool = (false, parse_bool, | |||
There was a problem hiding this comment.
This is… a pretty confusing option (both its description and behaviour).
We already have -C linker which possibly swaps out gcc for something else and now we pass a special flag into linker assuming it is gcc-compatible (in absence of this option) without checking if specified linker is indeed compatible. That might make -C linker harder to use.
Maybe it would be better to have -C linker=gcc which overrides any default/dubious flags we might add?
There was a problem hiding this comment.
We already have -C linker which possibly swaps out gcc for something else and now we pass a special flag into linker assuming it is gcc-compatible (in absence of this option) without checking if specified linker is indeed compatible. That might make -C linker harder to use
As far as I can tell that is an existing problem. Even if you override the linker, rustc assumes it takes either gcc or link.exe style arguments, depending on platform. IOW I tihnk -C linker is only good for specifying a different path to the expected linker, not for using a completely different 'style' of linker.
There was a problem hiding this comment.
I'm not super worried about random codegen options being deprecated at some point, an option like this is pretty flavorful and likely to only be a one-time use case (if ever), so if we in the future just hide this and make it ignored by default then it's probably not too bad.
It may be the case that gcc -fuse-ld=gold -fuse-ld=not-gold (or some similar incantation) also works, so we could just be sure to allow that pattern, but I don't know if it exists. Either way this seems fine to me for now.
|
Other than my nits, I really want this to land. |
|
🐤 👍 |
|
I've updated it to search |
|
|
||
| let gold_exists = match env::var("PATH") { | ||
| Ok(env_path) => { | ||
| env_path.split(":").any(|p| { |
I wouldn't be too worried about this, personally. That seems super unlikely to ever happen (if it doesn't support windows) and even if it does exist it'll exist as |
|
A very relevant DragonflyBSD commit |
|
Gold only works reliably on x86 / x86_64 so this should probably also check that the target is x86 to avoid asking for gold on cross-compiles. Edit: maybe arm? Actually, checking that gold exists anywhere then asking for it for a cross-compile is probably bad. Maybe the check should be that the target == host and is a supported arch. |
|
@bstrie suggests this requires an RFC. cc @rust-lang/compiler |
brson
added
T-compiler
|
@brson my ld.gold --help prints this: Are you claiming most of these targets do not work? |
|
@nagisa No, I'm claiming I have no evidence they do. I know that I read just today that gold's powerpc support is incomplete. Edit: I see my previous statement was strongly saying gold only works on x86. That was a mistake. |
|
I made this patch more defensive. Now it only uses gold when ld.gold exists, the host is linux, the host and the target are roughly the same, and the target is x86/x86_64. As @nagisa points out there are other platforms that gold support, but I have most confidence this won't break anything on x86 linux. |
|
Hello, being bound to Debian Wheezy, I'm now forced to use Unfortunately, I'm stuck with cargo based projects containing a custom |
|
@xitep You can write a wrapper shell script for |
|
@eddyb thank you so much for the idea! it works nicely. |
|
This might have killed play.rust-lang.org. see e.g. https://play.rust-lang.org/?gist=5dc29f8c79c267056ca2&version=nightly cc @alexcrichton @brson |
|
Another bit of incompatibility: gold does not have the same default library search path; in particular, it does not search in |
|
@birkenfeld That sounds like a distro bug rather than rustc's fault, unless it's intentional, in which case... what's the reasoning? |
|
You mean, the library being in |
|
@birkenfeld I was referring to "gold does not have the same default library search path". |
|
Ah! That, as far I could find out from quick Googling, is "normal" behavior for gold. Apparently ld.bfd includes the runtime linker search path (i.e. Possibly gold's behavior is cleaner here, I don't know. But it's definitely annoying to trip over these kinds of problems without a good way to disable gold from Cargo. |
|
@birkenfeld if gold repsects |
|
Yes, I can (and I will). Just noting this as a data point to consider before stabilizing this change (one which likely doesn't show up in a crater run). |
|
|
|
This broke playpen on nightly. |
This seems really bad. So when you use the Rust downloaded from official sources, it doesn't work on Debian? I think rustc needs to be able to figure out this scenario. |
|
@xitep Do you know how we could identify configurations like yours and automatically disable gold? |
|
@xitep @birkenfeld I filed issues for both the regression you identified. |
|
Thanks! |
|
@brson many thanks! so far i haven't searched intensively, but it looks like something along the lines of you can see the full output on my debian wheezy (stable) of the |
|
I think I should mention, @brson, that |
|
Thanks for the interesting note @lhecker. |
|
@xitep Thanks for the investigation on detecting -fuse-ld. I'm noting it in the bug. |
brson
removed
the
relnotes
This reverts commit 34dc0e0. cc #30783 #30784 #29974 r? @alexcrichton
When using
ccfor linking rustc will, if gold is available (by looking for/usr/bin/ld.gold), pass-fuse-ld=goldtocc.In some scenarios gold links much faster than ld. Servo uses it to considerably speed up linking. gold behaves nearly identically to ld (though I think there are rare corner cases that don't work still). I've run this through crater and everything there continues to link.
To disable, pass
-C disable-gold.