Add s390x-unknown-none-softfloat with RustcAbi::Softfloat

[fneddy]

followup on #150766

add an s390x-unknown-none-softfloat target to use for kernel compilation, as the Linux kernel does not wish to pay the overhead of saving float registers by default on kernel switch. this target's extern "C" ABI is unspecified, so it is unstable and subject to change between versions, just like the Linux intrakernel ABI and extern "Rust" ABIs are unstable.

enforce target feature incompatibility by adding RustcAbi::Softfloat. this is itself just a rename of RustcAbi::X86Softfloat, accepting both "x86-softfloat" and "softfloat" as valid strings in the target.json format. the target-features of "soft-float" and "vector" are incompatible for s390x, so issue a compatibility warning if they are combined.

[tgross35]

Cc @RalfJung who is the expert in this bit of code.

to use the soft-float feature one need to also set the s390x-softfloat flag in the .rustc_abi target field. This will automatically enable the LLVM +soft-float feature flag. On normal compilation with the default ABI, soft-float is still marked incompatible! So the ABI cannot be be broken by accident.

There aren't any tests indicating how this is meant to be used; do you mean that attempting to use +soft-float is rejected via target properties? I.e., what happens when a binary built with +soft-float gets linked against the normal s390x-unknown-none libcore?

The ABI change can be covered with a simple test in tests/assembly-llvm/, using //@ add-minicore so it can build on all platforms. The test indicating that we reject mixing crates built with and without +soft-float can probably live in tests/ui/target_modifiers (one crate will live in auxiliary/, the other lives in that directory and gets the ABI-changing flags).

[workingjubilee]

The way the code works allows us to take a simpler route for this case:

• Add a target that is s390x-unknown-none-softfloat (assuming that name is amenable to the relevant maints who are effectively going to be asked to maintain this target) cc @cuviper @uweigand again
• Add a warning for trying to set the +soft-float or -soft-float feature at all through the CLI by adding it as a feature but with Stability::Forbidden:

  rust/compiler/rustc_target/src/target_features.rs

  Lines 27 to 31 in 503745e

  	/// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be
  	/// set in the target spec. It is never set in `cfg(target_feature)`. Used in
  	/// particular for features are actually ABI configuration flags (not all targets are as nice as
  	/// RISC-V and have an explicit way to set the ABI separate from target features).
  	Forbidden { reason: &'static str },

• Have the s390x-unknown-none-softfloat target pass "+soft-float" in the list of features enabled as part of its target definition, because the relevant field in the target spec is speaking directly to what LLVM features are enabled, last I checked.

Then we do not need to have "agreement" on the soft-float flag between target-features in FeatureConstraints, because it becomes "use the target or bust".

[workingjubilee]

The above description of how it is easier to solve this by avoiding the question of target-features being set at all is why we have nudged towards "a new target":

@fneddy @uweigand To bring you up to speed on a detail that has become important to clarify, and I think may have been missed: Rust target features are not LLVM target features. This is not just because Cranelift and GCC are backends in the process of being implemented. It is mandated by the language itself.

Long ago, in Rust RFC 2045, Rust decided to allow #[target_feature(enable = "...")], a syntax which adds a target feature to a function. It also decided to allow cfg(target_feature = "..."), which works like you would expect. "..." can be any old string you like. In those glorious old times, this would go straight-through to LLVM.

However, because these attributes live in Rust source code, they are subject to rustc's stability policies around the language, or at least there is an expectation that they are. People are not amused when you tell them that their source code doesn't build because LLVM changed how they support that feature! Unfortunately, the list of LLVM features changes surprisingly often. This story is well-told by the mapping of Rust features to LLVM features in rustc_codegen_llvm.

And that is on top of the realization that many target features are nonsensical to set on a per-function basis, despite being implemented as settable in that way by LLVM!

Since that realization, the compiler and language teams have more-or-less been trying to walk that back as much as possible. I'm approximating. This is the other big reason for my and Ralf's suggestion of "new target" as the way to "configure" this, instead of requiring agreement: There's no reason to reflect the way C compilers configure something like this when we already have to go so far in a different direction.

The main reason to introduce target modifiers for a target is when you want to support many subtle variations, especially along more than one axis. The many-splendored world of ABI-incompatible RISCV variants, for example. Here, we only have heard calls for two de facto targets.

[RalfJung]

Add a warning for trying to set the +soft-float or -soft-float feature at all through the CLI by adding it as a feature but with Stability::Forbidden:

Note that the target feature already exists, it just needs the message to be changed.

Also I think we can't make it Forbidden when it is used like this for the ABI. (Maybe we should fix that.) But that's fine, the ABI check ensures that it cannot be toggled. However, without a builtin target we won't know if this actually works... so it'd probably be best to add such a target in the same PR.

[RalfJung]

Also I think we can't make it Forbidden when it is used like this for the ABI. (Maybe we should fix that.)

Actually, it seems I misremembered. It can remain Frobidden. The only practical difference between Forbidden and (Un)Stable here is whether the feature can be queried with cfg(target_feature).

[fneddy]

my further agenda so far:

• add a s390x-unknown-none-softfloat target which explicitly enables softfloat.
• add tests to make sure that you cannot enable softfloat with s390x-unknown-linux-gnu or s390x-unknown-linux-musl
• add proper error messages
• add documentation for s390x-unknown-none-softfloat

open question:

• prevent linking +softfloat and -softfloat?
  I just tried it with clang on x86 and s390x and it there is no error or warning emitted by the linker.
  I think the only way to prevent linking would be to define new elf e_flags values and let the linker check on that.

[RalfJung]

prevent linking +softfloat and -softfloat?

Since you can't even set the flag any more, we also don't have to worry about linking I think. Linking things with different target triples is so far out there, IMO we can ignore that case.

[workingjubilee]

Here, we only have heard calls for two de facto targets.

Correction: three. I forgot we also supported musl for this target!

I just tried it with clang on x86 and s390x and it there is no error or warning emitted by the linker.

Yeah, I also think we can rely on "...they're different targets, did you really expect linking them to go well?" People implementing non-Cargo build systems could try to bypass our resistance to that, but they usually try to exercise a mote of sense here and provide a slightly better UX than "directly wielding the three-edged sword of a C compiler", and usually are target-aware in how they operate. We do know the way cargo works simply won't try it. There's still some possibilities we could try to test for locking down, but as far as I know Linux's kbuild is also reluctant to make this conflation, so that covers the two major users of interest.

[workingjubilee]

Mind, if you can easily trick rustc into trying to link incompatible binaries when it drives the linker, we'd want to investigate that further. But we do consider external tool usage... using a linker without going through rustc, supplying a linker script to rustc, or interdicting rustc's attempts to access external tools via replacing the linker with a wrapper script... to be essentially equivalent to unsafe, per our supplemental tools policy which basically says, "We demand very few things for a sane build environment, but if you take those into your own hands, that's on you."

[fneddy]

currently the resulting ABI incompatibility is modeled here:

rust/compiler/rustc_target/src/target_features.rs

Lines 1221 to 1233 in e87834e

	match self.rustc_abi {
	None => {
	// Default hardfloat ABI.
	FeatureConstraints { required: &[], incompatible: &["soft-float"] }
	}
	Some(RustcAbi::S390xSoftFloat) => {
	// Softfloat ABI, requires corresponding target feature.
	// llvm will switch to soft-float ABI just based on this feature.
	FeatureConstraints { required: &["soft-float"], incompatible: &[] }
	}
	Some(r) => {
	panic!("invalid Rust ABI for s390x: {r:?}");
	}

However this is not (yet) treated as a hard failure but just a warning:

rust/compiler/rustc_interface/src/util.rs

Lines 92 to 100 in b765963

	for feature in abi_feature_constraints.required {
	if !sess.unstable_target_features.contains(&Symbol::intern(feature)) {
	sess.dcx().emit_warn(errors::AbiRequiredTargetFeature { feature, enabled: "enabled" });
	}
	}
	for feature in abi_feature_constraints.incompatible {
	if sess.unstable_target_features.contains(&Symbol::intern(feature)) {
	sess.dcx().emit_warn(errors::AbiRequiredTargetFeature { feature, enabled: "disabled" });
	}

rust/compiler/rustc_interface/messages.ftl

Lines 1 to 3 in b765963

	interface_abi_required_feature =
	target feature `{$feature}` must be {$enabled} to ensure that the ABI of the current target can be implemented correctly
	.note = this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

As mentioned, llvm currently will silently switch to the unofficial soft-float ABI if the flag was set. So the emited warning is rather misleading in this case. Neither do we say that we actually use a different ABI, nor do we say that this results in incompatibility.

Should I keep this as is for this case or should I add another check that will create a hard failure?.

[RalfJung]

What do you find misleading about the warning? It says that the ABI of the current target is not implemented correctly when using -Ctarget-feature=+soft-float. Using a different ABI surely means that the actual ABI is not implemented correctly so this sounds right to me. I also find "ABI not implemented correctly" sufficiently scary, but if you want to add a note about how that means this code may fail in mysterious ways when linked with other code for the same target that uses different flags (such as the standard library), we can do that. I'm open to suggestions for how to improve the wording. :)

I don't think we should have a separate check, but we could make the warning a hard error on s390x if the target maintainers are fine with that.

[fneddy]

I just wanted to point out that one could ignore the warning and do nonsense like --target=s390x-unknown-linux-gnu -C target-feature=+soft-float or --target=s390x-unknown-none-softfloat -C target-feature=+vector.

We will definitely need hard errors on -softfloat,+packedstack,+backchain because this combination is not implemented in llvm. But that check needs access to the session therefore currently I think i'll add that to the checks in rustc_codegen_ssa or rustc_codegen_llvm.

[RalfJung]

I just wanted to point out that one could ignore the warning and do nonsense like --target=s390x-unknown-linux-gnu -C target-feature=+soft-float or --target=s390x-unknown-none-softfloat -C target-feature=+vector.

Yeah. I'm aware. I think making the warning a hard error for some targets is a good idea; we just didn't do this globally due to backwards compatibility.

Also, what's wrong with the last example? Is vector incompatible with softfloat? Currently the ABI information encoded in this PR does not seem to prohibit that combination. At least on x86, LLVM will use the soft-float ABI even if you set +avx so that combination is actually fine there.

We will definitely need hard errors on -softfloat,+packedstack,+backchain because this combination is not implemented in llvm. But that check needs access to the session therefore currently I think i'll add that to the checks in rustc_codegen_ssa or rustc_codegen_llvm.

Yeah finding a good place for such an odd target limitation will be tricky. :/ But that's a question for a different PR.

[fneddy]

Also, what's wrong with the last example? Is vector incompatible with softfloat?

on s390x the float and vector registers overlap. So if you enable softfloat you consequently should disable vector. This is present in llvm. I would also add this as incompatible to generate proper errors from rustc.

[RalfJung]

on s390x the float and vector registers overlap. So if you enable softfloat you consequently should disable vector.

I don't see why that's a consequence.
"soft-float" (the part we are mostly concerned with here, anyway) is about the ABI, not about whether the registers are available. It can make perfect sense to have float registers but not use them in the ABI. For instance, the Linux kernel locally enables FPU usage in some places for performance, but to ensure sound linking with the rest of the kernel the ABI should still not use float registers.

[uweigand]

I don't see why that's a consequence. "soft-float" (the part we are mostly concerned with here, anyway) is about the ABI, not about whether the registers are available. It can make perfect sense to have float registers but not use them in the ABI. For instance, the Linux kernel locally enables FPU usage in some places for performance, but to ensure sound linking with the rest of the kernel the ABI should still not use float registers.

Well, on SystemZ the vector feature also changes the ABI - it will cause vector types to be passed in vector registers, not in memory. A combination of soft-float with vector would be a third ABI that doesn't really make sense.

[RalfJung]

We only allow vector types to be passed by-val when vector is enabled, so that ABI difference is not observable.

But, I'm also fine with making vector as "incompatible" with the soft-float ABI, if that's what the target maintainers prefer. We can always reconsider when there is user demand.

[rustbot]

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

[fneddy]

Ok I think I incorporated the points needed. I would appreciate more comments and reviews.

[workingjubilee]

I am perplexed by this.

The Linux kernel toolchain presumably does need to build the target's libcore, no? And they just build the same libcore, last I checked. Are you thinking of liballoc?

In any case, this is... unhelpful. Please address your peers with this section, instead. People who are working on building the Linux kernel with Rust on s390x. It would be better to just link out to Linux kernel documentation if you would prefer that.

[fneddy]

You are absolutely right. I don’t know what I saw that made me think the kernel has its own libcore. I will correct this!

[workingjubilee]

This segment is still waiting on an update, I think, both to correct the libcore detail and also... just... you might as well say something like...

"Code for this target can be built by the usual methods of -Zbuild-std or building a custom toolchain after enabling the target in bootstrap.toml, but you probably want to just use kconfig/kbuild which should take care of this for you."

...At least, if my understanding of what kbuild does is correct, heh.

That way people get an appropriate nudge to do the right thing, instead of just telling them the thing that clearly can be done shouldn't ever need to be done. Because, well, they might be someone updating kbuild.

[fneddy]

ok so this is ready for a next round. through I have to wait for rust-lang/stdarch#2022 to be merged and then i'll rebase.

[workingjubilee]

Cool and good. Found some nits. Will take a final scan after the final rebase.

View changes since this review

[fneddy]

should i squash everything or rewrite everything into logical commit units or leave as is?
I rewrote this into 4 commits

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[fneddy]

@rustbot ready

[workingjubilee]

And with one last note, we're done!

View changes since this review

[workingjubilee]

cool!

mostly-additive change, should be fine to

@bors r+ rollup

[rust-bors]

📌 Commit 83dba5b has been approved by workingjubilee

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.