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

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

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

petrochenkov
Copy link
Contributor

I want to do some refactorings in rustc_target - merge lld_flavor and linker_is_gnu into linker_flavor, support combination gcc+lld (#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).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 14, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2022
@rust-log-analyzer

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor Author

ping @estebank
Could you reassign this PR if you don't plan to review it?
It's blocking some other work, so I wouldn't want to wait until triage team reassigns it in a couple of months instead.

@petrochenkov
Copy link
Contributor Author

r? rust-lang/compiler

@rust-highfive rust-highfive assigned jackh726 and unassigned estebank Aug 29, 2022
@petrochenkov
Copy link
Contributor Author

cc @lqd as well, maybe you have time to review this.

@lqd
Copy link
Member

lqd commented Aug 30, 2022

I do have some time and will take it from esteban/jack

r? @lqd

@rust-highfive rust-highfive assigned lqd and unassigned jackh726 Aug 30, 2022
@@ -1159,6 +1201,7 @@ pub struct TargetOptions {
/// Default linker flavor used if `-C linker-flavor` or `-C linker` are not passed
/// on the command line. Defaults to `LinkerFlavor::Gcc`.
pub linker_flavor: LinkerFlavor,
linker_flavor_json: LinkerFlavorCli,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume all the _json fields are named with this suffix as they refer to the json target specs ? Maybe it would be nice if they were named _cli to match the CLI type ?

The naming is not super important, especially as I believe these fields are going to be temporary and refactored away soon enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these fields are what is parsed directly from json specs.
They use the same LinkerFlavorCli structure as command line options for simplicity, but they are semantically json.

@lqd
Copy link
Member

lqd commented Aug 30, 2022

I want to do some refactorings in rustc_target - merge lld_flavor and linker_is_gnu into linker_flavor, support combination gcc+lld (#96827).

If I understand correctly, some of the end goal was described in this comment of yours, and this PR represents a "temporary" state of things:

  • to help the other refactorings, by e.g. not requiring a complete huge mass edit of all target specs at each step or having an impact on the currently stable values
  • and help reach a cleaned up state that coherently represents a vision of all the requirements for target specs, linker clusters, etc in the future

If you have some specific tasks in mind, I would like to help you with the refactorings.

In particular, this PR's manual translations between the 2 representations are going to be error-prone (subtly forgetting an update_{to,from}_cli call could lead to the 2 maps being out of sync) so I expect the end goal of these refactorings is not to have these duplicate _json fields to avoid that possibility. Once cleaned up, we'd be able to only convert between the cli/json interface at their entry points (in the session arg parsing, and the json serialization/deserialization) and use the internal representation everywhere, without the translation layer.

Is the above roughly correct and what you had in mind ?

If that's the case, then r=me: this improves things towards the goal, will unblock all the other work, and we'll clean everything up and avoid the footguns soon.


I'm not familiar with the tier 3 target x86_64-unknown-l4re-uclibc and its l4-bender linker, nor do I have the ability to test it. As you mentioned in the PR adding it, the linker flavor didn't seem necessary so it's nice to see it cleaned up. (But if something's wrong here wrt its peculiarities about spaces and arg ordering, I expect the target maintainers to notice, maybe).

@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 Aug 30, 2022
@petrochenkov
Copy link
Contributor Author

@lqd

If I understand correctly, some of the end goal was described in #96827 (comment), and this PR represents a "temporary" state of things:

Yes, I'd like to have the list of options in the end of that comment to become user-visible and stable in the end.
I'd expect the "temporary" state of things to last until those options have some consensus and are ready stabilization, then we'll be able to make a single big break for the json specs - use the new flavors and remove various other accumulated compatibility stuff.

If you have some specific tasks in mind, I would like to help you with the refactorings.

petrochenkov@53eca42 is pretty much ready modulo some cleanup.
#96827 is needed to actually implement the logic for *-lld-cc flavors.
After that the only thing is to make the new flavor-strings user visible and pass them through some FCP and stabilization process.

this PR's manual translations between the 2 representations are going to be error-prone (subtly forgetting an update_{to,from}_cli call could lead to the 2 maps being out of sync)

In theory, 2 "syncs" after Target construction are enough - one in fn load_builtin (json fields are brought up to date) and one in fn from_json (non-json fields are brought up to date).

This PR keeps these 2 syncs, but delays one of them until fn to_json as a performance optimization, to avoid syncing on a common path (fn load_builtin).
I.e. the Target is intentionally left out-of-sync most of the time, until it actually needs to be in-sync (which happens very rarely, when --print target-spec-json is used).
This is important when all target specs are loaded, e.g. for --check-cfg (#95202).
(The third sync is purely to satisfy the assert_eq comparison in the unit test.)
So I think it's fine as is and doesn't need a further cleanup.

The alternative to extra *_json fields is to have two slightly different Target structures, I tried to start something like that but it was so much more cumbersome that I abandoned the idea very soon.

It may not be obvious from this PR, but the problem is that json fields cannot be converted on the fly individually, they need to be combined (e.g. linker_flavor_json + lld_flavor_json + linker_is_gnu_json -> linker_flavor). So the whole json needs to be parsed first, and then post-processed.
If not this case we wouldn't need this infra at all.

I'm not familiar with the tier 3 target x86_64-unknown-l4re-uclibc and its l4-bender linker

At the end of #85967 it pretty much turned into ld with a few peculiarities (no more than solaris or mingw). I just didn't want to bother the author to rewrite everything once again.

@petrochenkov
Copy link
Contributor Author

@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 Aug 31, 2022
@lqd
Copy link
Member

lqd commented Aug 31, 2022

The alternative to extra *_json fields is to have two slightly different Target structures, I tried to start something like that but it was so much more cumbersome that I abandoned the idea very soon.

Yeah this is what I was imagining as well, having separate Targets for the 2 different use-cases but hadn't tried it to see if it was viable. (There also was the possibility for the compatibility layer to be within the fields themselves and contain both maps, but it would probably be cumbersome as well.)

Ok I'm convinced, let's land this and we can keep iterating.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2022

📌 Commit 667eb18 has been approved by lqd

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 Aug 31, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 1, 2022
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).
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
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).
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 1, 2022
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).
@Dylan-DPC
Copy link
Member

@bors r-

this has conflicts with a recently merged PR

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 1, 2022
@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor Author

@bors r=lqd

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit a0e21ff has been approved by lqd

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 Sep 1, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2022
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#97739 (Uplift the `let_underscore` lints from clippy into rustc.)
 - rust-lang#99583 (Add additional methods to the Demand type)
 - rust-lang#100147 (optimization of access level table construction)
 - rust-lang#100552 (rustc_target: Add a compatibility layer to separate internal and user-facing linker flavors)
 - rust-lang#100827 (Simplify MIR opt tests)
 - rust-lang#101166 (Generate error index with mdbook instead of raw HTML pages)
 - rust-lang#101294 (Fix rust-lang#100844 rebase accident)
 - rust-lang#101298 (rustdoc: remove unused CSS `#main-content > .since`)
 - rust-lang#101304 (Add autolabels for `A-query-system`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit edf79cb into rust-lang:master Sep 2, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 2, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 7, 2022
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants