Linker flavors next steps: linker features #123656

lqd · 2024-04-08T23:15:36Z

This is my understanding of the first step towards @petrochenkov's vision for the future of linker flavors, described in #119906 (comment) and the discussion that followed.

To summarize: having Cc and Lld embedded in linker flavors creates tension about naming, and a combinatorial explosion of flavors for each new linker feature we'd want to use. Linker features are an extension mechanism that is complementary to principal flavors, with benefits described in #119906.

The most immediate use of this flag would be to turn self-contained linking on and off via features instead of flavors. For example, -Clinker-features=+/-lld would toggle using lld instead of selecting a precise flavor, and would be "generic" and work cross-platform (whereas linker flavors are currently more tied to targets). Under this scheme, MCP510 is expected to be -Clink-self-contained=+linker -Zlinker-features=+lld -Zunstable-options (though for the time being, the original flags using lld-cc flavors still work).

I purposefully didn't add or document CLI support for +/-cc, as it would be a noop right now. I only expect that we'd initially want to stabilize +/-lld to begin with.

r? @petrochenkov

You had requested that minimal churn would be done to the 230 target specs and this does none yet: the linker features are inferred from the flavor since they're currently isomorphic. We of course expect this to change sooner rather than later.

In the future, we can allow targets to define linker features independently from their flavor, and remove the cc and lld components from the flavors to use the features instead, this actually doesn't need to block stabilization, as we discussed.

(Best reviewed per commit)

rustbot · 2024-04-08T23:15:46Z

Some changes occurred in run-make tests.

cc @jieyouxu

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

petrochenkov · 2024-04-10T13:43:59Z

Also: the last commit here is purely cosmetic: 30% of the spec module was about linking so I moved these to a dedicated module. I can drop that commit if you prefer

Let's land it separately, before or after this PR.

petrochenkov · 2024-04-10T14:08:54Z

What happens if -Zlinker-features=+lld,-lld or -Zlinker-features=-lld -Zlinker-features=+lld.
IIRC, for -Clink-self-contained we just produced an error for cases like this.

petrochenkov · 2024-04-10T14:10:57Z

Let's land it separately, before or after this PR.

Or maybe not, the rest of the PR is noncontroversial, so I'm basically ready to r+ with #123656 (comment) addressed.

lqd · 2024-04-10T14:23:01Z

Let's land it separately, before or after this PR.

It's fine, we can land such a thing in the next PRs. I had removed it from my branch already by the time you added the other comment :)

What happens if -Zlinker-features=+lld,-lld or -Zlinker-features=-lld -Zlinker-features=+lld.

Right now nothing spectacularly amazing, in particular I was thinking whether MCP rust-lang/compiler-team#731 would need to be taken into account. I think we could at least make -Zlinker-features and -Clinker-self-contained=+/-component compose better, so we could do the +/- reduction while accumulating the CLI flags (e.g. +lld,-lld,+lld would not result in LinkerFeaturesCLI { enabled: lld, disabled: lld }, but simply in "enabled: lld").

So I will:

remove the cosmetic commit now
handle these cases of duplicate flags
remove +/-cc from the CLI (and documentation) only, because it's not really doing much yet and that could be confusing

lqd · 2024-04-10T18:23:51Z

Updated to:

remove the cosmetic commit, we'll land it later
handle duplicates like +lld,-lld as "the last feature flag wins"
only mention -Z linker-features=+/-lld in the code and documentation: we don't expose to users the cc feature that we have until it's actually doing something worthwhile during linking.

@rustbot ready

petrochenkov · 2024-04-11T16:13:29Z

compiler/rustc_codegen_ssa/src/back/link.rs

 /// - when the self-contained linker flag is active: the build of `lld` distributed with rustc,
 /// - or any `lld` available to `cc`.
+#[instrument(level = "debug", skip(cmd, sess))]
 fn add_lld_args(


There's one more place where the distinction between lld and non-lld is important - fn get_linker, it will add -flavor <FLAVOR> to the command line if the linker is lld.
Right now it won't be added even if -Clinker-features=+lld is specified.

Ideally fn linker_and_flavor would return a flavor already adjusted for -Clinker-features, then everything would automatically work correctly after that.

If that's the case then I'm not sure if we ever had -flavor <FLAVOR> for -Zgcc-ld=lld either, anything it did was way after get_linker, but I guess you may be talking about using -Clinker=lld directly and not via cc.

-flavor <FLAVOR> is not needed when lld is called through cc.
It's only needed when lld is called directly, and with an "improper" name not conveying the flavor, e.g. "rust-lld".

They are a flexible complementary mechanism to linker flavors, that also avoid the combinatorial explosion of mapping linking features to actual linker flavors.

but don't expose `+/-cc` yet

While they're isomorphic, we can flip the lld component where applicable, so that downstream doesn't have to check both the flavor and the linker features.

lqd · 2024-04-12T10:42:01Z

Ideally fn linker_and_flavor would return a flavor already adjusted for -Clinker-features, then everything would automatically work correctly after that.

Done. It feels like we'd want some kind of consistency check when features are toggled for flavors that wouldn't support them though: right now such adjustment would be a no op, but maybe we could be emitting errors in some cases. Probably not in all cases though, as the flavor can be inferred and could be wrong.

I'll mark this @rustbot ready but I'd like your thoughts on errors here. Maybe we can check for consistency when falling back to the unhinted target flavor. Maybe when we use both a modern linker flavor and the linker features on the CLI, so there's no inference/hinting involved.

petrochenkov · 2024-04-12T16:07:25Z

I'll think about the consistency checks later during stabilization.
@bors r+

bors · 2024-04-12T16:07:27Z

📌 Commit dee834b has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2024-04-13T11:10:05Z

⌛ Testing commit dee834b with merge 7106800...

bors · 2024-04-13T13:11:49Z

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

rust-timer · 2024-04-13T16:37:08Z

Finished benchmarking commit (7106800): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	3.9%	[3.9%, 3.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.3%	[-4.2%, -2.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-4.2%, 3.9%]	3

Cycles

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

Binary size

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

Bootstrap: 675.908s -> 677.551s (0.24%)
Artifact size: 316.10 MiB -> 316.04 MiB (-0.02%)

rustbot assigned petrochenkov Apr 8, 2024

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 Apr 8, 2024

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 Apr 10, 2024

lqd force-pushed the linker-features branch 5 times, most recently from 0058079 to a490b17 Compare April 10, 2024 17:32

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 Apr 10, 2024

petrochenkov reviewed Apr 11, 2024

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 Apr 11, 2024

lqd added 7 commits April 12, 2024 09:43

remove some unnecessary lifetimes

c39929c

introduce LinkerFeatures

695aac2

They are a flexible complementary mechanism to linker flavors, that also avoid the combinatorial explosion of mapping linking features to actual linker flavors.

add -Z linker-features to toggle lld on the CLI

2398d8c

but don't expose `+/-cc` yet

make CLI linker features influence the linker flavor

11b6d40

While they're isomorphic, we can flip the lld component where applicable, so that downstream doesn't have to check both the flavor and the linker features.

test self-contained linking via -Zlinker-features=+lld

5359d9c

document -Zlinker-features in the unstable book

317d0bd

test duplicate features, that the last +/-lld on the CLI wins

dee834b

lqd force-pushed the linker-features branch from a490b17 to dee834b Compare April 12, 2024 09:58

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 Apr 12, 2024

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 Apr 12, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 13, 2024

bors merged commit 7106800 into rust-lang:master Apr 13, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 13, 2024

lqd deleted the linker-features branch April 13, 2024 13:12

bors mentioned this pull request Apr 13, 2024

sess: stabilize -C stack-protector=all #121742

Closed

lqd mentioned this pull request Apr 30, 2025

Use lld by default on x86_64-unknown-linux-gnu stable #140525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linker flavors next steps: linker features #123656

Linker flavors next steps: linker features #123656

lqd commented Apr 8, 2024 •

edited

Loading

rustbot commented Apr 8, 2024

petrochenkov commented Apr 10, 2024

petrochenkov commented Apr 10, 2024

petrochenkov commented Apr 10, 2024

lqd commented Apr 10, 2024 •

edited

Loading

lqd commented Apr 10, 2024 •

edited

Loading

petrochenkov Apr 11, 2024

lqd Apr 12, 2024 •

edited

Loading

petrochenkov Apr 12, 2024

lqd commented Apr 12, 2024

petrochenkov commented Apr 12, 2024

bors commented Apr 12, 2024

bors commented Apr 13, 2024

bors commented Apr 13, 2024

rust-timer commented Apr 13, 2024

Linker flavors next steps: linker features #123656

Linker flavors next steps: linker features #123656

Conversation

lqd commented Apr 8, 2024 • edited Loading

rustbot commented Apr 8, 2024

petrochenkov commented Apr 10, 2024

petrochenkov commented Apr 10, 2024

petrochenkov commented Apr 10, 2024

lqd commented Apr 10, 2024 • edited Loading

lqd commented Apr 10, 2024 • edited Loading

petrochenkov Apr 11, 2024

Choose a reason for hiding this comment

lqd Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

petrochenkov Apr 12, 2024

Choose a reason for hiding this comment

lqd commented Apr 12, 2024

petrochenkov commented Apr 12, 2024

bors commented Apr 12, 2024

bors commented Apr 13, 2024

bors commented Apr 13, 2024

rust-timer commented Apr 13, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lqd commented Apr 8, 2024 •

edited

Loading

lqd commented Apr 10, 2024 •

edited

Loading

lqd commented Apr 10, 2024 •

edited

Loading

lqd Apr 12, 2024 •

edited

Loading