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

[craterbot]

🚧 Experiment pr-127731 is now running

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

[craterbot]

🎉 Experiment pr-127731 is completed!
📊 144 regressed and 10 fixed (488045 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[RalfJung]

There's only 2 crates.io failures and they are both spurious. And the github failures I checked were also all spurious. So looking pretty good I would say. :)

Seems like the next step process-wise is to check back in with the lang team, now that we have crater results. (That's based on this message.)
@rust-lang/lang this aims to fix #116558, which is part of the discussion we had in rust-lang/lang-team#235. The way it is fixed is by making it a hard error to call or declare an extern "C" function that passes a SIMD type by-value without enabling the target feature that enables SIMD vectors of the appropriate size. The reason it is made a hard error is that these functions end up having a different ABI depending on whether that target feature is enabled or not. "Rust" ABI functions are not affected as those functions pass SIMD vectors indirectly (they do that precisely to avoid these ABI issues).

There are two points we'd like your take on:

• How to stage this. Should it become a hard error immediately, or first be reported as a future-compat lint (I would say we can make it show up in dependencies immediately)? Crater found no regressions (probably because extern "C" functions are generally fairly rare), but of course crater doesn't see all the codebases out there.
• What exactly should be checked? Right now, the check is extremely low-level and looks at the computed effective ABI of the function, ensuring it doesn't end up with a too large by-value SIMD vector argument. The check is done during monomorphization, each time a function is instantiated, to make sure that we can even know the actual effective ABI. More mono-time checks are unfortunate, but there's no good way to test this in generic code since we can't know whether a generic argument is a SIMD vector or not. Give that it should be rather rare to run into this, hopefully a mono-time check is acceptable.
  The other concern is semver compatibility: if a library changes a public type, e.g., from being an array of i32 to being a SIMD vector instead, that can now break downstream code. However, it can only break downstream code that passes types of this library by-value across an extern "C". If that is an FFI boundary, this is already extremely sketchy to do due to all the concerns around FFI safety and ABI compatibility. The most realistic scenario I could come up with is code using extern "C" for Rust-to-Rust calls; not sure how much we worry about such code -- I presume it is fairly rare. Still, we could try to mitigate this by making the check some sort of overapproxomation that generally refuses to pass types from other crates across an extern "C" boundary if those types could in the future be changed such that the ABI ends up with a by-value SIMD vector (i.e., (i32, ForeignType) would be okay but TransparentWrapper<ForeignType> would not). This seems quite hard to do right. I personally am not convinced that is worth the effort.

[traviscross]

@rustbot labels +T-lang +I-lang-nominated

Nominating per the message from @RalfJung above.

[RalfJung]

unstable_target_features is an odd name... just leaving a note here so that the t-compiler reviewer can carefully check that this is checking the right thing: we have to ensure that the target feature is enabled when generating code for this function.

[RalfJung]

I helped with the initial implementation but for finalizing this, I'll hand off to someone in the compiler team.
(Also this is waiting for the lang team before it can land.)

r? compiler

[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.