Don't warn about v128 in wasm ABI transition

[alexcrichton]

The -Zwasm-c-abi=spec mode of extern "C" does not actually change the meaning of v128 meaning that the FCW lint firing is a false positive.

cc #138762 (comment)

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[alexcrichton]

Like with #139498, I'm going to beta-nominate this as the affected warning is currently present on beta.

[jieyouxu]

Hi @alexcrichton, could you elaborate on what you mean by "other warning if necessary"?

Based on the reported code snippet in #138762 (comment) but with #![allow(wasm_c_abi)]

#![allow(wasm_c_abi)]
use std::arch::wasm32::{
    v128,
    v128_any_true};

#[no_mangle]
pub fn any_true(a: v128) -> bool {
    v128_any_true(a)
}

I'm not observing another warning, am I missing something? Or rather, could you slightly elaborate on why v128 specifically is okay for wasm_c_abi?

[alexcrichton]

Oh sure yeah, this code:

use std::arch::wasm32::*;

#[unsafe(no_mangle)]
#[target_feature(enable = "simd128")]
pub extern "C" fn foo(_: v128) {}

currently yields two warnings

warning: `extern` fn uses type `std::arch::wasm32::v128`, which is not FFI-safe
 --> <source>:5:26
  |
5 | pub extern "C" fn foo(_: v128) {}
  |                          ^^^^ not FFI-safe
  |
  = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
  = note: this struct has unspecified layout
  = note: `#[warn(improper_ctypes_definitions)]` on by default

warning: this function definition involves an argument of type `std::arch::wasm32::v128` which is affected by the wasm ABI transition
 --> <source>:5:1
  |
5 | pub extern "C" fn foo(_: v128) {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #138762 <https://github.com/rust-lang/rust/issues/138762>
  = help: the "C" ABI Rust uses on wasm32-unknown-unknown will change to align with the standard "C" ABI for this target
  = note: `#[warn(wasm_c_abi)]` on by default

This PR is just addressing the second one which shows up transitively through crates, not the first one which I'm not changing here.

[jieyouxu]

This PR is just addressing the second one which shows up transitively through crates, not the first one which I'm not changing here.

Thanks, that's what I was missing

[jieyouxu]

Thanks

[jieyouxu]

@bors r+ rollup

[bors]

📌 Commit 38667e5 has been approved by jieyouxu

It is now in the queue for this repository.

[RalfJung]

improper_ctypes warns about so many things that are needed in practice and generally work fine, chances are people have it allowed... but I guess then they don't get to complain if we change the ABI.^^

[jieyouxu]

improper_ctypes warns about so many things that are needed in practice and generally work fine, chances are people have it allowed... but I guess then they don't get to complain if we change the ABI.^^

I suppose we can call this out in relnotes?

[jieyouxu]

Unsettled discussion on trade-offs
@bors r-

[RalfJung]

We have a blogpost about the transition, and the relnotes (I expect) will link there. So there are other ways people can learn about this.

But, also note that improper_ctypes is not an FCW so unlike this warning, it will not show up for dependencies. If my dependency passes v128 over a wasm ABI, then with this PR I will get no warning.

[alexcrichton]

Sorry I'm a bit confused, what's being asked for here?

The v128 type is already ABI-unstable in that it generates a compiler warning. The wasm C ABI transition doesn't affect that. This PR is instead fixing a bug where when using std::arch::wasm32::* intrinsics they're generating a warning about the C ABI transition which is a false positive. Given that I'm not certain what's being asked for to be changed here?

[RalfJung]

I'm slightly concerned some crate could be passing v128, either allowing or ignoring the lint, as that lint has plenty of false positives so it is often being allowed. Only the author of the crate would see the lint anyway. Some user of that crate would see this FCW, however, thus warning them that they can be affected by this transition.

Can you point at the declaration of such an intrinsic that causes a warning? Why would an intrinsic use the "C" ABI? Usually they do not.

[hanna-kruppe]

The intrinsics that get the warning are LLVM intrinsics, not Rust intrinsics, which stdarch declares as extern "C" here: https://github.com/rust-lang/stdarch/blob/7ae34fad5e3c55c1ff150121872d9ee5f4d56a94/crates/core_arch/src/wasm32/simd128.rs#L76

[jieyouxu]

r? RalfJung (since you have concerns)

[bors]

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

[RalfJung]

The intrinsics that get the warning are LLVM intrinsics, not Rust intrinsics, which stdarch declares as extern "C" here: https://github.com/rust-lang/stdarch/blob/7ae34fad5e3c55c1ff150121872d9ee5f4d56a94/crates/core_arch/src/wasm32/simd128.rs#L76

Is there a specific reason for this? Most (all?) other targets use extern "unadjusted" { ... } for this. Wasm "C" used to behave like "unadjusted" but the entire point of this ABI transition is to fix that.

[alexcrichton]

@RalfJung the warning this PR is fixing is showing up on beta/nightly right now and ideally something should be landed/backported to fix both channels. It sounds like you're concerned about correcting the source of truth for these intrinsics and warning as many people as possible about the usage of simd types in the C ABI on wasm, and while I sympathize with that I feel like it's orthogonal to the purpose of this PR, to fix the warning that should not be firing. In terms of backporting I suspect a more focused change such as this would be more amenable to backporting than updating the stdarch submodule which has the risk of bringing in other unrelated changes too.

I agree if it works it's probably best to use "unadjusted" as the ABI for wasm intrinsics in this situation, but I also don't think such work should block this PR and fixing the warning on beta.

[RalfJung]

I'm only in favor of landing this PR if it is actually correct that the ABI of this SIMD type is the same under the old and new ABI. My understanding is that that is not the case. We can of course land a temporary hack, but that should be clearly documented as such. @Amanieu what are your thoughts regarding the proper way to fix these warning that originate from compiler-builtins? I am a bit confused since I would expect that if the SIMD ABI changes in this transition, then we'd see SIMD intrinsics break when setting the -Z flag that enables the new ABI.

[RalfJung]

@Amanieu what are your thoughts regarding the proper way to fix these warning that originate from compiler-builtins?

Ah never mind, the compiler-builtins PR has landed. Any chance we could get a release so we can check whether that fixes the underlying issue here?

[alexcrichton]

I believe that simd types get special handling. Here's a comparison showing the ABI being the same. I'm under the impression this PR is not a "temporary hack" but it's a fix of a false positive of the transition warning firing.

[RalfJung]

Ah, if the ABI is the same then that is a good argument for not showing the warning. :) The PR description makes a different argument though, which is why I didn't realize this.

Could you update the PR description and the comment in the code to refer to the fact that vectors are indeed unaffected by the transition?

[alexcrichton]

Sure, updated

[RalfJung]

Suggested change

	// If this is a simd argument let other rustc warnings kick in and don't
	// provide extra warnings here
	// SIMD types are passed the same way under both ABIs.

[RalfJung]

This comment also needs updating.

[RalfJung]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.