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

Regression: opt-level > 1 breaks some wasm-bindgen with rust 1.48 but not 1.47 #79282

Closed
exrok opened this issue Nov 21, 2020 · 8 comments
Closed
Labels
E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@exrok
Copy link

exrok commented Nov 21, 2020

Edit: environment variable RUSTFLAGS="-C target-cpu=native" caused issue, which when I used 1.47 didn't get applied.

When a program is compiled with opt-level > 1 ({'z', 's', 2 ,3}), wasm-bindgen fails with an error on the produced binary. This seems like an unexpected regression especially since opt-level <= 1 ({'0','1'}) are unaffected. Rust 1.47 does not have this issue.

See rustwasm/wasm-bindgen#2366

How to Reproduce

src/main.rs

use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub fn start() {
    unsafe { js_sys::Float32Array::view(&[0.0f32]); }
}

cargo.toml

[package]
name = "wasmbug"
version = "0.1.0"
edition = "2018"

[dependencies]
wasm-bindgen = "0.2.68"
js-sys = "0.3.45"

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

Then with rust 1.48 on linux

> cargo build --target wasm32-unknown-unknown --release
...
  
> cd target/wasm32-unknown-unknown/release
> wasm-bindgen --version
wasm-bindgen 0.2.68
> wasm-bindgen --target web --no-typescript --out-dir . wasmbug.wasm
error: import of `__wbg_buffer_49131c283a06686f` doesn't have an adapter listed

Version it worked on

It most recently worked on: Rust 1.47

Version with regression

rustc --version --verbose:

rustc 1.48.0 (7eac88abb 2020-11-16)
binary: rustc
commit-hash: 7eac88abb2e57e752f3302f02be5f3ce3d7adfb4
commit-date: 2020-11-16
host: x86_64-unknown-linux-gnu
release: 1.48.0
LLVM version: 11.0

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 21, 2020
@jyn514 jyn514 added O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 21, 2020
@jonas-schievink jonas-schievink added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Nov 21, 2020
@camelid
Copy link
Member

camelid commented Nov 22, 2020

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 22, 2020
@Badel2
Copy link
Contributor

Badel2 commented Nov 22, 2020

I have a similar issue but I'm using a completely different toolchain (stdweb instead of wasm-bindgen). In 1.48 everything works fine, I can compile my code to wasm and execute it in a browser. In 1.49 I can compile it, but when running it says "unreachable executed". Compiling in debug mode fixes the error. I tried to debug it and it looks like a panic while serializing something into a SerializedValue, so I think it's a problem of stdweb incorrectly using std::mem::uninitialized:

koute/stdweb#411

It would be great if someone can confirm that this use of std::mem::uninitialized is undefined behavior. As mentioned in that issue, I run a cargo-bisect and the first bad commit is b836329

Note that my issue may not be the same as OP's issue. I tried to reproduce OP's issue but was unable, to me it works fine both in 1.48 and 1.49. Also, I had to rename src/main.rs to src/lib.rs, I assume that was a typo.

So my theory is that Float32Array uses some unsafe code internally, code that was allowed before but it's optimized away now.

If someone manages to reproduce this issue, try to run the bisect using this command:

cargo bisect-rustc --target wasm32-unknown-unknown --start 2020-09-27 --end 2020-09-28 

@Mark-Simulacrum
Copy link
Member

It looks like SerializedValue is a struct:

#[repr(u8)]
pub enum Tag {
    Undefined = 0,
    Null = 1,
    I32 = 2,
    F64 = 3,
    Str = 4,
    False = 5,
    True = 6,
    Array = 7,
    Object = 8,
    Reference = 9,
    Function = 10,
    FunctionMut = 12,
    FunctionOnce = 13,
    UnsafeTypedArray = 14,
    Symbol = 15
}

#[repr(C)]
pub struct SerializedValue< 'a > {
    data_1: u64,
    data_2: u32,
    tag: Tag,
    phantom: PhantomData< &'a () >
}

I am not sure what our rules for repr(u8) enums are in terms of initialization, but I suspect that we consider them still not valid to be uninit. cc @RalfJung

@exrok
Copy link
Author

exrok commented Nov 22, 2020

I figured out partially what the cause of the problem is, I had RUSTFLAGS="-C target-cpu=native" which obviously could cause issues. Had it somewhere on both my test machines and my friend machines. Strangely, this never caused a problem before.

I don't know if I should close the issue?
edit: grammar

@RalfJung
Copy link
Member

RalfJung commented Nov 22, 2020

mem::uninitialized of Tag is definitely UB, yes. There should be a warning saying that this type "does not permit being left uninitialized".

@Mark-Simulacrum
Copy link
Member

Closing as working as expected / won't fix, in that case.

@camelid
Copy link
Member

camelid commented Nov 23, 2020

Note: I believe this is the warning that RalfJung was referring to:

warning: the type `Tag` does not permit being left uninitialized
  --> src/main.rs:23:33
   |
23 |     let mut tag: Tag = unsafe { mem::uninitialized() };
   |                                 ^^^^^^^^^^^^^^^^^^^^
   |                                 |
   |                                 this code causes undefined behavior when executed
   |                                 help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
   |
   = note: `#[warn(invalid_value)]` on by default
note: enums have to be initialized to a variant
  --> src/main.rs:4:1
   |
4  | / pub enum Tag {
5  | |     Undefined = 0,
6  | |     Null = 1,
7  | |     I32 = 2,
...  |
19 | |     Symbol = 15
20 | | }
   | |_^

@Badel2
Copy link
Contributor

Badel2 commented Nov 23, 2020

Correct, there is a warning "the type SerializedValue does not permit being left uninitialized". I didn't see it at first because that warning originates in an external dependency. You can use cargo build -vv to show all the warnings of all the dependencies, I used that to compile the original OP example and didn't see any warnings. So most probably my issue is different from OP's issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants