[WIP] Linker flavors: next steps #119906
Closed
[WIP] Linker flavors: next steps #119906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
These commits modify compiler targets. |
petrochenkov
added
S-waiting-on-author
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Let's talk about one of the hard problems in CS. Linker flavor namingThe base component is unfortunately neededIt would be nice to just drop the base flavor components (like gnu/darwin/wasm/etc) from user-facing flavors and leave only generic Generic flavorsI think usually the user intent behind generic flavors can be worded as "enable lld", or "enable clang", or "disable C compiler and use bare linker". So, with this +/- generic flavors would look like this.
The alternatives to that are names like Naming full flavors after generic flavorsWhat if we apply the +/- idea to full flavors? Existing flavors will turn into or into if we drop negatives, or into if we drop commas as well (looks farther from generic flavors).
ConclusionsI'm not sure, basically. Or change full flavors to the |
|
@rustbot ready (on the design, not implementation) |
rustbot
added
S-waiting-on-review
lqd
reviewed
Jan 16, 2024
There was a problem hiding this comment.
On the design and direction:
- a possibly nice thing I wasn't anticipating with the clang flavor is they'd simplify our wasm targets specs (it's maybe one of the WIP bits of the PR, as I'm not yet sure it's fully implemented just by reading the code, since these targets -- or the apple ones -- shouldn't have a flavor where
uses_clangreturns true inadd_lld_args?) - since the clang-specific features won't be used or referenced very often in the compiler or specs, do you think
Ccas today'sYes/Nowith theYesvariant itself gaining the two cc kinds would be better or worse than the 3-variantCcas shown here? My thoughts are we should be using/checking forCc::Yesbasically everywhere and otherwise will now have to check if it's yes or clang. Reading these made me wonder whetherCc::Yes(_)would nicely encode this property of "de-emphasizing" the clang variant in general, and look for it inCc::Yes(compiler_flavor)only when needed. - I find the idea of generic flavors interesting, as we expect it's a simplification for users that want the implicit default, compared to having to pick the system-specific flavor. However, I agree that it makes the design space of linker flavors larger, and increases friction with the dash confusion, as well as impacting regular flavor names where it may not (or should not) need to do so. Your exploration and conclusion above shows this quite clearly I feel. It's possible that we can reduce the design space here if we chose to limit this to a different flag, like
-Clinker-flavor-features, to keep the existing flavors and naming (including the new clang flavors, and without issues withllvm-bc) free from these concerns. A new attribute is suboptimal for sure, but also has the advantage of keeping-Clinker-flavorfully explicit, while the new attribute would be about "applying modifiers to the implicit default flavor". Generic flavors could be an increase in complexity for what should generally be a niche use-case (though a nice UX when you hit that specific use-case). Then again, maybe the dash confusion isn't that big of a deal (and it's better than having+/-in the full flavors) and-Clinker-flavor=+lld,-ccis obvious enough as modifiers, without the base value. We can document it well enough, and it's quite understandable as shorthand when shown next to the actual full flavors (and has the benefit of not needing to remember the component order in the flavor, as-Clinker-flavor=*-lld-ccin+/-terms would be*-lld-*and easily confused with an incorrect*-*-lld). So the question is: picking the least bad option between-Clinker-flavorwith possible dash confusion, and having the generic flavors as another flag.
Comment on lines
+2223
to
+2224
| insert(LinkerFlavor::Gnu(Cc::Clang, Lld::No)); | ||
| insert(LinkerFlavor::Gnu(Cc::Clang, Lld::Yes)); |
There was a problem hiding this comment.
Possibly still WIP: these linker args should have a WasmLld or Unix principal flavor here, rather than Gnu?
Yes, the current state of the PR is a sketch, apple targets should switch to clang as the default flavor, and
Maybe, but it's a big churn, probably not worth doing until everything stabilizes for sure. |
Looking at the diff, I agree. Let's keep the 3-variant enum for now and once we're a bit surer about the overall structure (components, order, naming, etc) we'll clean these up easily. |
So, I thought maybe we should do a 180 degree turn and shift all the logic except principal flavors to Feature flags are a more flexible mechanism than current linker flavors, in general.
|
jschwe
reviewed
Jan 18, 2024
Comment on lines
+2287
to
+2288
| if flavor.uses_clang() && sess.target.linker_flavor != sess.host.linker_flavor { | ||
| cmd.arg(format!("--target={}", sess.target.llvm_target)); |
There was a problem hiding this comment.
Is this condition sufficient for the general case of adding --target when cross-compiling? The linker flavor doesn't contain any information about the target triple (unless I'm missing something), so I don't see how this can reliably detect cross-compiling.
There was a problem hiding this comment.
From what I know --target only affects which linker is looked up (ld.lld, or ld64.lld, etc) if clang is used for linking only.
So if the required flavor is the same as host, then clang without --target will already use the right linker.
On the other hand, there are some issues with crt object versions on macOS if --target is specified (fixed by #101792), maybe target takes priority over some relevant environment variables.
So always passing --target leads to issues.
We'll likely need to tweak this logic based on reported issues.
There was a problem hiding this comment.
For lld this may not matter, since lld is also a cross-linker.
My main concern is if gnu ld is used (linker flavor (Cc::Clang, Lld::No) for both host and target), then --target is required for clang to select e.g. aarch64-linux-gnu-ld instead of just ld.
That's a very interesting twist! That could unify a bunch of disparate code-paths and flags/options (and should also work for self-contained linking features), as well as simplify use with a single entry-point, It feels like it should simplify our current setup, and this PR's flavors of course, but were you also imagining using this for the generic flavors? (I don't think so?) |
Well, |
|
Sure, like I described in my earlier comment. Overall, I personally find this direction promising. A bunch of the support code could also be based on renaming the self-contained linking components to "linker features" (w/ a prefix), adding |
Could you elaborate, I don't understand this part. |
|
I meant you could imagine the possible linker features as a generalization of self-contained linking components. And get an approximation of how that'd look like by renaming |
|
I see, I suspected that, but it seems far enough from linker features, if they are understood as something representing linker interface. |
|
Ok, then let’s not merge linker-features and self-contained linking components. |
|
☔ The latest upstream changes (presumably #120468) made this pull request unmergeable. Please resolve the merge conflicts. |
|
To summarize, the use-cases were:
The other arguments for linker features:
In a future where So now we can also wonder if What do you think @petrochenkov? (It seems we already had shared ideas about the design, so I'm not sure if this was really waiting-on-review, @rustbot author) |
rustbot
added
S-waiting-on-author
I just r+'d the LLVM bitcode linker today - #117458, it should land soon. |
+ |
|
Sorry for the delay.
My point in general was that we should not make the "single principal flavor for a target" an axiom. "Windows GNU LLD" is a thin wrapper over "Windows MSVC LLD" that only translates command line options from one format to another. |
|
All the remaining part makes sense to me, except I'm still skeptical about moving the "self-contained" bit to the linker flavor/features. ("Self-contained" and other features will even be processed in different parts of the compiler because the latter is about linker command line interface, and the former is about directories included into |
|
I'll close this PR because it's only used for discussion, and the changes themselves are going to be implemented in a different way. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 13, 2024
Linker flavors next steps: linker features This is my understanding of the first step towards `@petrochenkov's` vision for the future of linker flavors, described in rust-lang#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 rust-lang#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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO: detailed description.
Features that may affect the naming scheme for linker flavors (at least those I'm aware of):
--target, something that is not supported by gcc-style compilers.LinkerFlavor::Gnufor aLinkerFlavor::Msvc-based target - rustc_target: support Unix-flavor linkers for UEFI #110869. Even if it requires some special logic, that would still be less heavy weight than providing whole separate targets with their own CI and distributed binaries.r? @lqd
cc @jschwe