Implement unstable `-Clinker-flavor=gcc:lld` for MCP 510 #96827

lqd · 2022-05-07T21:31:07Z

This PR is extracted from #96401 for easier review.

It implements half of MCP #510: adding a gcc:lld linker-flavor on the CLI, as a prerequisite for supporting rust-lld directly via a command similar to -Clinker-flavor=gcc:lld -Clink-self-contained=linker.

The option is unstable and requires -Z unstable-options to be used.

When used without -Clink-self-contained=linker, it's equivalent to -Clink-arg=-fuse-ld=lld.

r? @petrochenkov

rust-highfive · 2022-05-07T21:31:10Z

⚠️ Warning ⚠️

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

compiler/rustc_codegen_ssa/src/back/link.rs

petrochenkov · 2022-05-14T20:18:31Z

I suddenly got busy and will get to this PR (and then to #96884) next weekend most likely.

petrochenkov · 2022-05-21T22:58:10Z

gcc linker-flavor accept a colon-separator to detect what to pass to cc. For example, -Clinker-flavor=gcc:$linker will be passed as -fuse-ld=$linker verbatim.

But that's not a linker flavor, that's literally -Zgcc-ld that you (we?) have been trying to eliminate.

If that was a linker flavor, then we'd add

"gcc-ld"
"gcc-wasm-ld"
"gcc-ld64.lld"
"gcc-ld.lld"
"gcc-lld-link"

to already existing

"ld"
"wasm-ld"
"ld64.lld"
"ld.lld"
"lld-link"

flavors.
No strings passed around verbatim, only "generalized classes" of linkers.

If we want to pass strings to -fuse-ld verbatim then it's better to keep a separate option like -Zgcc-ld.
At the same time, half of the concerns on the Zulip thread was about how "passing strings to linker is bad and we need something more high level".

EDIT:

a separate option like -Zgcc-ld

And that option already exists on stable and is called -Clink-arg=-fuse-ld=$linker, which makes -Zgcc-ld unnecessary and removable if we offload the "self-containedness" part to #96884.

petrochenkov · 2022-05-21T23:02:38Z

(I need to reread the zulip thread because I'm starting to forget all of the colors of this shed.)

bors · 2022-05-26T01:57:23Z

☔ The latest upstream changes (presumably #97409) made this pull request unmergeable. Please resolve the merge conflicts.

lqd · 2022-05-31T15:35:13Z

Vadim and I discussed this over zulip. He'll have some feedback about the PR, but in the meantime other issues were filed to track questions and various refactorings for these parts related to linkers.

I've rebased this PR over the -Zgcc-ld=lld simplifications of #97375.

@rustbot ready

petrochenkov · 2022-06-17T21:35:23Z

Sorry for the delay.
I'm now back from vacation and will return to this soon.

petrochenkov · 2022-06-27T08:56:22Z

Update: I'm still against having a free form component in -Clinker-flavor, but I don't have time to write a good motivation and finish my experiments in petrochenkov@836063a / petrochenkov@2227d27.
The timing has been really unlucky here, once I'm less busy and can allocate a few continuous hours of Rust work, this is the first thing I'm going to address.

bors · 2022-07-14T11:23:13Z

☔ The latest upstream changes (presumably #98975) made this pull request unmergeable. Please resolve the merge conflicts.

only the stable values for `-Clinker-flavor` can be used on stable until we have more feedback on the interface

lqd · 2022-07-25T15:05:34Z

Rebased.

Update: I'm still against having a free form component in -Clinker-flavor

@petrochenkov I've pushed a separate commit to remove that, and use the existing LdImpl enum instead.

The two experiments are interesting but look more like general refactorings that don't necessarily impact the CLI for the MCP or prevent from making progress here until stabilization ? (and when they do impact the CLI, this can be seen as breaking changes on stable flags, something that I've avoided)

petrochenkov · 2022-08-08T18:05:18Z

Sorry for the delay.

So I wanted to try documenting what "linker flavor" even means.
(I'll consider both -Clinker-flavor and target specs.)

All linkers have some kind of command line interfaces and rustc needs to know which commands to use with each of them.
So we clusterize all such interfaces into a (somewhat arbitrary) number of classes and call them "linker flavors".

Technically, it's not even necessary, we can nearly always infer the flavor from linker name and target properties like is_like_windows/is_like_osx/etc.
However, the PRs originally introducing -Zlinker-flavor (#40018 and friends) were aiming to reduce this kind of inference and provide something certain and explicitly specified instead.

The minimal set of linker flavors could look something like this (1)

// Unix-like linker with GNU extensions (both naked and compiler-wrapped forms)
// Besides similar "default" Linux/BSD linkers this also include Windows/GNU, which is somewhat different.
Gnu { cc: bool },
// Basic Unix-like linker, possibly with non-GNU extensions aka "any other Unix" (both naked and compiler-wrapped forms)
// Apple targets, Wasm, Solaris/illumos, l4re, msp430 - very different targets.
// Why this exists separately from GNU - the `linker_is_gnu` target property (which I will include into linker flavor in this post)
// is annoying to infer using other target properties.
Unix { cc: bool },
// MSVC linker has a unique interface
Msvc,
// Other linker-like tools with unique interfaces for exotic targets
EmCc,
Bpf,
Ptx,

Then LLD appears (#48125) and real life forces our hand to increase the number of clusters to be a bit less minimal.
LLD linker is self-sufficent only if its executable (argv[0]) is named in specific way, otherwise it doesn't work and requires a -flavor LLD_FLAVOR argument to choose which logic to use.
Our shipped rust-lld in particular is not named in such specific way, maybe it was a mistake to ship it like this, but it's too late now.
So it needs the flavor option, so let's also make it "certain and explicit" and not inferred from other target properties.
(I'll also assume we want to call LLD through a C compiler too, which is the point of this PR.)

This changes our set of linker flavors to something like this (2):

// Unix-like linker with GNU extensions (both naked and compiler-wrapped forms)
// Besides similar "default" Linux/BSD linkers this also include Windows/GNU, which is somewhat different.
Gnu { lld: bool, cc: bool },
// Unix-like linker for Apple targets (both naked and compiler-wrapped forms)
// Why it was extracted from "umbrella" Unix - due to the corresponding LLD flavor.
Darwin { lld: bool, cc: bool },
// Unix-like linker for Wasm targets (both naked and compiler-wrapped forms)
// Why it was extracted from "umbrella" Unix - due to the corresponding LLD flavor.
// Non-LLD version does not exist, so the lld flag here is currently hardcoded and not boolean.
WasmLld { cc: bool },
// Basic Unix-like linker, possibly with non-GNU extensions aka "any other Unix" (both naked and compiler-wrapped forms)
// Solaris/illumos, l4re, msp430 - very different targets.
// LLD doesn't support any of these.
Unix { cc: bool },
// MSVC linker has a unique interface
// LLD supports it
Msvc { lld: bool },
// Other linker-like tools with unique interfaces for exotic targets
EmCc,
Bpf,
Ptx,

Then we could increase the number of clusters even more, e.g. to cover most of conditions in compiler\rustc_codegen_ssa\src\back\linker.rs (3):

Gnu { lld: bool, cc: bool },
Mingw { lld: bool, cc: bool }, // split from GNU
Darwin { lld: bool, cc: bool },
WasmLld { cc: bool },
Solaris { cc: bool }, // split from Unix
L4Re { cc: bool }, // split from Unix
Unix { cc: bool }, // "other Unix" still needs to exist
Msvc { lld: bool },
EmCc,
Bpf,
Ptx,

We can realistically do this, but it may be too much for my taste. It would still not eliminate all the target-specific logic in the rustc_codegen_ssa though.
Anyway, then number of clusters is somewhat arbitrary, as I said above.

So what conclusions I draw from all of this.

I'd like to keep the original "something certain and explicitly specified" idea of linker flavor, instead of inferring things from strings and other properties.
- This means no :arbitrary-string part for -Clinker-flavor, maybe this second component can be added to -Clinker instead in the future, although I don't think it's necessary now, because -Clink-arg=-fuse-ld=/mold/or/whatever still exists.
I'd like to keep the number of linker flavor "clusters" minimal under existing circumstances, and make the set (2) user-facing (both in -Clinker-flavor and in json target specs).
E.g. the (non-legacy) accepted string values will be
```
gnu
gnu-lld
gnu-cc
gnu-lld-cc
darwin
darwin-lld
darwin-cc
darwin-lld-cc
wasm-lld
wasm-lld-cc
unix
unix-cc
msvc
msvc-lld
em-cc
bpf
ptx
```
instead of the current
```
ld
ld.lld
gcc
ld64.lld
wasm-ld
msvc
lld-link
l4-bender
em
bpf-linker
ptx-linker
```
(I tried to keep the naming consistent.)
Proof-of-concept branch for this scheme (without any backward compatibility) - https://github.com/petrochenkov/rust/tree/flavor2.

petrochenkov · 2022-08-08T18:06:19Z

I'll review the code in this PR a bit later, I expect that we can mostly land it as is.

petrochenkov · 2022-08-09T21:53:22Z

Since the free form component is removed, gcc:lld can be renamed to gcc-lld (*) and added to enum LinkerFlavor.
fn add_link_args can then fill link args for this flavor if flags for gcc are filled.
LinkerFlavorCli is also no longer necessary, all flavors are well known.

(*) Although I'd prefer to use the scheme from #96827 (comment) eventually.

petrochenkov · 2022-08-09T21:59:27Z

compiler/rustc_codegen_ssa/src/back/link.rs

+            "Using `lld`, self-contained linking, and coverage or profile generation has known \
+            issues. See issue #79555 for more details, at \
+            https://github.com/rust-lang/rust/issues/79555",
+        );


Ugh.
Is it really a compiler's job to do this?

Same about other cases of copypasting issue tracker into the compiler's source code.

petrochenkov · 2022-08-09T22:04:51Z

compiler/rustc_codegen_ssa/src/back/link.rs

+        cmd.arg(format!("-Wl,-rustc-lld-flavor={}", sess.target.lld_flavor.as_str()));
+    } else {
+        // Otherwise, we were asked to use `lld` but not `rust-lld`.
+        cmd.arg("-fuse-ld=lld");


Maybe this PR should wait for #100200, in addition to fixing the regression it basically makes the "self-contained" and "lld" aspects of -Zgcc-ld orthogonal and removes useless checks like

// - checking the `lld-wrapper`s exist in the sysroot

above, that check something that is not necessarily true, and will prevent finding system lld instead.

petrochenkov · 2022-08-09T22:08:31Z

src/test/ui/linkers/gcc_ld_mismatch.rs

+// compile-flags: -Zgcc-ld=lld -Clinker-flavor=em -Zunstable-options
+
+// Test ensuring that until the unstable flag is removed (if ever), if both the linker-flavor and
+// `gcc-ld` flags are used, they ask for the same linker.


If making these two options incompatible helps to remove at least some code or conditions in fn handle_cli_linker_flavors then please let's do that instead.

rustc_target: Add a compatibility layer to separate internal and user-facing linker flavors I want to do some refactorings in `rustc_target` - merge `lld_flavor` and `linker_is_gnu` into `linker_flavor`, support combination gcc+lld (rust-lang#96827). This PR adds some compatibility infra that makes that possible without making any changes to user-facing interfaces - `-Clinker-flavor` values and json target specs. (For json target specs this infra may eventually go away since they are not very stable.) The second commit does some light refactoring of internal linker flavors (applies changes from petrochenkov@53eca42 that don't require mass-editing target specs).

bors · 2022-09-02T13:52:35Z

☔ The latest upstream changes (presumably #101318) made this pull request unmergeable. Please resolve the merge conflicts.

In accordance with the design from rust-lang#96827 (comment)

rustc_target: Refactor internal linker flavors In accordance with the design from rust-lang#96827 (comment) `lld_flavor` and `linker_is_gnu` fields are removed from internal target specs, but still parsed from JSON specs using compatibility layer introduced in rust-lang#100552. r? `@lqd`

lqd · 2023-07-01T00:15:01Z

superseded by #112910

Implement most of MCP510 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](https://github.com/lqd/rust/blob/9eee230cd0a56bfba3ce65121798d9f9f4341cdd/compiler/rustc_codegen_ssa/src/back/link.rs#L1671-L1676). - 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](rust-lang#96827 (comment)) 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-flavor`s 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`

Implement most of MCP510 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](https://github.com/lqd/rust/blob/9eee230cd0a56bfba3ce65121798d9f9f4341cdd/compiler/rustc_codegen_ssa/src/back/link.rs#L1671-L1676). - 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](rust-lang/rust#96827 (comment)) 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-flavor`s 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`

rust-highfive assigned petrochenkov May 7, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2022

lqd mentioned this pull request May 7, 2022

Implement -Zgcc-ld=lld stabilization MCP #96401

Closed

bjorn3 reviewed May 8, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Show resolved Hide resolved

lqd force-pushed the mcp_linker_flavor branch from 49a06ab to 0e60302 Compare May 8, 2022 06:56

This comment was marked as resolved.

Sign in to view

lqd force-pushed the mcp_linker_flavor branch from 3ced29e to b9a413d Compare May 21, 2022 17:11

lqd force-pushed the mcp_linker_flavor branch from b9a413d to aab6ab2 Compare May 31, 2022 15:23

nagisa mentioned this pull request Jun 3, 2022

contribute-nearcore: add info on ROCKSDB_LIB_DIR and other minor updates near/docs#1076

Merged

lqd added 7 commits July 25, 2022 10:48

add dedicated -C linker-flavor structures

3c08036

add tests for LinkerFlavorCli options parsing

8ef3d5b

use LinkerFlavorCli for -C linker-flavor

cc5ac27

implement linker-flavor enrichments for MCP

045cb0a

add sanity check about -Zgcc-ld and -Clinker-flavor=gcc:*

8d80814

require -Z unstable-options to use the new linker-flavor flag values

877fa6b

only the stable values for `-Clinker-flavor` can be used on stable until we have more feedback on the interface

test warning about known issue 79555 with lld

6e0bff9

lqd added 2 commits July 25, 2022 11:03

make an impossible case a bug!

a4c8128

remove freeform component from LinkerFlavorCli

08395ed

lqd force-pushed the mcp_linker_flavor branch from aab6ab2 to 08395ed Compare July 25, 2022 14:54

lqd changed the title ~~Implement unstable -Clinker-flavor=gcc:* for MCP 510~~ Implement unstable -Clinker-flavor=gcc:lld for MCP 510 Jul 25, 2022

petrochenkov reviewed Aug 9, 2022

View reviewed changes

petrochenkov 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 Aug 9, 2022

petrochenkov mentioned this pull request Aug 14, 2022

rustc_target: Add a compatibility layer to separate internal and user-facing linker flavors #100552

Merged

petrochenkov mentioned this pull request Sep 18, 2022

rustc_target: Refactor internal linker flavors #101988

Merged

petrochenkov added a commit to petrochenkov/rust that referenced this pull request Oct 6, 2022

rustc_target: Refactor internal linker flavors

572b6a9

In accordance with the design from rust-lang#96827 (comment)

pnkfelix mentioned this pull request Mar 16, 2023

-C linker-flavor=clang rust-lang/compiler-team#601

Closed

3 tasks

lqd mentioned this pull request Jun 21, 2023

Implement most of MCP510 #112910

Merged

lqd closed this Jul 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unstable `-Clinker-flavor=gcc:lld` for MCP 510 #96827

Implement unstable `-Clinker-flavor=gcc:lld` for MCP 510 #96827

lqd commented May 7, 2022 •

edited

Loading

rust-highfive commented May 7, 2022

petrochenkov commented May 14, 2022

This comment was marked as resolved.

petrochenkov commented May 21, 2022 •

edited

Loading

petrochenkov commented May 21, 2022

bors commented May 26, 2022

lqd commented May 31, 2022

petrochenkov commented Jun 17, 2022

petrochenkov commented Jun 27, 2022

bors commented Jul 14, 2022

lqd commented Jul 25, 2022

petrochenkov commented Aug 8, 2022 •

edited

Loading

petrochenkov commented Aug 8, 2022

petrochenkov commented Aug 9, 2022

petrochenkov Aug 9, 2022

petrochenkov Aug 9, 2022

petrochenkov Aug 9, 2022

petrochenkov Aug 9, 2022

bors commented Sep 2, 2022

lqd commented Jul 1, 2023

Implement unstable -Clinker-flavor=gcc:lld for MCP 510 #96827

Implement unstable -Clinker-flavor=gcc:lld for MCP 510 #96827

Conversation

lqd commented May 7, 2022 • edited Loading

rust-highfive commented May 7, 2022

petrochenkov commented May 14, 2022

This comment was marked as resolved.

petrochenkov commented May 21, 2022 • edited Loading

petrochenkov commented May 21, 2022

bors commented May 26, 2022

lqd commented May 31, 2022

petrochenkov commented Jun 17, 2022

petrochenkov commented Jun 27, 2022

bors commented Jul 14, 2022

lqd commented Jul 25, 2022

petrochenkov commented Aug 8, 2022 • edited Loading

petrochenkov commented Aug 8, 2022

petrochenkov commented Aug 9, 2022

petrochenkov Aug 9, 2022

Choose a reason for hiding this comment

petrochenkov Aug 9, 2022

Choose a reason for hiding this comment

petrochenkov Aug 9, 2022

Choose a reason for hiding this comment

petrochenkov Aug 9, 2022

Choose a reason for hiding this comment

bors commented Sep 2, 2022

lqd commented Jul 1, 2023

Implement unstable `-Clinker-flavor=gcc:lld` for MCP 510 #96827

Implement unstable `-Clinker-flavor=gcc:lld` for MCP 510 #96827

lqd commented May 7, 2022 •

edited

Loading

petrochenkov commented May 21, 2022 •

edited

Loading

petrochenkov commented Aug 8, 2022 •

edited

Loading