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

make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors #113631

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

lqd
Copy link
Member

@lqd lqd commented Jul 12, 2023

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

#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-flavors.

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.

Walkthrough of how this happens

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

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 #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

because sometimes users can't opt out
@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 Jul 12, 2023
@lqd
Copy link
Member Author

lqd commented Jul 12, 2023

If vadim agrees this is a good direction (compared to e.g. changing flavor inference and hints to bias -Clinker=arm-none-eabi-gcc towards inferring (Cc::Yes, Lld::No) instead of (Cc::Yes, target.lld) -- as most if not all ccs will themselves default to bfd), then we could nominate it for a backport since #112910 is already in beta.

@petrochenkov
Copy link
Contributor

Why doesn't -fuse-ld=lld work with arm-none-eabi-gcc?
Does that version of gcc simply not support -fuse-ld=lld, or it works but it's not able to find lld shipped with rustc?

@petrochenkov
Copy link
Contributor

Looks like it's the latter, but why isn't it able to find it?

we@DESKTOP-5ALPNIQ:~$ which ld.lld
/usr/bin/ld.lld
we@DESKTOP-5ALPNIQ:~$ arm-none-eabi-gcc -fuse-ld=lld main.c
collect2: fatal error: cannot find 'ld'
compilation terminated.
we@DESKTOP-5ALPNIQ:~$ arm-none-eabi-gcc -fuse-ld=unknown main.c
arm-none-eabi-gcc: error: unrecognized command-line option '-fuse-ld=unknown'

In my case it's in /usr/bin, so arm-none-eabi-gcc is likely hard-coded to search in very specific paths.
Does it ignore even the search path explicitly passed by rustc via -B /path/to/dir-with-rustc-shipped-lld?

@lqd
Copy link
Member Author

lqd commented Jul 13, 2023

or it works but it's not able to find lld shipped with rustc?

To be clear, in the cortex-m test, they're not using self-contained linking, so #112910 changed things unexpectedly for the general case.

Does it ignore even the search path explicitly passed by rustc via -B /path/to/dir-with-rustc-shipped-lld?

No, arm-none-eabi-gcc does not ignore the search path for me when using -Clink-self-contained=+linker -Zunstable-options, at least there's no cannot find 'ld' error anymore on my example and it seems linked successfully.

There are multiple workarounds to make things work, the question is more: is it acceptable that this stable behavior has changed when using -Clinker=arm-none-eabi-gcc, now that we are taking into account the non-hinted lld component from the target linker flavor ? That can be surprising.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 13, 2023

Ok, I did some more testing and figured what happens too.

  • The directory containing shipped rust-lld (<toolchain>/lib/rustlib/x86_64-unknown-linux-gnu/bin) is added to search directories (despite +linker not being specified), that's why the target finds its rust-lld linker and works by default.
  • The directory containing shipped ld.lld (<toolchain>/lib/rustlib/x86_64-unknown-linux-gnu/bin/gcc-ld) is not added to search paths (unless +linker is specified), that's why the -fuse-ld=lld doesn't find its ld.lld linker.

in the cortex-m test, they're not using self-contained linking

They do use self-contained linking because they find and use lld shipped with rustc.
+linker is just not fully implemented yet, otherwise it would control whether rust-lld shipped with rustc would be found or not, and the thumbv6m-none-eabi target's spec would enable +linker by default.
(The two directories mentioned above should also be merged into one.)

@petrochenkov
Copy link
Contributor

My point is that the case in #113597 should actually work.
(For now I'm ignoring that the underlying linker change from ld to lld can also be surprising even if both work successfully.)

@lqd
Copy link
Member Author

lqd commented Jul 13, 2023

(For now I'm ignoring that the underlying linker change from ld to lld can also be surprising even if both work successfully.)

I was myself mostly concerned about that: adding -fuse-ld=lld where lld wasn't used before. Hence this PR bailing out so that -Clinker=arm-none-eabi-gcc keeps using arm-none-eabi-ld.bfd. The OP mentions that there are use-cases that -Clinker=arm-none-eabi-gcc caters to, that are different from -Clinker=arm-none-eabi-ld, and from -Clinker=rust-lld (the more common case).

My point is that the case in #113597 should actually work.

There were two ways to think about this: we could do nothing when none of the unstable flags are used, or change the target specs defaulting to rust-lld to enable +linker (but with a "hidden" change of linkers in some cases). Otherwise we get the discrepancy you're mentioning of having 2 ways to use the exact same linker, with one of them failing due to the missing search path.

Would you prefer I close this PR and update the target specs like thumbv6m-none-eabi's instead ?

@lqd
Copy link
Member Author

lqd commented Jul 13, 2023

Though as I said, the description in #113597 (comment) makes me think that the future end goal for these use-cases is to be able to opt out of lld when needed, via -Clinker=arm-none-eabi-gcc -Clinker-flavor=gnu-cc. And land this PR in the meantime.

@petrochenkov
Copy link
Contributor

Would you prefer I close this PR and update the target specs like thumbv6m-none-eabi's instead ?

No, just thinking aloud about a possible long term solution.

@petrochenkov
Copy link
Contributor

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?

Maybe the value should depend on the value of -Cself-contained:+linker?

Or maybe we should just keep the current scheme because it's simple, and it's all guessing anyway, and if the new linker flavors are stabilized then we can guess wrong.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2023

📌 Commit 2b61a5e 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2023
@lqd
Copy link
Member Author

lqd commented Jul 13, 2023

It has been discussed before, and IIUC rejected because it's incomplete and all guesses anyways, but maybe an additional data point in the -C linker=arm-none-eabi-gcc|clang guessing situation would be to also look for a few -fuse-ld well-known values (but maybe not paths) used in linker args.

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````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 13, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#113536 (avoid building proof trees in select)
 - rust-lang#113558 (Only use max_line_length = 100 for *.rs)
 - rust-lang#113570 (refactor proof tree formatting)
 - rust-lang#113623 (Add jump to doc)
 - rust-lang#113629 (Add Adt to SMIR)
 - rust-lang#113631 (make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc1cb04 into rust-lang:master Jul 13, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 13, 2023
@lqd lqd deleted the fix-113597 branch July 14, 2023 13:41
@lqd
Copy link
Member Author

lqd commented Jul 14, 2023

Issue #113597 now happens on beta, nominating this PR for backport.

Summary: on targets where an lld flavor is the default, and when using a -Clinker override with a platform-specific linker driver (like arm-none-eabi-gcc on thumbv6m-none-eabi in issue #113597), we need to guess the linker flavor, and inference uses the target's default linker flavor in this situation.

This can be surprising but was entirely non-consequential until #112910. It started using the linker flavor to prepare for MCP510's -Clink-self-contained=+linker -Clinker-flavor=gnu-lld-cc to pass arguments to the linker driver. In that situation, the inference guess resulted in an additional -fuse-ld=lld argument when calling arm-none-eabi-gcc , causing issue #113597.

The hope for future stabilization is that the new linker flavors will allow more precise descriptions of the linker used, even when using a generic linker driver, so that rustc doesn't need to guess and people are not surprised.

Until then, this PR fixes the issue and bypasses the new behavior introduced in #112910, by just bailing out if none of its unstable arguments are used.

@lqd lqd added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 14, 2023
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 20, 2023
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 22, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.73.0, 1.72.0 Jul 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2023
…k-Simulacrum

[beta] backport

This PR backports:

- rust-lang#113690: allow opaques to be defined by trait queries, again
- rust-lang#113631: make MCP510 behavior opt-in to avoid conflicts between the CLI and target flavors

It also includes a bump of bootstrap to the released stable.

r? `@Mark-Simulacrum`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request 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 pull request 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
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.

Regression when using linker=arm-none-eabi-gcc
6 participants