[RFC] Target Modifiers

[Darksonn]

• We introduce the concept of "target modifier". A target modifier is a flag where it may be unsound if you link together two compilation units that disagree on the flag.
• We fail the build if rustc can see two Rust compilation units that do not agree on the exact set of target modifier flags.
• There are already several existing flags that could fall into this category. There are also hypothetical new flags that do.
• The error can be silenced using the -Cunsafe-allow-abi-mismatch escape hatch.
• Not having a stable way to build stdlib crates does not block stabilization of target modifiers.
• As a future extension we may be able to relax the rules to allow some specific kinds of mismatches.
• This RFC does not stabilize any target modifiers. That should happen in follow-up MCPs/FCPs/RFCs/etc.

Rendered

[Amanieu]

Actually RISC-V is one of the few targets that handles this properly. The target features control what instructions codegen is allowed to use while the ABI is specified separately in the target spec. This means that you can enable FP instructions while keeping a soft-float ABI. Notice how the E target uses a separate ABI.

It's the other targets that often have target features which change the ABI.

[workingjubilee]

The change that does that was merged in the time between writing and posting this RFC.

[workingjubilee]

I know, because I'm the one that signed off on that change being merged.

And rv32e target HAS to have its ilp32e ABI specified, because that's the only avenue for how you inform LLVM that you want to do codegen for rv32e.

[rmsyn]

It's the other targets that often have target features which change the ABI.

So, that's what I thought, too. That RISC-V extensions were additive.

How do these "subtractive" extension play with target triples that define a base set of extensions, e.g. is this sound

rustc --target riscv32imac-unknown-none-elf --target-features e

Would it make sense to add a bare set of riscv[32|64]-unknown-<platform>-<libc> target triples (in addition to current triples), and allow user to explicitly add all of the extensions they want to use?

[rmsyn]

And rv32e target HAS to have its ilp32e ABI specified, because that's the only avenue for how you inform LLVM that you want to do codegen for rv32e.

It looks like the RV32E extension is incompatible with the D extension because of the ILP32E calling convention (from riscv-elf-psabi-doc):

The ILP32E calling convention is not compatible with ISAs that have registers that require load and store alignments of more than 32 bits.

In particular, this calling convention must not be used with the D ISA extension.

[rmsyn]

I don't know the riscv situation so well. Happy to reword if the paragraph is not accurate.

So, it looks accurate to me. Maybe the discussion about target triples is related, but somewhat tangential to this RFC. It may be something that requires its own RFC: when to introduce a new set of target triples for RISC-V targets.

It's a situation that should be fairly rare, especially as the ISA matures even more. However, there are no target triples that specify a riscv32e-unknown-none-elf, for example.

I'm open to working on such an RFC, and linking back to this RFC for the discussion of incompatible RISC-V extensions.

The more I think about it, the more adding a base set of bare RISC-V targets makes sense (following from the riscv64-linux-android target):

• riscv32-unknown-none-elf
• riscv64-unknown-none-elf
• ...

I would think that an attempt to use --target-features e on an I target should be rejected.

I agree, that would make sense to me as well. Unless LLVM is doing some magic to resolve when both E and I extensions are requested, only E is used?

Even then, it would be nice for rustc to reject the pairing using the techniques discussed in this RFC.

More rejections for --target-features e:

• riscv32gc-*
• riscv64gc-*
• any other targets that implicitly include the D and/or I extension
• --target-features d
• --target-features i

[workingjubilee]

LLVM rejects it.

  if (HasI && HasE)
    return getIncompatibleError("i", "e");

[workingjubilee]

@rmsyn

It's a situation that should be fairly rare, especially as the ISA matures even more. However, there are no target triples that specify a riscv32e-unknown-none-elf, for example.

Are you sure about that? https://github.com/rust-lang/rust/blob/a06b7cbe21967a86050fa92dab843c8afda1c28e/compiler/rustc_target/src/spec/targets/riscv32e_unknown_none_elf.rs

[rmsyn]

@rmsyn

It's a situation that should be fairly rare, especially as the ISA matures even more. However, there are no target triples that specify a riscv32e-unknown-none-elf, for example.

Are you sure about that? https://github.com/rust-lang/rust/blob/a06b7cbe21967a86050fa92dab843c8afda1c28e/compiler/rustc_target/src/spec/targets/riscv32e_unknown_none_elf.rs

Ah, I guess not... I was looking at the output of rustc --print target-list on v1.82.0 stable. Thanks for the link.

[RalfJung]

This means that you can enable FP instructions while keeping a soft-float ABI.

That's actually fine on most targets, e.g. on x86 if you set the soft-float target feature then you can enable x87 and SSE and whatnot and keep using the soft-float ABI. Only aarch64 screws this up (from the targets I checked so far).

The interesting question is what happens on RISC-V when I disable the FP instructions on a target that usually uses the hardfloat ABI. Sadly, what LLVM usually does in that case is silently fall back to the softfloat ABI. But I haven't checked for RISC-V.

[RalfJung]

Shouldn't we also require the flag when building D?
In particular, it seems good to require the flag when building the final artifact, to ensure there is some proximity to the place where "everything comes together". When building C, I don't know yet what else will be linked into this binary, so the flag being sound may depend on what happens later when D is built.

[Darksonn]

Maybe? If we consider the scenario I mention in the RFC:

However, real-world scenarios where mismatching a target modifier is necessary are quite uncommon. The only case I'm aware of is the runtime for a sanitizer. For example, when ASAN (address sanitizer) detects a bug, it calls into a special ASAN-failure-handler function. The function for handling ASAN-failures should not be sanitized.

then I think the behavior I proposed is the right one. The CU containing the sanitizer runtime is clearly the odd-one-out.

[RalfJung]

Okay so there are situations where the crate C can locally say "yes I am sure that this is fine and the rest of the code doesn't have to worry about this".

But the RFC makes this a blanket recommendation for all cases, which I don't think is warranted.

[Noratrieb]

If the only use case we can think of is an ASAN runtime, it makes me question if we want this flag at all. Sanitizer runtimes are compiler-internal and can use all kinds of funny ways to subvert these checks. If we don't have a use case for ignoring modifier mismatches in user code, then I think we shouldn't add it.
I'd say that we should ship this feature without a stable way to disable it. If users then come to us with good use cases for allowing mismatches we can add it later.

[workingjubilee]

There are other use-cases. Mostly, they are insanely subtle, like the combinations that are relevant to aarch64 with shadow-stack and/or fixed-x18 and a few other features, so they're somewhat inappropriate to discuss in the RFC (aside from "no, truly, they are insanely subtle", perhaps).

[Noratrieb]

I see, in that case it makes sense to have it. It would be useful to have a few of them listed out to make sure that the "-Cunsafe-allow-abi-mismatch ignores the current crate from mismatch checks" behavior is indeed the right one. But I'm inclined to believe that it is.

[workingjubilee]

Yeah, in my opinion I almost want to say that we should broadly avoid canonizing certain combinations of ABI flags, in the very likely event that they are incorrect in some case we have not yet foreseen.

[RalfJung]

I'm a bit confused by this example... so this is linking two Rust crates but not with rustc? When does that happen?

[Darksonn]

Imagine the Linux kernel where you have two built-in drivers. Neither driver depends on the other driver. Each driver is compiled into an object file by rustc. When performing the final link of all of the object files that make up the kernel, that is done using the usual C linker and not by rustc.

[tgross35]

Will target modifier information be available at that point? Wondering if the system linkers could theoretically learn to detect this in some form.

[Darksonn]

They could. In fact, .note.gnu.property is an example of exactly that.

[elichai]

I just want to say thank you for incorporating great errors and not unparsable linker errors ❤️

[tgross35]

Will target modifier information be available at that point? Wondering if the system linkers could theoretically learn to detect this in some form.

[tgross35]

Is there an issue listing flags that we will want to make target modifiers? Might be good to collect future work in one place if that doesn't exist yet.

[Darksonn]

There are some individual issues (e.g., rust#129893 and rust#131058) for flags that are unsound due to ABI issues, but I don't think we have a combined issue tracking all of them.

[RalfJung]

There is rust-lang/rust#131837

[davidtwco]

I think this makes a lot of sense, it's had some time to attract feedback and all of the feedback seems to have been addressed, so I'll start an FCP on it.

@rustbot fcp merge

[davidtwco]

It turns out you need to use the correct bot to do that.

@rfcbot fcp merge

[rfcbot]

Team member @davidtwco has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Mark-Simulacrum
• @Noratrieb
• @SparrowLii
• @cjgillot
• @compiler-errors
• @davidtwco
• @estebank
• @matthewjasper
• @nagisa
• @oli-obk
• @petrochenkov
• @spastorino
• @wesleywiser

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[SparrowLii]

It dose makes much sense! Just some small questions

I don't see description for interop, so I think this rfc only deals with compilation between rust crates? There are a lot of projects involving interop (mostly Rust-C/C++) in real scenarios, hopefully there will be similar solutions in the future.

[Noratrieb]

Suggested change

	machine code. Some flags can be turned on and off for each CU (compilation
	unit) in your project separately, and other flags must be applied to the entire
	application as a whole. The typical reason for flags to be in the latter
	machine code. Some flags can be turned on and off for each crate (which is a compilation unit)
	in your project separately, and other flags must be applied to the entire
	application as a whole. The typical reason for flags to be in the latter

nit: we have a fancy word for compilation units already :) (then change all later mentions of "CU" to "crate")

[Darksonn]

I found this change awkward in "Problems with mixing non-target-modifiers" since it speaks in general terms and would also apply to C or C++ compilation units.

[Noratrieb]

If the only use case we can think of is an ASAN runtime, it makes me question if we want this flag at all. Sanitizer runtimes are compiler-internal and can use all kinds of funny ways to subvert these checks. If we don't have a use case for ignoring modifier mismatches in user code, then I think we shouldn't add it.
I'd say that we should ship this feature without a stable way to disable it. If users then come to us with good use cases for allowing mismatches we can add it later.