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

Wasm with lto and use of f32::sin() gets an unknown reference to env module #72758

Closed
rodrigorc opened this issue May 29, 2020 · 1 comment · Fixed by #72759
Closed

Wasm with lto and use of f32::sin() gets an unknown reference to env module #72758

rodrigorc opened this issue May 29, 2020 · 1 comment · Fixed by #72759
Labels
C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ regression-from-stable-to-beta Performance or correctness regression from stable to beta.

Comments

@rodrigorc
Copy link

This bug was wrongly posted at wasm-bindgen, but it looks like it is a compiler issue.

The following example fails to create a proper module, with either:

  • rustc 1.44.0-beta.4 (02c25b3 2020-05-23)
  • rustc 1.45.0-nightly (a74d186 2020-05-14)
    (stable rustc 1.43.1 works fine).

Cargo.toml

[package]
name = "wasm-env-issue"
version = "0.1.0"
edition = "2018"

[lib]
crate-type=["cdylib"]

[profile.release]
lto = true
panic = "abort"

[dependencies]
wasm-bindgen = "0.2.55"

src/lib.rs

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn wasm_sin(key: i32) -> i32 {
    (key as f32).sin() as i32
}

With this command to compile:

wasm-pack build --no-typescript --target no-modules

produces this error:

error: cannot import from modules (env) with --no-modules

Looking at the compiled wasm, with wasm-dis, there is this line that doesn't seem correct.

(import "env" "_ZN4core9panicking18panic_bounds_check17hadfb911a22557eccE" (func $_ZN4core9panicking18panic_bounds_check17hadfb911a22557eccE (param i32 i32 i32)))

Switching lto to false or changing the sin() to asin(), for example, make the "env" disappear.

@rodrigorc rodrigorc added the C-bug Category: This is a bug. label May 29, 2020
alexcrichton added a commit to alexcrichton/rust that referenced this issue May 29, 2020
Pulls in a fix for rust-lang#72758, more details on the linked issue.

[Crate changes included here][changes]

[changes]: rust-lang/compiler-builtins@0.1.28...0.1.31
@alexcrichton
Copy link
Member

For some more background on this issue, this is a standalone reproduction:

#[no_mangle]
pub extern "C" fn wasm_sin(key: i32) -> i32 {
    (key as f32).sin() as i32
}

where the issue can be seen with:

$ rustc +stable foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O
$ wasm2wat foo.wasm | rg import
$ rustc +stable foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O -C lto
$ wasm2wat foo.wasm | rg import
$ rustc +beta foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O
$ wasm2wat foo.wasm | rg import
$ rustc +beta foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O -C lto
$ wasm2wat foo.wasm | rg import
  (import "env" "_ZN4core9panicking18panic_bounds_check17hadfb911a22557eccE" (func $_ZN4core9panicking18panic_bounds_check17hadfb911a22557eccE (type 0)))

Here the bug is that, in beta (and onwards) with LTO turned on there's an unresolved symbol.

Bisection pointed out #70846 as the cause of the bug here. The problem appears to be that previously compiler-builtins was optimized enough such that llvm removed all panics and bounds checks, but with a codegen unit reshuffling of compiler-builtins this did not happen and a bounds check was still in there.

It's intended that compiler-builtins cannot panic, and we even have a test that this is the case. Unfortunately though WebAssembly wasn't tested on CI which notably activates the libm intrinsics, which is where the panic was coming from here (the sin function). I expanded testing to include wasm in rust-lang/compiler-builtins#360, and the first few commits failed showing lots of references to panics.

I fixed the implementation in libm at rust-lang/libm#244. Most of those changes were so in debug mode there weren't panic checks. In release mode there was only one panic check which needed fixing, but fixing them all was needed for compiler-builtins's CI.

I've submitted a fix for this in #72759 by updating compiler-builtins.

@alexcrichton alexcrichton added O-wasm Target: WASM (WebAssembly), http://webassembly.org/ regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 29, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 29, 2020
@bors bors closed this as completed in 4b1f86a May 31, 2020
@LeSeulArtichaut LeSeulArtichaut removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 31, 2020
alvinhochun added a commit to alvinhochun/wasm-project-template that referenced this issue Jun 17, 2020
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. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ regression-from-stable-to-beta Performance or correctness regression from stable to beta.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants