rustc_target: introduce Vendor, Abi, Env, Os

[tamird]

Improve type safety by using an enum rather than strings.

I'm not really sure this is better since only a few vendors have special semantics. r? @nnethercote

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

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

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

FWIW for a related change recently we had quite a bit of discussion: rust-lang/compiler-team#926.

[nnethercote]

I have a bunch of nits but this generally looks good. Linker is another candidate for enum-ification, if you aren't yet bored of this stuff :)

View changes since this review

[nnethercote]

Any idea what "win7" is? Makes me think of Windows 7, which is a weird name for a vendor.

[tamird]

Dates back to #118150 which explains:

I went with naming the target x86_64-win7-windows-msvc, inserting the win7 in the vendor field (usually set to to pc). This is done to avoid ecosystem churn, as quite a few crates have cfg(target_os = "windows") or cfg(target_env = "msvc"), but nearly no cfg(target_vendor = "pc"). Since my goal is to be able to seamlessly swap to the win7 target, I figured it'd be easier this way.

[nnethercote]

Ugh, ok. Thanks for the info!

[nnethercote]

FWIW for a related change recently we had quite a bit of discussion: rust-lang/compiler-team#926.

IMO these changes are so similar to the change in that MCP that additional discussion isn't needed.

[tamird]

Addressed all but one comment and left things unsquashed to (somewhat) ease review. I do plan to squash once you're happy with it.

View changes since this review

[tamird]

Dates back to #118150 which explains:

I went with naming the target x86_64-win7-windows-msvc, inserting the win7 in the vendor field (usually set to to pc). This is done to avoid ecosystem churn, as quite a few crates have cfg(target_os = "windows") or cfg(target_env = "msvc"), but nearly no cfg(target_vendor = "pc"). Since my goal is to be able to seamlessly swap to the win7 target, I figured it'd be easier this way.

[nnethercote]

r=me with the commits squashed, thanks!

@bors delegate=tamird

[bors]

✌️ @tamird, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

[tamird]

range-diff is ~empty, which is what I expected!

@bors r=@nnethercote

[bors]

📌 Commit cd47df7 has been approved by nnethercote

It is now in the queue for this repository.

[madsmtm]

Hmm, I'm semi against making target_vendor an enum - it's not like the other ones, in that it's not supposed to carry any meaning (at least not after rust-lang/lang-team#102).

I'd prefer to keep it as just a String.

[tamird]

That sounds nice, but it is just not the case. There are many places where the vendor is consulted, especially to check if it's Apple.

[bjorn3]

Checking for apple should be done through is_like_darwin.

[tamird]

% git grep -E '[^:] Vendor::' -- ':!compiler/rustc_target/src/spec/targets/' ':!compiler/rustc_target/src/spec/base'
compiler/rustc_codegen_cranelift/src/abi/mod.rs:            (Arch::X86_64, _) | (Arch::AArch64, Vendor::Apple) => match (ty, is_signed) {
compiler/rustc_codegen_cranelift/src/codegen_f16_f128.rs:        if fx.tcx.sess.target.vendor == Vendor::Apple && fx.tcx.sess.target.arch == Arch::X86_64 {
compiler/rustc_codegen_cranelift/src/codegen_f16_f128.rs:        if fx.tcx.sess.target.vendor == Vendor::Apple && fx.tcx.sess.target.arch == Arch::X86_64 {
compiler/rustc_codegen_cranelift/src/codegen_f16_f128.rs:        if fx.tcx.sess.target.vendor == Vendor::Apple && fx.tcx.sess.target.arch == Arch::X86_64 {
compiler/rustc_codegen_ssa/src/back/link.rs:                        && sess.target.vendor != Vendor::Uwp
compiler/rustc_codegen_ssa/src/back/linker.rs:    if matches!(flavor, LinkerFlavor::Msvc(..)) && t.vendor == Vendor::Uwp {
compiler/rustc_codegen_ssa/src/back/linker.rs:    assert!(cmd.get_args().is_empty() || sess.target.vendor == Vendor::Uwp);
compiler/rustc_codegen_ssa/src/common.rs:    target.vendor == Vendor::Pc
compiler/rustc_metadata/src/native_libs.rs:    if sess.target.vendor == Vendor::Fortanix
compiler/rustc_target/src/spec/mod.rs:            self.vendor == Vendor::Apple,
compiler/rustc_target/src/spec/mod.rs:        if let Vendor::Other(s) = &self.vendor {
compiler/rustc_target/src/spec/mod.rs:            || (self.env == Env::Sgx && self.vendor == Vendor::Fortanix)
src/tools/miri/src/machine.rs:                    if target.options.vendor == Vendor::Apple {

It's not just Apple, either.

[bjorn3]

For UWP it can probably check target.abi instead. is_mingw_gnu_toolchain doesn't need to check the vendor at all. It already checks OS and environment. And IMO we should change x86_64-fortanix-unknown-sgx to x86_64-unknown-fortanix-sgx. Fortanix effectively operates as an OS for SGX enclaves. I believe that covers everything currently checking the vendor field.

[tamird]

These sound like good improvements to me. How would you prevent backsliding?

[bjorn3]

The vendor field coule be marked with #[rustc_lint_opt_deny_field_access] if you mark the TargetOptions struct with #[rustc_lint_opt_ty].

[tamird]

x86_64-fortanix-unknown-sgx is a tier 2 platform. Surely we can't just go and change it? Other than that, I have a branch with this change that I'll PR in a moment.

[tamird]

#148760

[tamird]

@bors r=@nnethercote

[bors]

📌 Commit 6b87b87 has been approved by nnethercote

It is now in the queue for this repository.

[bjorn3]

This should be placed under Other IMO. Matching on Vendor::Unknown never makes sense under any circumstance. It is a catch-all for when there is no known vendor value, which is also what the Other variant is for.

[tamird]

Please see prior discussion here

[madsmtm]

I'm gonna unmark this as accepted, since I'd like to see the Vendor changes reverted, and that we go with the plan discussed in #148531 (comment) instead (preferably making those changes in a separate PR).
@bors r-

(Do correct me if this is bad form).

We should bump the priority next time it's scheduled for merging, to avoid further merge conflicts.

[nnethercote]

@tamird: looks like you are planning to merge #148760 before this, and then adjust this accordingly? I give you a gold star for flexibility. It's annoying to do a nice refactoring cleanup that follows the contours of the existing code and then be told that the existing code shouldn't exist. ⭐

[tamird]

@tamird: looks like you are planning to merge #148760 before this, and then adjust this accordingly? I give you a gold star for flexibility. It's annoying to do a nice refactoring cleanup that follows the contours of the existing code and then be told that the existing code shouldn't exist. ⭐

Annoying is a crucial part of doing OSS :)

[madsmtm]

Yeah, I didn't mean anything rude by it, I do appreciate the work you're doing here!

[bors]

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