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)

[rustbot]

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

[Amanieu]

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?

You mean stdarch? There's no release, you just need to update the submodule.

[alexcrichton]

@rustbot ready

[RalfJung]

You mean stdarch? There's no release, you just need to update the submodule.

Ah sorry, I mixed up the crates.

[RalfJung]

@alexcrichton thanks, and sorry for the delay!
@bors r+

[bors]

📌 Commit 19e44d4 has been approved by RalfJung

It is now in the queue for this repository.

[jieyouxu]

This was discussed in today's compiler triage meeting. Beta backport was approved. Thanks!

@rustbot label: +beta-accepted