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

Regression when using linker=arm-none-eabi-gcc #113597

Closed
adamgreig opened this issue Jul 12, 2023 · 14 comments · Fixed by #113631
Closed

Regression when using linker=arm-none-eabi-gcc #113597

adamgreig opened this issue Jul 12, 2023 · 14 comments · Fixed by #113631
Assignees
Labels
A-cli Area: Command line interface to the compiler. A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Milestone

Comments

@adamgreig
Copy link
Member

The cortex-m CI tests building with all our supported linkers: Rust's provided lld, ARM GCC's arm-none-eabi-ld, and ARM GCC's arm-none-eabi-gcc. It recently encountered an error when using arm-none-eabi-gcc in nightly builds since #112910.

The most likely culprit is that the failing version has started added "-fuse-ld=lld" to the linker arguments, which was not present in previous working versions. @lqd, I wonder if we're meant to be adding some other argument to disable adding -fuse-ld=lld?

Code

The error occurs with the cortex-m-rt CI and can be reproduced like so:

git clone https://github.com/rust-embedded/cortex-m
cd cortex-m/cortex-m-rt
rustup toolchain install nightly-2023-07-03
rustup target add thumbv6m-none-eabi --toolchain nightly-2023-07-03
cargo +nightly-2023-07-03 rustc --target thumbv6m-none-eabi --example alignment --features cortex-m/critical-section-single-core -- -C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles

Or viewed in CI here.

The error is:

error: linking with `arm-none-eabi-gcc` failed: exit status: 1
  |
  = note: LC_ALL="C" PATH="<snip>" VSLANG="1033" "arm-none-eabi-gcc" "/tmp/rustcFHUl0t/symbols.o" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/examples/alignment-c363c9558a2fe06e.15yn3mrxz53m7uw7.rcgu.o" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/examples/alignment-c363c9558a2fe06e.6wqau5vl49hu4fi.rcgu.o" "-Wl,--as-needed" "-L" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/deps" "-L" "/home/adam/Projects/re/cortex-m/target/debug/deps" "-L" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/build/cortex-m-rt-69b7ab10efe30aa1/out" "-L" "/home/adam/.rustup/toolchains/nightly-2023-07-03-x86_64-unknown-linux-gnu/lib/rustlib/thumbv6m-none-eabi/lib" "-Wl,-Bstatic" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/deps/libpanic_halt-f60a67ec71296eeb.rlib" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/deps/libcortex_m_rt-9a1a89eba3425cf7.rlib" "/home/adam/.rustup/toolchains/nightly-2023-07-03-x86_64-unknown-linux-gnu/lib/rustlib/thumbv6m-none-eabi/lib/librustc_std_workspace_core-cc16e9be0f247487.rlib" "/home/adam/.rustup/toolchains/nightly-2023-07-03-x86_64-unknown-linux-gnu/lib/rustlib/thumbv6m-none-eabi/lib/libcore-b9a65150d0351313.rlib" "/home/adam/.rustup/toolchains/nightly-2023-07-03-x86_64-unknown-linux-gnu/lib/rustlib/thumbv6m-none-eabi/lib/libcompiler_builtins-2769d4646d3d396e.rlib" "-Wl,-Bdynamic" "-fuse-ld=lld" "-Wl,--eh-frame-hdr" "-Wl,-z,noexecstack" "-L" "/home/adam/.rustup/toolchains/nightly-2023-07-03-x86_64-unknown-linux-gnu/lib/rustlib/thumbv6m-none-eabi/lib" "-o" "/home/adam/Projects/re/cortex-m/target/thumbv6m-none-eabi/debug/examples/alignment-c363c9558a2fe06e" "-Wl,--gc-sections" "-no-pie" "-nodefaultlibs" "-nostartfiles" "-Tlink.x"
  = note: collect2: fatal error: cannot find 'ld'
          compilation terminated.

Version it worked on

nightly-2023-07-02

Version with regression

nightly-2023-07-03

bisected to 8e2d5e3

from PR #112910

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@adamgreig adamgreig added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jul 12, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Jul 12, 2023
@jyn514
Copy link
Member

jyn514 commented Jul 12, 2023

linker: Some("rust-lld".into()),

shows that the default for the target is rust-lld. i wonder if the new code uses the default instead of the linker in -C linker for some reason?

@jyn514 jyn514 added A-linkage Area: linking into static, shared libraries and binaries A-cli Area: Command line interface to the compiler. labels Jul 12, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 12, 2023
@lqd
Copy link
Member

lqd commented Jul 12, 2023

I wonder if we're meant to be adding some other argument to disable adding -fuse-ld=lld?

You were not meant to do anything, sorry about that. I'll fix it.

@lqd lqd self-assigned this Jul 12, 2023
@lqd
Copy link
Member

lqd commented Jul 12, 2023

I can't build a thumbv6m-none-eabi toolchain locally, via ./x build library/ --target thumbv6m-none-eabi, which would be complete enough for this cortex-m test to build. There are issues about missing core/std.

I tried on a no_std example, and can also reproduce the arm-none-eabi-gcc issue and investigate more there, and that rust-lld and arm-none-eabi-ld work.

The code I added in #112910 is asked to add CLI arguments for a Gnu(Cc::Yes, Lld::Yes) flavor: this asks to link using lld via a c/c++ compiler, and why -fuse-ld=lld is added. I assume that flavor is incorrect when using -C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles @adamgreig ? (I don't know who the target maintainers are, so I'll also ping the other 2 people involved in the discovery: @hannobraun @thejpster)

-C linker=arm-none-eabi-ld and -C linker=rust-lld still work because they result in a non-cc request, and we don't do anything there as expected.

For my own understanding: does arm-none-eabi-gcc use arm-none-eabi-ld by default maybe ? And if so, I'd expect that explicitly specifying the linker would also fix the issue, à la -C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles -C link-arg=-fuse-ld=bfd which seems to work for me ?

I'll look into why this surprising flavor is used in this situation, as it seems to be the pre-existing issue here uncovered by the PR, and report back. It could be an issue in the flavor inference, or some information missing in the targets' definitions, and so on.

@workingjubilee workingjubilee added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 12, 2023
@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 12, 2023

Technically, the target builds.
As a tier 2 target, we would strongly prefer it to not require additional antics to build.

There was some discussion as to whether this should be P-high but I have taken lqd's remarks that this may be just exposed preexisting problems as a reason to set it slightly lower.

@lqd
Copy link
Member

lqd commented Jul 12, 2023

The linker flavor used is a mix between what can be inferred from the CLI (-C linker) and the target's default linker flavor:

  • there is no linker flavor on the CLI (and that also offers another workaround on nightly: -C linker-flavor=gnu-cc -Zunstable-options), so it will have to be inferred from here to here.
  • in infer_linker_hints -C linker=arm-none-eabi-gcc infers a Some(Cc::Yes) cc hint, and no hint about lld.
  • the target's linker_flavor is combined in with_cli_hints with these hints. We have our Cc::Yes, but there is no hint about lld, so the target's flavor lld component is used. It's Gnu(Cc::No, Lld::Yes).
  • so we now have our Gnu(Cc::Yes, Lld::Yes) flavor

@adamgreig @hannobraun @thejpster: is the linker people are expected to use on thumbv6m-none-eabi supposed to be some lld ? Is the default linker and flavor in thumb_base.rs correct ?

In the meantime, I'll see to change this so that the lld flags are only added when the unstable MCP510 options are used, to keep this stable and unfortunate behavior unchanged when the CLI and target conflict instead of compose.

@adamgreig
Copy link
Member Author

Thanks for looking into this quickly!

Most people building for thumbv6m-none-eabi will use the default rust-lld linker today. It works fine for (I believe) all Rust-only projects. However, it's not uncommon to use arm-none-eabi-ld, maybe to use specific GCC linker script directives, or just when hunting down linker-related bugs. For both of those use cases there's no problem with nightly, and I think the default in thumb_base is therefore fine.

Some people do need to use arm-none-eabi-gcc when linking with pre-compiled C libraries - I understand that using gcc pulls in a target specific libc, and a lot of pre-compiled C libraries will be trying to link against symbols it exports which Rust doesn't. I believe arm-none-eabi-gcc does delegate to arm-none-eabi-ld.

Passing either -C linker-flavor=gnu-cc -Zunstable-options or -C link-arg=-fuse-ld=bfd does work to get a build without errors, though I don't have a test case handy to check it's actually getting what people need out of arm-none-eabi-gcc. I'll try and dig around a bit more for the specific use cases where it's required over arm-none-eabi-ld.

@thejpster
Copy link

My main recollection is that gcc requires you to put -Wl, in front of your linker arguments and using ld directly you don't 🤣

But I think -nodefaultlibs and friends are gcc options not ld options so maybe ld won't bring in libc and the C startup automatically. I'll test it out shortly.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 13, 2023
make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

Fixes rust-lang#113597, which contains more details on how this happens through the code, and showcases an unexpected `Gnu(Cc::Yes, Lld::Yes)` flavor.

rust-lang#112910 added support to use `lld` when the flavor requests it, but didn't explicitly do so only when using `-Clink-self-contained=+linker` or one of the unstable `-Clinker-flavor`s.

The problem: some targets have a `lld` linker and flavor, e.g. `thumbv6m-none-eabi` from that issue. Users can override the linker but there are no linker flavors precise enough to describe the linker opting out of lld: when using `-Clinker=arm-none-eabi-gcc`, we infer this is a `Cc::Yes` linker flavor, but the `lld` component is unknown and therefore defaulted to the target's linker flavor, `Lld::Yes`.

<details>
<summary>Walkthrough of how this happens</summary>

The linker flavor used is a mix between what can be inferred from the CLI (`-C linker`) and the target's default linker flavor:

- there is no linker flavor on the CLI (and that also offers another workaround on nightly: `-C linker-flavor=gnu-cc -Zunstable-options`), so it will have to be inferred [from here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1334-L1336) to [here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1321-L1327).
- in [`infer_linker_hints`](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L320-L352) `-C linker=arm-none-eabi-gcc` infers a `Some(Cc::Yes)` cc hint, and no hint about lld.
- the target's `linker_flavor` is combined in `with_cli_hints` with these hints. We have our `Cc::Yes`, but there is no hint about lld, [so the target's flavor `lld` component is used](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L356-L358). It's [`Gnu(Cc::No, Lld::Yes)`](https://github.com/rust-lang/rust/blob/993deaa0bf8bab9dd3eadfd1fbeb093328e95afe/compiler/rustc_target/src/spec/thumb_base.rs#L35).
- so we now have our `Gnu(Cc::Yes, Lld::Yes)` flavor

</details>

This results in a `Gnu(Cc::Yes, Lld::Yes)` flavor on a non-lld linker, causing an additional unexpected `-fuse-ld=lld` argument to be passed.

I don't know if this target defaulting to `rust-lld` is expected, but until MCP510's new linker flavor are stable, when people will be able to describe their linker/flavor accurately, this PR keeps the stable behavior of not doing anything when the linker/flavor on the CLI unexpectedly conflict with the target's.

I've tested this on a `no_std` `-C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles --target thumbv6m-none-eabi` example, trying to simulate one of `cortex-m`'s test mentioned in issue rust-lang#113597 (I don't know how to build a local complete  `thumbv6m-none-eabi` toolchain to run the exact test), and checked that `-fuse-lld` was indeed gone and the error disappeared.

r? `@petrochenkov`
@thejpster
Copy link

main.c:

int main() {
    puts("Hello, world");
    return 0;
}

Compile object code:

$ arm-none-eabi-gcc -o main.o -c main.c -mcpu=cortex-m3

Link with GCC:

$ arm-none-eabi-gcc -o main main.o -mcpu=cortex-m3
<bunch of errors about missing _close_r, _lseek_r and other syscalls that the newlib C library requires>

Link with ld:

$ arm-none-eabi-ld -o main main.o -mcpu=cortex-m3
warning: cannot find entry symbol _start
main.o: in function `main` main.c: undefined reference to `puts`

This does imply that for trivial invocations arm-none-eabi-gcc will include both the default start code (which provides _start) and newlib (the C library), whilst arm-none-eabi-ld will not.

@thejpster
Copy link

thejpster commented Jul 13, 2023

See also #113535, where I set the default target for bare-metal SPARC to be gcc, precisely because the default libraries it links in are so useful - they include not only the start-up code (no idea how that works, didn't need to find out) but also a C library for talking to the stdout of a popular SPARC CPU emulator.

So, yeah, GCC is a really useful linker driver in some scenarios and we don't want to stop that from working.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 13, 2023
make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

Fixes rust-lang#113597, which contains more details on how this happens through the code, and showcases an unexpected `Gnu(Cc::Yes, Lld::Yes)` flavor.

rust-lang#112910 added support to use `lld` when the flavor requests it, but didn't explicitly do so only when using `-Clink-self-contained=+linker` or one of the unstable `-Clinker-flavor`s.

The problem: some targets have a `lld` linker and flavor, e.g. `thumbv6m-none-eabi` from that issue. Users can override the linker but there are no linker flavors precise enough to describe the linker opting out of lld: when using `-Clinker=arm-none-eabi-gcc`, we infer this is a `Cc::Yes` linker flavor, but the `lld` component is unknown and therefore defaulted to the target's linker flavor, `Lld::Yes`.

<details>
<summary>Walkthrough of how this happens</summary>

The linker flavor used is a mix between what can be inferred from the CLI (`-C linker`) and the target's default linker flavor:

- there is no linker flavor on the CLI (and that also offers another workaround on nightly: `-C linker-flavor=gnu-cc -Zunstable-options`), so it will have to be inferred [from here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1334-L1336) to [here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1321-L1327).
- in [`infer_linker_hints`](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L320-L352) `-C linker=arm-none-eabi-gcc` infers a `Some(Cc::Yes)` cc hint, and no hint about lld.
- the target's `linker_flavor` is combined in `with_cli_hints` with these hints. We have our `Cc::Yes`, but there is no hint about lld, [so the target's flavor `lld` component is used](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L356-L358). It's [`Gnu(Cc::No, Lld::Yes)`](https://github.com/rust-lang/rust/blob/993deaa0bf8bab9dd3eadfd1fbeb093328e95afe/compiler/rustc_target/src/spec/thumb_base.rs#L35).
- so we now have our `Gnu(Cc::Yes, Lld::Yes)` flavor

</details>

This results in a `Gnu(Cc::Yes, Lld::Yes)` flavor on a non-lld linker, causing an additional unexpected `-fuse-ld=lld` argument to be passed.

I don't know if this target defaulting to `rust-lld` is expected, but until MCP510's new linker flavor are stable, when people will be able to describe their linker/flavor accurately, this PR keeps the stable behavior of not doing anything when the linker/flavor on the CLI unexpectedly conflict with the target's.

I've tested this on a `no_std` `-C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles --target thumbv6m-none-eabi` example, trying to simulate one of `cortex-m`'s test mentioned in issue rust-lang#113597 (I don't know how to build a local complete  `thumbv6m-none-eabi` toolchain to run the exact test), and checked that `-fuse-lld` was indeed gone and the error disappeared.

r? ``@petrochenkov``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 13, 2023
make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

Fixes rust-lang#113597, which contains more details on how this happens through the code, and showcases an unexpected `Gnu(Cc::Yes, Lld::Yes)` flavor.

rust-lang#112910 added support to use `lld` when the flavor requests it, but didn't explicitly do so only when using `-Clink-self-contained=+linker` or one of the unstable `-Clinker-flavor`s.

The problem: some targets have a `lld` linker and flavor, e.g. `thumbv6m-none-eabi` from that issue. Users can override the linker but there are no linker flavors precise enough to describe the linker opting out of lld: when using `-Clinker=arm-none-eabi-gcc`, we infer this is a `Cc::Yes` linker flavor, but the `lld` component is unknown and therefore defaulted to the target's linker flavor, `Lld::Yes`.

<details>
<summary>Walkthrough of how this happens</summary>

The linker flavor used is a mix between what can be inferred from the CLI (`-C linker`) and the target's default linker flavor:

- there is no linker flavor on the CLI (and that also offers another workaround on nightly: `-C linker-flavor=gnu-cc -Zunstable-options`), so it will have to be inferred [from here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1334-L1336) to [here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1321-L1327).
- in [`infer_linker_hints`](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L320-L352) `-C linker=arm-none-eabi-gcc` infers a `Some(Cc::Yes)` cc hint, and no hint about lld.
- the target's `linker_flavor` is combined in `with_cli_hints` with these hints. We have our `Cc::Yes`, but there is no hint about lld, [so the target's flavor `lld` component is used](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L356-L358). It's [`Gnu(Cc::No, Lld::Yes)`](https://github.com/rust-lang/rust/blob/993deaa0bf8bab9dd3eadfd1fbeb093328e95afe/compiler/rustc_target/src/spec/thumb_base.rs#L35).
- so we now have our `Gnu(Cc::Yes, Lld::Yes)` flavor

</details>

This results in a `Gnu(Cc::Yes, Lld::Yes)` flavor on a non-lld linker, causing an additional unexpected `-fuse-ld=lld` argument to be passed.

I don't know if this target defaulting to `rust-lld` is expected, but until MCP510's new linker flavor are stable, when people will be able to describe their linker/flavor accurately, this PR keeps the stable behavior of not doing anything when the linker/flavor on the CLI unexpectedly conflict with the target's.

I've tested this on a `no_std` `-C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles --target thumbv6m-none-eabi` example, trying to simulate one of `cortex-m`'s test mentioned in issue rust-lang#113597 (I don't know how to build a local complete  `thumbv6m-none-eabi` toolchain to run the exact test), and checked that `-fuse-lld` was indeed gone and the error disappeared.

r? ```@petrochenkov```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 13, 2023
make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

Fixes rust-lang#113597, which contains more details on how this happens through the code, and showcases an unexpected `Gnu(Cc::Yes, Lld::Yes)` flavor.

rust-lang#112910 added support to use `lld` when the flavor requests it, but didn't explicitly do so only when using `-Clink-self-contained=+linker` or one of the unstable `-Clinker-flavor`s.

The problem: some targets have a `lld` linker and flavor, e.g. `thumbv6m-none-eabi` from that issue. Users can override the linker but there are no linker flavors precise enough to describe the linker opting out of lld: when using `-Clinker=arm-none-eabi-gcc`, we infer this is a `Cc::Yes` linker flavor, but the `lld` component is unknown and therefore defaulted to the target's linker flavor, `Lld::Yes`.

<details>
<summary>Walkthrough of how this happens</summary>

The linker flavor used is a mix between what can be inferred from the CLI (`-C linker`) and the target's default linker flavor:

- there is no linker flavor on the CLI (and that also offers another workaround on nightly: `-C linker-flavor=gnu-cc -Zunstable-options`), so it will have to be inferred [from here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1334-L1336) to [here](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_codegen_ssa/src/back/link.rs#L1321-L1327).
- in [`infer_linker_hints`](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L320-L352) `-C linker=arm-none-eabi-gcc` infers a `Some(Cc::Yes)` cc hint, and no hint about lld.
- the target's `linker_flavor` is combined in `with_cli_hints` with these hints. We have our `Cc::Yes`, but there is no hint about lld, [so the target's flavor `lld` component is used](https://github.com/lqd/rust/blob/5dac6b320be868f898a3c753934eabc79ff2e406/compiler/rustc_target/src/spec/mod.rs#L356-L358). It's [`Gnu(Cc::No, Lld::Yes)`](https://github.com/rust-lang/rust/blob/993deaa0bf8bab9dd3eadfd1fbeb093328e95afe/compiler/rustc_target/src/spec/thumb_base.rs#L35).
- so we now have our `Gnu(Cc::Yes, Lld::Yes)` flavor

</details>

This results in a `Gnu(Cc::Yes, Lld::Yes)` flavor on a non-lld linker, causing an additional unexpected `-fuse-ld=lld` argument to be passed.

I don't know if this target defaulting to `rust-lld` is expected, but until MCP510's new linker flavor are stable, when people will be able to describe their linker/flavor accurately, this PR keeps the stable behavior of not doing anything when the linker/flavor on the CLI unexpectedly conflict with the target's.

I've tested this on a `no_std` `-C linker=arm-none-eabi-gcc -C link-arg=-nostartfiles --target thumbv6m-none-eabi` example, trying to simulate one of `cortex-m`'s test mentioned in issue rust-lang#113597 (I don't know how to build a local complete  `thumbv6m-none-eabi` toolchain to run the exact test), and checked that `-fuse-lld` was indeed gone and the error disappeared.

r? ````@petrochenkov````
@lqd
Copy link
Member

lqd commented Jul 13, 2023

Thanks for the information, it's very helpful.

The good news is that using GCC as a linker is still working 😅. In the future where we switch to lld, maybe rustc will also need an explicit linker flavor here, to make sure the flavor it guesses actually matches your expectations. But that's a topic for another day/stabilization: in the more immediate future, things will return to normal for the target and cortex-m's tests in a day or two.

@thejpster
Copy link

thejpster commented Jul 13, 2023

Thank you!

Just to come back to this point:

I don't know who the target maintainers are

I was under the impression that Arm Ltd were maintaining the thumbv*m-none-eabi* targets. However, they don't appear to have the platform-support documentation that is required for more recently added targets. Maybe that could be addressed.

@bors bors closed this as completed in fc1cb04 Jul 13, 2023
@lqd
Copy link
Member

lqd commented Jul 14, 2023

This is now on beta, reopening to track backport.

@lqd lqd reopened this Jul 14, 2023
@jyn514 jyn514 added this to the 1.72.0 milestone Jul 14, 2023
@workingjubilee
Copy link
Contributor

@thejpster Pursuant to your concern I have opened #113739 to summarize the issue that affects indeed all our tier 2 targets.

@lqd
Copy link
Member

lqd commented Aug 10, 2023

The backport of #113631 has landed on beta in #113938. We can close this issue as fixed.

@lqd lqd closed this as completed Aug 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 15, 2023
Infer `Lld::No` linker hint when the linker stem is a generic compiler driver

This PR basically reverts the temporary solution in rust-lang#113631 to a more long-term solution.

r? `@petrochenkov`

In [this comment](rust-lang#113631 (comment)), you had ideas about a long-term solution:

> I wonder what a good non-temporary solution for the inference would look like.
>
>     * If the default is `(Cc::No, Lld::Yes)` (e.g. `rust-lld`)
>
>       * and we switch to some specific platform compiler (e.g. `-C linker=arm-none-eabi-gcc`), should we change to `Lld::No`? Maybe yes?
>       * and we switch to some non-default but generic compiler `-C linker=clang`? Then maybe not?
>
>     * If the default is `(Cc::Yes, Lld::Yes)` (e.g. future x86_64 linux with default LLD)
>
>       * and we switch to some specific platform compiler (e.g. `-C linker=arm-none-eabi-gcc`), should we change to `Lld::No`? Maybe yes?
>       * and we switch to some non-default but generic compiler `-C linker=clang`? Then maybe not?
>

I believe that we should infer the `Lld::No` linker hint for any `-Clinker` override, and all the cases above:
- the linker drivers have their own defaults, so in my mind `-Clinker` is a signal to use its default linker / flavor, rather than ours or the target's. In the case of generic compilers, it's more likely than not going to be `Lld::No`. I would expect this to be the case in general, even when including platform-specific compilers.
- the guess will be wrong if the linker driver uses lld by default (and we also don't want to search for `-fuse-ld` link args), but will work in the more common cases. And the minority of other cases can fix the wrong guess by opting into the precise linker flavor.
- this also ensures backwards-compatibility: today, even on targets with an lld default and overriding the linker, rustc will not use lld. That includes `thumbv6m-none-eabi` where issue rust-lang#113597 happened.

It looks like the simplest option, and the one with least churn: we maintain the current behavior in ambiguous cases.

I've tested that this works on rust-lang#113597, as expected from the failure.

(I also have a no-std `run-make` test using a custom target json spec: basically simulating a future `x86_64-unknown-linux-gnu` using an lld flavor by default, to check that  e.g. `-Clinker=clang` doesn't use lld. I could add that test to this PR, but IIUC such a custom target requires `cargo -Z build-std` and we have no tests depending on this cargo feature yet. Let me know if you want to add this test of the linker inference for such targets.)

What do you think ?
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 15, 2023
Infer `Lld::No` linker hint when the linker stem is a generic compiler driver

This PR basically reverts the temporary solution in rust-lang#113631 to a more long-term solution.

r? ``@petrochenkov``

In [this comment](rust-lang#113631 (comment)), you had ideas about a long-term solution:

> I wonder what a good non-temporary solution for the inference would look like.
>
>     * If the default is `(Cc::No, Lld::Yes)` (e.g. `rust-lld`)
>
>       * and we switch to some specific platform compiler (e.g. `-C linker=arm-none-eabi-gcc`), should we change to `Lld::No`? Maybe yes?
>       * and we switch to some non-default but generic compiler `-C linker=clang`? Then maybe not?
>
>     * If the default is `(Cc::Yes, Lld::Yes)` (e.g. future x86_64 linux with default LLD)
>
>       * and we switch to some specific platform compiler (e.g. `-C linker=arm-none-eabi-gcc`), should we change to `Lld::No`? Maybe yes?
>       * and we switch to some non-default but generic compiler `-C linker=clang`? Then maybe not?
>

I believe that we should infer the `Lld::No` linker hint for any `-Clinker` override, and all the cases above:
- the linker drivers have their own defaults, so in my mind `-Clinker` is a signal to use its default linker / flavor, rather than ours or the target's. In the case of generic compilers, it's more likely than not going to be `Lld::No`. I would expect this to be the case in general, even when including platform-specific compilers.
- the guess will be wrong if the linker driver uses lld by default (and we also don't want to search for `-fuse-ld` link args), but will work in the more common cases. And the minority of other cases can fix the wrong guess by opting into the precise linker flavor.
- this also ensures backwards-compatibility: today, even on targets with an lld default and overriding the linker, rustc will not use lld. That includes `thumbv6m-none-eabi` where issue rust-lang#113597 happened.

It looks like the simplest option, and the one with least churn: we maintain the current behavior in ambiguous cases.

I've tested that this works on rust-lang#113597, as expected from the failure.

(I also have a no-std `run-make` test using a custom target json spec: basically simulating a future `x86_64-unknown-linux-gnu` using an lld flavor by default, to check that  e.g. `-Clinker=clang` doesn't use lld. I could add that test to this PR, but IIUC such a custom target requires `cargo -Z build-std` and we have no tests depending on this cargo feature yet. Let me know if you want to add this test of the linker inference for such targets.)

What do you think ?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command line interface to the compiler. A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants