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

Implement most of MCP510 #112910

Merged
merged 11 commits into from
Jul 2, 2023
Merged

Implement most of MCP510 #112910

merged 11 commits into from
Jul 2, 2023

Conversation

lqd
Copy link
Member

@lqd lqd commented Jun 21, 2023

This implements most of what remains to be done for MCP510:

  • turns -C link-self-contained into a +/- list of components, like -C link-self-contained=+linker,+crto,+libc,+unwind,+sanitizers,+mingw. The scaffolding is present for all these expected components to be implemented and stabilized in the future on their own time. This PR only handles the -Zgcc-ld=lld subset of these link-self-contained components as -Clink-self-contained=+linker
  • handles -C link-self-contained=y|n as-is today, for compatibility with rustc_codegen_ssa::back::link::self_contained's explicit opt-in and opt-out.
  • therefore supports our plan to opt out of rust-lld (when it's enabled by default) even for current -Clink-self-contained users, with e.g. -Clink-self-contained -Clink-self-contained=-linker
  • turns add_gcc_ld_path into its expected final form, by using the -C link-self-contained=+linker CLI flag, and whether the LinkerFlavor has the expected Cc::Yes and Lld::Yes shape (this is not yet the case in practice for any CLI linker flavor)
  • makes the new clean linker flavors selectable in the CLI in addition to the legacy ones, in order to opt-in to using cc and lld to emulate -Zgcc-ld=lld
  • ensure the new -C link-self-contained components, and -C linker-flavors are unstable, and require -Z unstable-options to be used

The up-to-date set of flags for the future stable CLI version of -Zgcc-ld=lld is currently: -Clink-self-contained=+linker -Clinker-flavor=gnu-lld-cc -Zunstable-options.

It's possible we'll also need to do something for distros that don't ship rust-lld, but maybe there are already no tool search paths to be added to cc in this situation anyways.

r? @petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 21, 2023
@petrochenkov
Copy link
Contributor

Linking to the previous PRs: #96827 #96884

@petrochenkov
Copy link
Contributor

do you want the CLI to be able to select actual LinkerFlavors (directly, without using LinkerFlavorCli)

I actually planned to implement it next weekend.
So, yes, but not necessarily in this PR.

Like this

pub enum LinkerFlavorCli {
    // New
    Gnu(Cc, Lld),
    Darwin(Cc, Lld),
    WasmLld(Cc),
    Unix(Cc),
    Msvc(Lld),
    EmCc,
    Bpf,
    Ptx,
    // Old
    Gcc,
    Ld,
    Lld(LldFlavor),
    Msvc,
    Em,
    BpfLinker,
    PtxLinker,
}

Then some of the old ones can be removed because they are tier 3, and the rest of them will be supported forever (i.e. this is not even a migration).

Then I wanted to add a couple of new "generic" flavors like *-cc or *-lld-* where the unspecified parts would be inferred from the target spec (the actual user-visible names won't contain *s of course).

@petrochenkov petrochenkov marked this pull request as ready for review June 22, 2023 15:56
@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@petrochenkov
Copy link
Contributor

This basically looks nearly ready for merging to me.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2023
@lqd
Copy link
Member Author

lqd commented Jun 22, 2023

I can definitely add the new flavors to LinkerFlavorCli in this PR if you want, and a few tests, to make it a bit closer to usable for real.

I may miss some subtleties around the conversions, although now everything should be automatic, so I understand if you'd prefer doing it yourself.

@petrochenkov
Copy link
Contributor

Full implementation of -C link-self-contained will also require modifying bootstrap to put all the things into corresponding subdirectories (and retrieving from there in rustc).

@petrochenkov
Copy link
Contributor

@lqd
Yes, if you just add the new variants to enum LinkerFlavorCli it will do most of the work.
The only thing I expect breaking is check_compatibility, it may require writing out the match explicitly, because the roundtrip check will no longer work.

@lqd
Copy link
Member Author

lqd commented Jun 22, 2023

Ok great, I'll do that and report back.

@rustbot

This comment was marked as resolved.

@lqd
Copy link
Member Author

lqd commented Jun 22, 2023

I've tried adding the new flavors from the end of your old comment, and today's comment:

Like this

pub enum LinkerFlavorCli {
    // New
    Gnu(Cc, Lld),
    Darwin(Cc, Lld),
    WasmLld(Cc),
    Unix(Cc),
    Msvc(Lld),
    EmCc,
    Bpf,
    Ptx,
    // Old
    Gcc,
    Ld,
    Lld(LldFlavor),
    Msvc,
    Em,
    BpfLinker,
    PtxLinker,
}

A wrinkle was that we have both old and new values for Msvc so I merged the two and consider Msvc(Lld::No) as legacy.
That seemed safe to do, especially when keeping the legacy conversion code mostly as-is; but if you want we could have two different variants.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2023
@bors

This comment was marked as resolved.

@lqd
Copy link
Member Author

lqd commented Jun 28, 2023

rebased to fix conflicts

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 29, 2023

It's possible we'll also need to do something for distros that don't ship rust-lld, but maybe there are already no tool search paths to be added to cc in this situation anyways.

I expected this to not require special treatment, even if we add the directory in which rust-lld is usually shipped to search paths, it will be empty and the search will continue to find lld elsewhere on the system.

@lqd
Copy link
Member Author

lqd commented Jun 29, 2023

Agreed, I had the same opinion. We could also check that the build config has rust.lld enabled for -Clinked-self-contained=+linker to work, and otherwise emit an error, like we do for similar cases.

But indeed it might just be a case of verifying that the fallback to another LLD on the $PATH works as we expect -- e.g. by adding a test in the future.

@petrochenkov
Copy link
Contributor

In the end we'll probably have to drop bitflags again to accommodate for target defaults and heuristics.

Like

component1: Option<bool>,
...
componentN: Option<bool>,

Some(true) means explicitly enabled, Some(false) means explicitly disabled, and None means use default heuristics.

@petrochenkov
Copy link
Contributor

Or maybe LinkSelfContainedDefault will use options, and LinkSelfContained keeps using bitflags.
Let's keep things as is for now.

compiler/rustc_target/src/spec/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/mod.rs Show resolved Hide resolved
compiler/rustc_target/src/spec/mod.rs Show resolved Hide resolved
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2023
lqd added 6 commits June 30, 2023 21:07
linker flavors

- only the stable values for `-Clink-self-contained` can be used on stable until we
have more feedback on the interface
- `-Zunstable-options` is required to use unstable linker flavors
@lqd
Copy link
Member Author

lqd commented Jun 30, 2023

With a prayer to the MIPS gods of Segfaults:

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 30, 2023

📌 Commit 38f5a99 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2023
@bors
Copy link
Contributor

bors commented Jul 2, 2023

⌛ Testing commit 38f5a99 with merge 8e2d5e3...

@bors
Copy link
Contributor

bors commented Jul 2, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 8e2d5e3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2023
@bors bors merged commit 8e2d5e3 into rust-lang:master Jul 2, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jul 2, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8e2d5e3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 3
Improvements ✅
(secondary)
-1.1% [-1.2%, -1.0%] 6
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.5%, 2.7%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.1% [-2.5%, -1.7%] 2
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) 0.2% [-2.5%, 2.7%] 6

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.4% [3.7%, 7.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 662.486s -> 661.616s (-0.13%)

@lqd lqd deleted the mcp510 branch July 2, 2023 07:02
@lqd
Copy link
Member Author

lqd commented Jul 2, 2023

Note for weekly triage: this doesn't change the compiler in any significant way, and requires opt-in flags, so these diesel and wg-grammar perf wins are noise.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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`````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants