[WIP] Infer linker-flavor from the selected linker #50359
Conversation
rust-highfive
added
the
S-waiting-on-review
| }) | ||
| } | ||
|
|
||
| pub fn linker_flavor(&self) -> LinkerFlavor { |
There was a problem hiding this comment.
This is the main change but the logic probably doesn't work correctly on Windows because of the .exe suffix.
|
The job Click to expand the log.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 |
|
Can we have a method for explicitly specifying the linker flavour? |
|
@nagisa In what context? ( |
|
Oh, well then. |
There was a problem hiding this comment.
Thanks @japaric! For actually invoking the linker we need to know the flavor/path to the linker, and I thinkt he high level idea of how I'm thinking we'll do this is (in order):
- If both are explicitly specified, use them
- If only flavor is explicitly specified, then infer the default path based on the flavor
- If only the path is specified, infer the flavor from the
file_stem - The same logic then applies to the target spec options, recursively
- If nothing is specified (either explicitly or implicitly through the target spec) then we emit an error
| @@ -600,11 +600,51 @@ impl Session { | |||
| .panic | |||
| .unwrap_or(self.target.target.options.panic_strategy) | |||
| } | |||
| pub fn linker_flavor(&self) -> LinkerFlavor { | |||
|
|
|||
| fn linker(&self) -> &str { | |||
There was a problem hiding this comment.
AFAIK there's not actually any reason these methods have to live in librustc, if you'd like feel free to delete them here and move the wholesale into librustc_trans
There was a problem hiding this comment.
I also think this method looks mostly correct except for the last part. We'll probably want to rename this to something like linker_file_stem and then make sure you use the file_stem method of Path at the end to remove bat/exe suffixes
| @@ -42,7 +42,6 @@ pub fn target() -> Result<Target, String> { | |||
| target_vendor: "unknown".to_string(), | |||
| data_layout: "e-p:32:32-i64:64-v128:32:128-n32-S128".to_string(), | |||
| arch: "asmjs".to_string(), | |||
| linker_flavor: LinkerFlavor::Em, | |||
There was a problem hiding this comment.
I think this ay break emscripten, right? I think there's no other way to say that "the emscripten target uses emcc" by default, right?
| @@ -65,7 +65,6 @@ pub fn target() -> Result<Target, String> { | |||
| target_vendor: "unknown".to_string(), | |||
| data_layout: "e-m:e-p:32:32-i64:64-n32:64-S128".to_string(), | |||
| arch: "wasm32".to_string(), | |||
| linker_flavor: LinkerFlavor::Lld(LldFlavor::Wasm), | |||
There was a problem hiding this comment.
Similar to emscripten above I think this means that wasm will attempt to use cc by default if we don't otherwise set some form of a default here
There was a problem hiding this comment.
Or maybe it's best to sum this up as "we probably want to continue have default linker flavors", I think?
| LinkerFlavor::Ld | ||
| } else if linker == "emcc" { | ||
| LinkerFlavor::Em | ||
| } else if linker == "link.exe" { |
There was a problem hiding this comment.
lld-link.exe is the LLD equivalent here. Might also be worth handling both with and without the .exe suffix?
shepmaster
added
S-waiting-on-author
|
Ping from triage, @japaric ! You have some review feedback to address! |
|
☔ The latest upstream changes (presumably #50105) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thanks for the input, @alexcrichton. I don't have time to incorporate your feedback right now so I'm going to close this in the meantime to clear the PR queue. |
try to infer linker flavor from linker name and vice versa This is a second take on PR #50359 that implements the logic proposed in #50359 (review) With this change it would become possible to link `thumb*` binaries using GNU's LD on stable as `-C linker=arm-none-eabi-ld` would be enough to change both the linker and the linker flavor from their default values of `arm-none-eabi-gcc` and `gcc`. To link `thumb*` binaries using rustc's LLD on stable `-Z linker-flavor` would need to be stabilized as `-C linker=rust-lld -Z linker-flavor=ld.lld` are both required to change the linker and the linker flavor, but this PR doesn't propose that. We would probably need some sort of stability guarantee around `rust-lld`'s name and availability to make linking with rustc's LLD truly stable. With this change it would also be possible to link `thumb*` binaries using a system installed LLD on stable using the `-C linker=ld.lld` flag (provided that `ld.lld` is a symlink to the system installed LLD). r? @alexcrichton
And make
linker-flavoroptional in target specifications.This PR reduces the need for
-Z linker-flavorby having rustc infer the linker flavor from the name of the linker.This has been tested on Linux and by cross compiling for ARM Cortex-M with 3 different linkers (gcc, rustc's lld and ld.lld), but the logic probably needs quite a bit of tweaking to not break existing use cases so I would suggest not landing this before the 1.27 beta cutoff to maximize test time.
r? @alexcrichton