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

The force-enabling of SIMD128 feature can lead to unloadable WASMs in some browsers #144

Closed
stephanemagnenat opened this issue Jan 11, 2024 · 9 comments · Fixed by #149
Closed
Labels

Comments

@stephanemagnenat
Copy link

In several cases, memchr force-enables the SIMD feature on wasm32 (for example here). This has been an issue for me because, in a fairly large project where memchr is used indirectly by several packages (nom, nom_locate, pulldown-cmark and regex), the code with SIMD gets pulled in and the resulting WASM fails to load on some slightly-older browsers such as Safari iOS 15, with an error looking like:

CompileError: WebAssembly.Module doesn't parse at byte X: can't get Function local's type in group Y, in function at index Z

An example of code pulled in (as shown by wasm-decompile) is:

function memchr_arch_wasm32_simd128_packedpair_Finder_with_pair_impl_hd8b88a6eee7061af(a:long_ptr, b:int, c:int, d:int, e:int)

In release mode the code has so far been optimized out but in dev mode it is still in, although I did not specify target-feature=+simd128 in my RUSTFLAGS, so it is a problem to test/debug with older Safari. I'm also worried whether in some cases such code might find its way to the release mode as well.

A possible work-around would be to add more aggressive dead code elimination in dev mode but I did not find how to do that.

@BurntSushi
Copy link
Owner

Is it possible to reproduce this using wasmtime? That's what I use for testing the wasm stuff. In CI, I use this:

CARGO_BUILD_TARGET=wasm32-wasi RUSTFLAGS=-Ctarget-feature=+simd128 CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime run --wasm simd --" cargo t

So I tried getting rid of enabling SIMD:

CARGO_BUILD_TARGET=wasm32-wasi CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime run --" cargo t

But that still works fine. Then I tried forcefully disabling simd128:

CARGO_BUILD_TARGET=wasm32-wasi RUSTFLAGS=-Ctarget-feature=-simd128 CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime run --" cargo t

But that works too. So try as I might, I can't make the current code fail.

I note that SIMD isn't actually force enabled on wasm32. It is specifically only used when the simd128 feature is enabled at compile time. For example:

#[inline]
pub fn is_available() -> bool {
#[cfg(target_feature = "simd128")]
{
true
}
#[cfg(not(target_feature = "simd128"))]
{
false
}
}

Now, there are functions defined with #[target_feature(enable = "simd128")] that are present regardless of whether simd128 is enabled or not. But that should be fine. The entire point of #[target_feature(enable = "...")] is to be able to annotate functions with ISA extensions that might not actually be supported by the current CPU.

From what I can gather, the problem here is that something is choking on the function definition itself in the wasm binary. So I suppose your commentary about dead code elimination makes sense...

Since wasm32's simd128 stuff is only enabled when simd128 is enabled at compile time, the code can probably add a #[cfg(target_feature = "simd128")] to the definition of the simd128 module itself, so that that code isn't present at all if the simd128 feature isn't enabled at compile time. The problem with this approach is that it is incompatible in a world where we do runtime detection for simd128. I don't think that's available in wasm yet (not sure, I'm not wasm expert), but I think it's planned? So the #[cfg(target_feature = "simd128")] idea doesn't seem like a long term fix.

@stephanemagnenat
Copy link
Author

stephanemagnenat commented Jan 11, 2024

Thank you very much for looking into this!

Is it possible to reproduce this using wasmtime?

I do not know, I have a giant (14 MB) wasm file to be used on the web.

It is easy to validate with wasm-validate from wabt though:

wasm-validate --disable-simd FILE.wasm

Now, there are functions defined with #[target_feature(enable = "simd128")] that are present regardless of whether simd128 is enabled or not.

Yes, that is the problem. On Safari iOS (test with version 15.5, from late 2021/early 2022, so not terribly old), the resulting WASM fails to load because the browser attempts to validate it and it finds instructions it doesn't know (the SIMD ones present in the functions that are never called), so the validation and therefore the loading fail.

But that should be fine.

In general it should be, but unfortunately with validating browsers, it is not.

Since wasm32's simd128 stuff is only enabled when simd128 is enabled at compile time, the code can probably add a #[cfg(target_feature = "simd128")] to the definition of the simd128 module itself, so that that code isn't present at all if the simd128 feature isn't enabled at compile time.

That would solve the problem at the moment with browsers.

The problem with this approach is that it is incompatible in a world where we do runtime detection for simd128. I don't think that's available in wasm yet (not sure, I'm not wasm expert), but I think it's planned? So the #[cfg(target_feature = "simd128")] idea doesn't seem like a long term fix.

I agree, but I would argue that the short term problem is way more critical. I guess we could switch back to a less brutal approach if/when there is a runtime detection mechanism widely available in browsers?

@stephanemagnenat
Copy link
Author

stephanemagnenat commented Jan 11, 2024

The current way of doing feature detection with WASM on browsers is to try to load a small WASM with the specific feature and see whether it fails. See for example this library from Google.

@stephanemagnenat stephanemagnenat changed the title The force-enabling of SIMD128 feature can lead to unloadable WASMs The force-enabling of SIMD128 feature can lead to unloadable WASMs in some browsers Jan 11, 2024
@BurntSushi
Copy link
Owner

cc @alexcrichton Do you have any opinions here? I'm tempted to just gate the wasm32 SIMD optimizations under #[cfg(target_feature = "simd128")]. I think enabling simd128 at compile time is already required to take advantage of the SIMD in this crate on the wasm32 target, so I don't believe there will be any regression. But it might be difficult to unwind that change and support true runtime feature detection in the future.

@alexcrichton
Copy link
Contributor

(apologies if I'm repeating anything already said here I'm only responding to @BurntSushi's latest comment)

I would agree with your conclusion, gating everything behind #[cfg]. WebAssembly is pretty unlikely to ever get true runtime feature detection though. Embedders of wasm can do feature detection at the JS level but it would require them to manage two builds of their module and downloading the right one. There's no way to have a single module with simd that older browsers without simd support don't load.

@BurntSushi
Copy link
Owner

All righty, done deal. I'll get a patch up for this soonish.

@stephanemagnenat
Copy link
Author

Thank you both of you for your quick reaction and your attention!

@stephanemagnenat
Copy link
Author

Any update on the patch?

BurntSushi added a commit that referenced this issue Mar 27, 2024
It turns out that providing routines with `#[target_feature(enable =
"simd128")]` on `wasm32` can fail in some older browsers. The specific
problem is not totally clear to me, but it is straight-forward enough to
fix (I hope) by just requiring that `simd128` be enabled at compile time
in order to include the `wasm32` SIMD modules in this crate.

This would not be a great solution if WASM supported runtime CPU feature
detection. And the status quo is that `simd128` has to be enabled at
compile time anyway for the SIMD code to take effect. So this shouldn't
cause any regressions and is likely something we can do long term as
well. We can re-evaluate once and if WASM gets support for runtime CPU
feature detection.

We also add a CI test for `wasm32` *without* the `simd128` target
feature enabled.

Fixes #144
BurntSushi added a commit that referenced this issue Mar 27, 2024
It turns out that providing routines with `#[target_feature(enable =
"simd128")]` on `wasm32` can fail in some older browsers. The specific
problem is not totally clear to me, but it is straight-forward enough to
fix (I hope) by just requiring that `simd128` be enabled at compile time
in order to include the `wasm32` SIMD modules in this crate.

This would not be a great solution if WASM supported runtime CPU feature
detection. And the status quo is that `simd128` has to be enabled at
compile time anyway for the SIMD code to take effect. So this shouldn't
cause any regressions and is likely something we can do long term as
well. We can re-evaluate once and if WASM gets support for runtime CPU
feature detection.

We also add a CI test for `wasm32` *without* the `simd128` target
feature enabled.

Fixes #144
@BurntSushi
Copy link
Owner

This should hopefully be fixed in memchr 2.7.2 on crates.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants