rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.76k stars 12.76k forks source link

`--enable-simd` feature is present in `wasm32-unknown-unknown` target, not enabled by `target-feature=+simd128` #133290

Closed dj8yfo closed 20 hours ago

dj8yfo commented 1 day ago

I tried this code:

https://github.com/dj8yfo/abstract-dao/tree/056f74e16fbaba84b75c49522c1176c477a975d1

git clone https://github.com/dj8yfo/abstract-dao.git
cd abstract-dao/
git checkout origin/1.82_plus_simd128_bug
cargo build --target wasm32-unknown-unknown --release

I expected to see this happen:

❯ : wasm-opt --print-features target/wasm32-unknown-unknown/release/near_abstract_dao.wasm
--enable-mutable-globals
--enable-sign-ext
--enable-reference-types
--enable-multivalue

In fact, this behaves as expected on previous version of Cargo.lock in https://github.com/dj8yfo/abstract-dao/tree/fe9d16797bf582f0ea24380c1e4ad6bdb8c97283 , before running cargo update

Instead, this happened: Additional --enable-simd feature is present in output wasm, which wasn't enabled with RUSTFLAGS = "-C target-feature=+simd128"

❯ : wasm-opt --print-features target/wasm32-unknown-unknown/release/near_abstract_dao.wasm
--enable-mutable-globals
--enable-simd
--enable-sign-ext
--enable-reference-types
--enable-multivalue

Meta

Present on rustc 1.84.0-nightly (3fee0f12e 2024-11-20) and rustc 1.83.0-beta.6 (4ff8ff0ec 2024-11-16) and on rustc 1.81.0 (eeb90cda1 2024-09-04) if moving backwards

rustc --version --verbose:

❯ : rustc -vV
rustc 1.82.0 (f6e511eec 2024-10-15)
binary: rustc
commit-hash: f6e511eec7342f59a25f7c0534f1dbea00d01b14
commit-date: 2024-10-15
host: x86_64-unknown-linux-gnu
release: 1.82.0
LLVM version: 19.1.1

this is also reproduced on aarch64-apple-darwin , more discussion/context is present in https://github.com/nearuaguild/abstract-dao/pull/9

Backtrace

``` ```

dj8yfo commented 1 day ago

on this version https://github.com/dj8yfo/abstract-dao/tree/056f74e16fbaba84b75c49522c1176c477a975d1 both 1.81 and 1.82 emit:

❯ : wasm2wat target/wasm32-unknown-unknown/release/near_abstract_dao.wasm o+e>| grep v128.store
        v128.store align=4
        v128.store align=4
        v128.store align=4
    v128.store align=8
    v128.store offset=16 align=8
    v128.store align=1
    v128.store align=1
        v128.store align=4

and on https://github.com/dj8yfo/abstract-dao/tree/fe9d16797bf582f0ea24380c1e4ad6bdb8c97283 v128.store aren't present

hanna-kruppe commented 1 day ago

It’s very likely that this is caused by one of the new/updated libraries in your lockfile, not a bug in the Rust compiler. Like instruction set extensions for physical CPU instruction sets, wasm target_features can be enabled for individual functions even if they’re not globally enabled. This is almost never correct for wasm because wasm engines validate the entire module up-front so you can’t do dynamic feature detection (the main use case for per-function target features) but library authors aren’t always aware of this. The lockfile diff is too large to pin down the culprit but you could look at the names of the functions that contain SIMD instructions, and/or update dependencies one-by-one with cargo update foo to narrow it down.

dj8yfo commented 22 hours ago

@hanna-kruppe i've managed to find the breaking diff, but the problem is rust tooling won't help, if someone declares #[target_feature(enable = "simd128")] without having a similar simd-named feature in list of features, declared by crate, will it?

i was able to find a crate by inspecting cargo build --verbose output

Running `/home/user/.rustup/toolchains/1.82-x86_64-unknown-linux-gnu/bin/rustc --crate-name const_hex --edition=2021 /home/user/.cargo/registry/sr
c/index.crates.io-6f17d22bba15001f/const-hex-1.13.1/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib
 --emit=dep-info,metadata,link -C opt-level=z -C panic=abort -C codegen-units=1 -C overflow-checks=on --cfg 'feature="alloc"' --cfg 'feature="default"' --cfg
 'feature="hex"' --cfg 'feature="std"' --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values("__fuzzing", "alloc", "default", "force-generic", "hex", "nightly", "portable-simd", "serde", "std"))' -C metadata=c9407567ee90fba3 -C extra-filename=-c9407567ee90fba3 --out-dir /home/user/Documents/code/abstract-
dao/target/wasm32-unknown-unknown/release/deps --target wasm32-unknown-unknown -C strip=debuginfo -L dependency=/home/user/Documents/code/abstract-dao/tar
get/wasm32-unknown-unknown/release/deps -L dependency=/home/user/Documents/code/abstract-dao/target/release/deps --extern cfg_if=/home/user/Documents/c
ode/abstract-dao/target/wasm32-unknown-unknown/release/deps/libcfg_if-a445d74a3cb11c4b.rmeta --extern hex=/home/user/Documents/code/abstract-dao/target/wa
sm32-unknown-unknown/release/deps/libhex-22b3235bdcb070cc.rmeta --cap-lints allow`

but if the library author had not included portable_simd feature by mistake or intentionally, i suspect i would've been out of luck finding it.

@hanna-kruppe you've somewhat highlighted the problem it's not possible to find out which target-feature were enabled by which library by a single command

hanna-kruppe commented 21 hours ago

Yes, Rust doesn't take a stance on this sort of thing. It's possible that rustc/cargo could do something to make diagnosis easier or reduce the odds of the problem occurring. It's not an unreasonable request, but there's some design work that someone would have to do to turn it into a concrete feature request.

dj8yfo commented 21 hours ago

@hanna-kruppe i'll close this then; can you take a look at https://github.com/DaniPopes/const-hex/issues/17 and confirm whether it's correct logic to conditionally enable this simd128 feature and IS a compiler problem, or it's incorrect logic to conditionally enable simd128 feature and NOT a compiler problem.

because the only doc i've found on it https://rust-lang.github.io/rfcs/2045-target-feature.html isn't that terribly clear at first glance

hanna-kruppe commented 21 hours ago

More up-to-date and hands-on documentation for using #[target_feature] is found in the std::arch documentation. The wasm32 submodule in particular also talks a bit about wasm-specific challenges.

dj8yfo commented 20 hours ago

@hanna-kruppe your original advice to look at

you could look at the names of the functions that contain SIMD instructions,

is unlikely to help to narrow the source of feature enabled, because it looks like the feature got enabled everywhere, and not only for const-hex individual functions

(func $_ZN67_$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$core..clone..Clone$GT$5clone17hea91e10ef6e5c6ceE (type 5) (param i32 i32)
...
v128.load offset=16 align=4
...
)
(func $_ZN5bytes9bytes_mut8BytesMut6freeze17h9bfc57e8231549f9E (type 5) (param i32 i32)
...
        v128.store align=4
...
)
dj8yfo commented 20 hours ago

@hanna-kruppe well, reading your explanation and looking at doc in https://doc.rust-lang.org/core/arch/wasm32/index.html Image

it looks like there's an issue with compiler after all, because generated wat got these SIMD instructions enabled everywhere, and not only for const-hex functions. I mean, after updating single package from https://github.com/dj8yfo/abstract-dao/tree/fe9d16797bf582f0ea24380c1e4ad6bdb8c97283 state:

cargo update -p const-hex
hanna-kruppe commented 19 hours ago

I don't have a lot of time to look further into this. You may have more luck getting someone to look into it if you minimize the reproducer to a single crate and can give instructions for testing it without having to install wasm-opt.