Emit error when calling/declaring functions with vectors that require missing target feature

[veluca93]

On some architectures, vector types may have a different ABI depending on whether the relevant target features are enabled. (The ABI when the feature is disabled is often not specified, but LLVM implements some de-facto ABI.)

As discussed in rust-lang/lang-team#235, this turns out to very easily lead to unsound code.

This commit makes it a post-monomorphization error to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. This ensures that these functions are always called with a consistent ABI.

See the nomination comment for more discussion.

r? RalfJung

Fixes #116558

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @RalfJung (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[RalfJung]

Please also add some tests so that we can see this check in action.

[RalfJung]

Looks good for the initial draft, let's see what crater says. :)

@bors try

[bors]

⌛ Trying commit e8302b3 with merge 7587ff3...

[bors]

☀️ Try build successful - checks-actions
Build commit: 7587ff3 (7587ff3622fbec0abf6ac551eab5226f22f5d958)

[RalfJung]

@craterbot check

[craterbot]

👌 Experiment pr-127731 created and queued.
🤖 Automatically detected try build 7587ff3
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[traviscross]

We talked about this this the lang-docs call today. It'd be good to look into how this change might be documented in the Reference. It has some bearing on:

• Document the target_feature_11 feature reference#1181
• Spec abi chapter reference#1545

cc @RalfJung @veluca93 @chorman0773

[RalfJung]

One impact it has on the reference/docs is that we can entirely remove this section -- one can no longer cause ABI incompatibility UB using target features. That's a great win in my book. :)

[veluca93]

We talked about this this the lang-docs call today. It'd be good to look into how this change might be documented in the Reference. It has some bearing on:

• Document the target_feature_11 feature reference#1181
• Spec abi chapter reference#1545

cc @RalfJung @veluca93 @chorman0773

Sorry for the delay!

To my understanding, the reference changes for this PR should be minimal. I suspect a paragraph along these lines should be sufficient to describe the behaviour:

Note that, with some ABIs, the specific ISA that a function targets affects the ABI for passing certain types as arguments (or return values) of functions.
For example, on x86_64 with the "C" ABI, enabling avx makes an argument of type __m256 be passed by register instead of by stack.
This behaviour can easily lead to surprises. As such, Rust disallows

• defining functions with arguments/return values that would change ABI if more features were enabled
• calling functions for which some arguments would change ABI if called in a function with more features enabled

Note that neither of these situations can arise when dealing with functions that use the Rust ABI.
This behaviour is similar - but not identical - to the one of the -Wpsabi flag in some C++ compilers.

[RalfJung]

IIRC the ABI for __m256 without AVX isn't even really documented, it's just what GCC/clang happen to do? Basically that type is supposed to only exist when AVX is available, but in Rust for better or worse the type always exists, so now we have to deal with this edge case.

[chorman0773]

It actually is, but how it is is subtle (and somewhat confusing).

From confirmation on the x86_64-psabi mailing list:

• The first SSE eightbyte is passed in the next available xmm parameter register
• The remaining SSEUP eightbytes are passed in the next available portion of that register
• If any register used for passing a parameter is unavailable, the entire value is passed on the stack.

The TL;DR is that the psabi is supposed to treat failing to pass an SSEUP eightbyte in part of an xmm register (which naturally includes the containing ymm and zmm registers) the same as it would treat failing to pass any other class of eightbyte in a register - pass the whole value on the stack. Another way to think about it is that each eightbyte of an {x,y,z}mm register is a "separate" register that is used to pass SSEUP eightbytes. When passing __m128, __m256, or __m512 (as the first parameter/return), it's passed in {xmm0[0..64], xmm0[64..128], ymm0[128..192], ymm0[192..256], ...} and if any of those "registers" don't exist or are unavailable (ie. avx or avx512f are disabled), then we stop trying to pass the value in registers and pass the whole thing on the stack or return it in memory.

clang's behaviour is to decay the SSEUP eightbytes that can't be passed into SSE (thus passing in memory and returning in xmm0:xmm1 for __m256 values), which is incorrect (there is an llvm bug open to this effect, and its acknowledged as a bug). SSEUP eightbytes are replaced with SSE eightbytes when they aren't immediately preceeded by SSE or SSEUP.

[nikomatsakis]

Am I correct in understanding that

1. it would definitely be UB if you had some code that included the target feature and some code that did not
2. it is probably a bug to not be using the target feature and to call such a method (declaring...meh) because it will rely on some semi-specified ABI definition and anyway why are you passing this SIMD value without being able to work with it

?

[RalfJung]

it would definitely be UB if you had some code that included the target feature and some code that did not

Yes. Specifically, if caller and callee differ in this target feature, then the call is UB.

it is probably a bug to not be using the target feature and to call such a method (declaring...meh) because it will rely on some semi-specified ABI definition and anyway why are you passing this SIMD value without being able to work with it

Yes -- it is somewhat questionable that we even let you name the __m128i type when the corresponding feature is missing.

[pnkfelix]

The lang team discussed this in our triage meeting today.

• We would generally prefer to have things like this roll out via future-incompat warnings unless there is sufficient motivation (either w.r.t. (un)ease of implementation or a catastrophic level of impact if left unaddressed). We do not want to normalize an attitude of "the impact predicted by crater was low, therefore a hard error is okay."
• We didn't see evidence in this case to motivate sidestepping a future-incompat warning step. So we would appreciate the PR being revised to use a future-incompat warning (either for both the function declaration and the function call site, or, if the PR author desires, a future incompat warning for declaring such a function, while hard-erroring on an actual call to such a function.)
• We are comfortable with a post-monomorphization error here.
• We agree with Ralf that trying to generalize/complicate this check in some way to address semver compat is not worth the effort.

Hopefully that addresses everything you all wanted from the lang team's side; we definitely want to see this move forward.

[veluca93]

I changed the code to emit future-compat lints instead.

However, during the rebase I found some unexpected changes in unrelated tests (perhaps due to some subtle changes in the cycle resolution order in collector.rs? Just guessing here):

• tests/crashes/131342-2.rs no longer crashes (but the original snippet from ICE with core::iter::adapters::peekable + recursion + pattern matching on peek #131342 still does)
• tests/ui/layout/post-mono-layout-cycle-2.rs and tests/ui/layout/post-mono-layout-cycle.rs emit a different error message

Another question that is IMO still open is whether https://doc.rust-lang.org/stable/std/primitive.fn.html#requirements-concerning-target-features should still be removed when making the change a future-compat lint instead.

@pnkfelix, I think the code should be ready for review at this point.

[RalfJung]

Another question that is IMO still open is whether https://doc.rust-lang.org/stable/std/primitive.fn.html#requirements-concerning-target-features should still be removed when making the change a future-compat lint instead.

I think we can leave that out of this PR, to delay the discussion until later.