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

Breaking change in wasm32-unknown-unknown's ABI on nightly #81386

Closed
alexcrichton opened this issue Jan 25, 2021 · 5 comments · Fixed by #81388
Closed

Breaking change in wasm32-unknown-unknown's ABI on nightly #81386

alexcrichton opened this issue Jan 25, 2021 · 5 comments · Fixed by #81388
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@alexcrichton
Copy link
Member

Some tests in the wasm-bindgen project have started failing in nightly, and bisection points to #80594 as the culprit. The minimization of this looks like:

#[repr(C)]
pub struct Foo {
    pub a: u32,
    pub b: u32,
    pub c: u32,
}

#[no_mangle]
pub extern "C" fn foo(a: Foo) {}

where before this compiles as:

$ rustc +before foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O && wasm2wat foo.wasm | grep 'func..foo'
  (func $foo (type 0) (param i32 i32 i32))
  (export "foo" (func $foo))
$ rustc +after foo.rs --crate-type cdylib --target wasm32-unknown-unknown -O && wasm2wat foo.wasm | grep 'func..foo'
  (func $foo (type 0) (param i32))
  (export "foo" (func $foo))
@jonas-schievink jonas-schievink added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jan 25, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 25, 2021
@bjorn3
Copy link
Member

bjorn3 commented Jan 25, 2021

That must have been a93dace. This all types with Abi::Aggregate to become PassMode::Indirect. Later abi adjustments could then turn it into PassMode::Cast as necessary. I thought abi adjustments would never leave the dummy PassMode::Direct in place for Abi::Aggregate, but it seems not for wasm. I don't think this should ever happen as it will cause LLVM to do it's own abi adjustments. The whole point of that PR was to make it easier for Cranelift to match the abi of LLVM, which those abi adjustments would make harder.

I think there will need to be an arg.cast_to(...) call inserted in the abi handling of wasm. What kind of types does wasm-bindgen produce?

#[repr(C)]
pub struct Bar {
    pub foo: u8,
}

#[repr(C)]
pub struct Foo {
    pub a: Bar,
    pub b: u32,
    pub c: u32,
}

#[no_mangle]
pub extern "C" fn foo(a: Foo) {}

produces (func $foo (type 0) (param i32 i32 i32 i32 i32 i32)), so I can't think of a clear rule describing what is used by LLVM. The easiest fix is maybe to special case wasm32-unknown-unknown at

to use PassMode::Direct. This only affects wasm, which isn't supported by cg_clif anyway.

@bjorn3
Copy link
Member

bjorn3 commented Jan 25, 2021

#81388 will hopefully fix this.

@hameerabbasi hameerabbasi added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 25, 2021
@hameerabbasi
Copy link
Contributor

Assigning P-high as discussed on Zulip.

@Manishearth
Copy link
Member

@alexcrichton do you remember which of the tests were failing here? I was under the impression wasm-bindgen didn't support passing structs as arguments, and instead passed them by-reference.

I'm working with passing structs by-value over Wasm and am dealing with similar ABI weirdnesses (rust-diplomat/diplomat#657), and was curious if wasm-bindgen had already solve the problem somewhere but I can't get it to ever pass any aggregate by-value.

@alexcrichton
Copy link
Member Author

Alas this is too far back for me, I don't remember :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants