No more target.target #77943

est31 · 2020-10-14T17:49:27Z

Two main changes of this PR:

Turn target_pointer_width into an integer and rename to pointer_width.
The compiler only allowed three valid values for the width anyways.
An integer is more natural for this value, and saves a few allocations
and copies.
Remove the rustc_session::config::Config wrapper and replace it with
its inner member Target. Aka. no more target.target. This makes life so
much easier, but it also causes a ton of downstream breakage.

Some changes of this PR were done using tooling. These tooling-made changes
were isolated to their own commits to make review easier.
It's best to review the PR commit-by-commit.

Miri PR: rust-lang/miri#1583

I request p=10 bors priority because of the breakage.

rust-highfive · 2020-10-14T17:49:31Z

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2020-10-14T21:23:50Z

r? @petrochenkov
I actually planned to do this as a follow up to #77729.

petrochenkov · 2020-10-14T22:52:23Z

@est31
Could you simultaneously rename target_pointer_width to just pointer_width to address the "Avoid tautologies..." item from #77729 as well?
(In json it still needs to be represented with a string "target-pointer-width" though, for backward compatibility.)

tesuji · 2020-10-15T02:28:36Z

compiler/rustc_codegen_ssa/src/back/write.rs

@@ -307,7 +307,7 @@ pub struct CodegenContext<B: WriteBackendMethods> {
    pub allocator_module_config: Arc<ModuleConfig>,
    pub tm_factory: TargetMachineFactory<B>,
    pub msvc_imps_needed: bool,
-    pub target_pointer_width: String,
+    pub target_pointer_width: u32,


Since we only allow/support pointer size of 16/32/64. I prefer to encode this
to an enum instead to avoid mistaken typo.

I've considered an enum, and think that #77604 is a good idea, but in this instance a number feels more natural to me.

Enums have to be imported, which causes overhead. Most of the present enums in the spec module are on the optional TargetOptions structure, so only have to imported by the few targets that specify those options. @petrochenkov 's PR moves the only enum currently imported by all target files LinkerFlavor into TargetOptions.

Ideally one would have PointerWidth::16 or something, but identifiers can't start width digits, so it'd be PointerWidth::W16 or something, which isn't pleasant IMO.

Many places in the compiler assume the values to be integers already, some also do comparisons with other widths specified.

That being said, there are advantages of enums, main one is that one can build exhaustive matches throughout the compiler, and that it's immediately clear that only 16,32,64 bits are supported.

est31 · 2020-10-15T08:08:24Z

I actually planned to do this as a follow up to #77729.

Didn't know about your PR, sorry. I've searched the PRs for target.target and if github PR search were any good, it would have found your PR, but it didn't :(. Glad that you're ok with me doing this.

Have you done your PR with automatic tools, or more manually? If the latter is the case, I'd suggest to merge this PR after yours, because it's quite easy for me to rebase thanks to having automated the large diff causing changes.

Could you simultaneously rename target_pointer_width to just pointer_width to address the "Avoid tautologies..." item from #77729 as well?

That's a good point. Will do it. 👍

petrochenkov · 2020-10-15T09:24:22Z

Rebasing shouldn't be a problem here, we can land this PR first.

Also change target_pointer_width to pointer_width. Preparation for a subsequent type change of target_pointer_width to an integer together with a rename to pointer_width. On its own, this commit breaks the build. I don't like making build-breaking commits, but in this instance I believe that it makes review easier, as the "real" changes of this PR can be seen much more easily. Result of running: find compiler/rustc_target/src/spec/ -type f -exec sed -i -e 's/target_pointer_width: "$.*$"\..*,/pointer_width: \1,/g' {} \;

Rename target_pointer_width to pointer_width because it is already member of the Target struct. The compiler supports only three valid values for target_pointer_width: 16, 32, 64. Thus it can safely be turned into an int. This means less allocations and clones as well as easier handling of the type.

…inter_width Preparation for a subsequent change that replaces rustc_target::config::Config with its wrapped Target. On its own, this commit breaks the build. I don't like making build-breaking commits, but in this instance I believe that it makes review easier, as the "real" changes of this PR can be seen much more easily. Result of running: find compiler/ -type f -exec sed -i -e 's/target\.target$[)\.,; ]$/target\1/g' {} \; find compiler/ -type f -exec sed -i -e 's/target\.target$/target/g' {} \; find compiler/ -type f -exec sed -i -e 's/target.ptr_width/target.pointer_width/g' {} \; ./x.py fmt

The wrapper type led to tons of target.target across the compiler. Its ptr_width field isn't required any more, as target_pointer_width is already present in parsed form.

est31 · 2020-10-15T11:16:34Z

r? @petrochenkov

petrochenkov · 2020-10-15T12:43:13Z

@bors r+ p=10 (conflict-prone)

bors · 2020-10-15T12:43:15Z

📌 Commit d683e3a has been approved by petrochenkov

bors · 2020-10-15T16:50:11Z

⌛ Testing commit d683e3a with merge b5c9e24...

bors · 2020-10-15T19:03:21Z

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing b5c9e24 to master...

rust-highfive · 2020-10-15T19:04:33Z

📣 Toolstate changed by #77943!

Tested on commit b5c9e24.
Direct link to PR: #77943

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

@oli-obk

Tested on commit rust-lang/rust@b5c9e24. Direct link to PR: <rust-lang/rust#77943> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

Replace target.target with target Fix fallout caused by rustc PR: rust-lang/rust#77943 Fixes rust-lang/rust#77988

…crum rustc_target: Move some target options from `Target` to `TargetOptions` The only reason for `Target` to `TargetOptions` to be separate structures is that options in `TargetOptions` have reasonable defaults and options in `Target` don't. (Otherwise all the options logically belong to a single `Target` struct.) This PR moves a number of options with reasonable defaults from `Target` to `TargetOptions`, so they no longer needs to be specified explicitly for majority of the targets. The move also allows to inherit the options from `rustc_target/src/spec/*_base.rs` files in a nicer way. I didn't change any specific option values here. The moved options are `target_c_int_width` (defaults to `"32"`), `target_endian` (defaults to `"little"`), `target_os` (defaults to `"none"`), `target_env` (defaults to `""`), `target_vendor` (defaults to `"unknown"`) and `linker_flavor` (defaults to `LinkerFlavor::Gcc`). Next steps (in later PRs): - Find a way to merge `TargetOptions` into `Target` - If not, always access `TargetOptions` fields through `Deref` making it a part of `Target` at least logically (`session.target.target.options.foo` -> `session.target.target.foo`) - ~Eliminate `session::config::Config` and use `Target` instead (`session.target.target.foo` -> `session.target.foo`)~ Done in rust-lang#77943. - Avoid tautologies in option names (`target.target_os` -> `target.os`) - Resolve _ rust-lang#77730 (rustc_target: The differences between `target_os = "none"` and `target_os = "unknown"`, and `target_vendor = "unknown"` and `target_vendor = ""` are unclear) noticed during implementation of this PR.

rust-highfive assigned davidtwco Oct 14, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2020

est31 mentioned this pull request Oct 14, 2020

Replace target.target with target rust-lang/miri#1583

Merged

est31 force-pushed the target_refactor branch 2 times, most recently from c4dcf4b to 45d6190 Compare October 14, 2020 19:35

rust-highfive assigned petrochenkov and unassigned davidtwco Oct 14, 2020

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 Oct 14, 2020

tesuji reviewed Oct 15, 2020

View reviewed changes

jyn514 added A-target-specs Area: Compile-target specifications C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 15, 2020

est31 force-pushed the target_refactor branch from 45d6190 to 60ba8e7 Compare October 15, 2020 10:00

est31 added 4 commits October 15, 2020 12:01

Remove rustc_session::config::Config

d683e3a

The wrapper type led to tons of target.target across the compiler. Its ptr_width field isn't required any more, as target_pointer_width is already present in parsed form.

est31 force-pushed the target_refactor branch from 60ba8e7 to d683e3a Compare October 15, 2020 10:02

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 Oct 15, 2020

bors mentioned this pull request Oct 15, 2020

mangling: mangle impl params w/ v0 scheme #75675

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2020

bors merged commit b5c9e24 into rust-lang:master Oct 15, 2020

rustbot added this to the 1.49.0 milestone Oct 15, 2020

rust-highfive mentioned this pull request Oct 15, 2020

miri no longer builds after rust-lang/rust#77943 #77988

Closed

This was referenced Oct 15, 2020

Prefer enum Endian instead of arbitrary String in rustc_target::Target #77604

Closed

rustc_target: Move some target options from Target to TargetOptions #77729

Merged

Change built-in kernel targets to be os = none throughout #77916

Merged

bors added a commit to rust-lang/miri that referenced this pull request Oct 15, 2020

Auto merge of #1583 - est31:target_refactor, r=RalfJung

1b3a27c

Replace target.target with target Fix fallout caused by rustc PR: rust-lang/rust#77943 Fixes rust-lang/rust#77988

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No more target.target #77943

No more target.target #77943

est31 commented Oct 14, 2020 •

edited

Loading

rust-highfive commented Oct 14, 2020

petrochenkov commented Oct 14, 2020

petrochenkov commented Oct 14, 2020

tesuji Oct 15, 2020

est31 Oct 15, 2020

est31 commented Oct 15, 2020

petrochenkov commented Oct 15, 2020

est31 commented Oct 15, 2020

petrochenkov commented Oct 15, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

rust-highfive commented Oct 15, 2020

No more target.target #77943

No more target.target #77943

Conversation

est31 commented Oct 14, 2020 • edited Loading

rust-highfive commented Oct 14, 2020

petrochenkov commented Oct 14, 2020

petrochenkov commented Oct 14, 2020

tesuji Oct 15, 2020

Choose a reason for hiding this comment

est31 Oct 15, 2020

Choose a reason for hiding this comment

est31 commented Oct 15, 2020

petrochenkov commented Oct 15, 2020

est31 commented Oct 15, 2020

petrochenkov commented Oct 15, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

rust-highfive commented Oct 15, 2020

est31 commented Oct 14, 2020 •

edited

Loading