Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[target_feature(enable = "simd128")] does not work properly on 1.73, but work fine on 1.72.1 #116516

Closed
mrDIMAS opened this issue Oct 7, 2023 · 15 comments · Fixed by #116576
Closed
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression.

Comments

@mrDIMAS
Copy link

mrDIMAS commented Oct 7, 2023

Code

I tried this code on 1.73:

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

#[target_feature(enable = "simd128")]
fn foo() {
	const FOO: v128 = i8x16(0, 1, -1, -1, -1, -1, -1, -1, 2, 3, -1, -1, -1, -1, -1, -1);
	println!("{:?}", FOO);
}

fn main() {
    foo();
}

Compiled it with cargo build --target=wasm32-unknown-unknown, I expected it to compile fine as on previous 1.72.1, but it gave me these errors:

$ cargo build --target=wasm32-unknown-unknown
   Compiling regression_test v0.1.0 (C:\Users\123\Desktop\regression_test)
error[E0080]: evaluation of constant value failed
 --> src\main.rs:5:20
  |
5 |     const FOO: v128 = i8x16(0, 1, -1, -1, -1, -1, -1, -1, 2, 3, -1, -1, -1, -1, -1, -1);
  |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ calling a function that requires unavailable target features: simd128

note: erroneous constant used
 --> src\main.rs:6:19
  |
6 |     println!("{:?}", FOO);
  |                      ^^^

note: erroneous constant used
 --> src\main.rs:6:19
  |
6 |     println!("{:?}", FOO);
  |                      ^^^
  |
  = note: this note originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0080`.
error: could not compile `regression_test` (bin "regression_test") due to previous error

Version it worked on

It most recently worked on: 1.72.1

Output:

$ cargo build --target=wasm32-unknown-unknown
Compiling regression_test v0.1.0 (C:\Users\123\Desktop\regression_test)
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s

Version with regression

rustc --version --verbose:

$ rustc --version --verbose
rustc 1.73.0 (cc66ad468 2023-10-03)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: x86_64-pc-windows-msvc
release: 1.73.0
LLVM version: 17.0.2

Other

I read release notes for 1.73 and couldn't find anything that could be related to this, also tried to search for an existing issue about this - no luck. This issue also broke compilation of fast_image_resize crate and transitively, Fyrox game engine.

@mrDIMAS mrDIMAS added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 7, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 7, 2023
@ehuss
Copy link
Contributor

ehuss commented Oct 7, 2023

Bisected to #113720. cc @RalfJung

@saethlin
Copy link
Member

saethlin commented Oct 7, 2023

And then this was fixed by (or at least it started compiling since) #115580

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2023

Hm, why does it think that the i8x16 function requires that target feature? That function doesn't have any target_feature attribute...

Cc @eduardosm

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2023

Ah, that's what changed in #115580 -- that attribute got removed from a bunch of functions.

This was changed in rust-lang/stdarch#1448. Cc @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Oct 8, 2023

That was changed in stdarch specifically in response to #113720, to avoid breaking users who were using the const fn already.

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2023

In principle we could probably silence this check on wasm, since we document wasm to be a target where calling functions with unsupported target features is safe.

@eduardosm
Copy link
Contributor

What happens in wasm when you call a function that requires an unavailable target feature?

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2023

I am told that the wasm runtime will refuse to even load a module that contains invalid operations, so there are two options:

  • the generated code makes no use of the target feature, and so everything just works
  • the generated code makes use of the target feature, and we don't even ever get to the function call since loading the wasm module already failed

@apiraino
Copy link
Contributor

apiraino commented Oct 9, 2023

@mrDIMAS can you check on of the latest nightlies to verify if it works for you? (ref. comment)

@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2023

We have a special exception for wasm that allows target_feature functions to not be marked unsafe:

if tcx.sess.target.is_like_wasm || tcx.sess.opts.actually_rustdoc {
// The `#[target_feature]` attribute is allowed on
// WebAssembly targets on all functions, including safe
// ones. Other targets require that `#[target_feature]` is
// only applied to unsafe functions (pending the
// `target_feature_11` feature) because on most targets
// execution of instructions that are not supported is
// considered undefined behavior. For WebAssembly which is a
// 100% safe target at execution time it's not possible to
// execute undefined instructions, and even if a future
// feature was added in some form for this it would be a
// deterministic trap. There is no undefined behavior when
// executing WebAssembly so `#[target_feature]` is allowed
// on safe functions (but again, only for WebAssembly)
//
// Note that this is also allowed if `actually_rustdoc` so
// if a target is documenting some wasm-specific code then
// it's not spuriously denied.
//
// This exception needs to be kept in sync with allowing
// `#[target_feature]` on `main` and `start`.
} else if !tcx.features().target_feature_11 {
let mut err = feature_err(
&tcx.sess.parse_sess,
sym::target_feature_11,
attr.span,
"`#[target_feature(..)]` can only be applied to `unsafe` functions",
);
err.span_label(tcx.def_span(did), "not an `unsafe` function");
err.emit();
} else {

We should probably have the same exception in the target-feature check in the interpreter. @eduardosm do you have the time for a follow-up PR implementing that?

@mrDIMAS
Copy link
Author

mrDIMAS commented Oct 9, 2023

@apiraino I re-checked on latest nightly and seems to work fine:

$ rustup default nightly
info: using existing install for 'nightly-x86_64-pc-windows-msvc'
info: default toolchain set to 'nightly-x86_64-pc-windows-msvc'

  nightly-x86_64-pc-windows-msvc unchanged - rustc 1.75.0-nightly (bf9a1c8a1 2023-10-08)

$ rustup target add wasm32-unknown-unknown
info: downloading component 'rust-std' for 'wasm32-unknown-unknown'
info: installing component 'rust-std' for 'wasm32-unknown-unknown'

$ cargo build --target=wasm32-unknown-unknown
   Compiling fail v0.1.0 (C:\Users\123\Desktop\regression_test)
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s

@eduardosm
Copy link
Contributor

do you have the time for a follow-up PR implementing that?

I will do that

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

Issue is fixed on nightly+beta, leaving open to follow PR #116576 fixing this comment

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 11, 2023
@bors bors closed this as completed in 03cbf50 Oct 15, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 15, 2023
Rollup merge of rust-lang#116576 - eduardosm:const-eval-wasm-target-features, r=RalfJung

const-eval: allow calling functions with targat features disabled at compile time in WASM

This is not unsafe on WASM, see rust-lang#84988

r? `@RalfJung`

Fixes rust-lang#116516
@mrDIMAS
Copy link
Author

mrDIMAS commented Oct 15, 2023

In which version this fix will be included?

@apiraino
Copy link
Contributor

In which version this fix will be included?

Should land in stable 1.74 (around mid November), iiuc.

See release cycle or an easier to read calendar

@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 16, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 17, 2023
…r=RalfJung

const-eval: allow calling functions with targat features disabled at compile time in WASM

This is not unsafe on WASM, see rust-lang/rust#84988

r? `@RalfJung`

Fixes rust-lang/rust#116516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression.
Projects
None yet
9 participants