Fix abi for wasm-bindgen by bjorn3 · Pull Request #81388 · rust-lang/rust

bjorn3 · 2021-01-25T17:51:04Z

Hopefully fixes #81386. @alexcrichton can you confirm this fixes wasm-bindgen?

alexcrichton · 2021-01-25T18:43:28Z

I don't really understand #80594 or why it would break wasm. Similarly I don't understand the code this is modifying in this PR at this moment to fix the issue. I would expect that this issue would get fixed in this file https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/abi/call/wasm32_bindgen_compat.rs, however.

bjorn3 · 2021-01-25T19:01:47Z

Previously PassMode::Direct would be used first, some abi independent adjustments would be performed and then some abi dependent adjustments. I expected those to always result in PassMode::Cast or PassMode::Indirect, but for wasm32_bindgen_compat PassMode::Direct is kept in most cases, even when that would require LLVM to do it's own abi adjustments. Now for Abi::Aggregate, PassMode::Indirect is used, followed by some abi independent and dependent adjustments, which breaks the wasm32_bindgen_compat abi as it didn't do the expected adjustments. This fix reverts back to the old behavior for wasm32-unknown-unknown. Doing this in wasm32_bindgen_compat would require repeating all abi independent adjustments there.

alexcrichton · 2021-01-25T19:18:09Z

Sorry a short explanation won't really help me review this, to review this PR as-is I would have to dive in myself to understand things, which I don't have time to do right now.

There is already one specific location, however, which selects the wasm ABI:

rust/compiler/rustc_target/src/abi/call/mod.rs

Lines 634 to 637 in 7fba12b

    
           "wasm32" => match cx.target_spec().os.as_str() { 
        
               "emscripten" | "wasi" => wasm32::compute_abi_info(cx, self), 
        
               _ => wasm32_bindgen_compat::compute_abi_info(self), 
        
           },

I don't think we should have two different checks for the ABI, and even less so two different checks. I hope that in the long-term the previous ABI (e.g. that on the stable/beta channel) will be the final ABI for wasm, in which case I don't think there should be a "if wasm do this" check in the middle of otherwise platform-independent code.

bjorn3 · 2021-01-25T19:42:08Z

I hope that in the long-term the previous ABI (e.g. that on the stable/beta channel) will be the final ABI for wasm

That would make it impossible to link against C code, which uses the official wasm C abi instead of the wasm-bindgen compat abi.

I don't think there should be a "if wasm do this" check in the middle of otherwise platform-independent code.

There are already several target checks in the platform-independent code. For example some targets don't ignore ZST arguments, which the platform-independent code normally sets to ignore. (and it must always ignore them for the rust abi to make closure to fn-ptr casts work.)

alexcrichton · 2021-01-25T19:52:20Z

I'm thinking more broadly than what's the case right-this-red-hot-second. I would hope that Clang changes ABI as well.

Again, I can't review changes to this code as-is. One comment I would have, however, is that this is a different check that the one already existing in the codebase. Those two checks should probably not be different nor entirely isolated from one another.

rust-log-analyzer · 2021-01-25T20:31:54Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking rustc_ast v0.0.0 (/checkout/compiler/rustc_ast)
    Checking rustc_target v0.0.0 (/checkout/compiler/rustc_target)
    Checking rustc_feature v0.0.0 (/checkout/compiler/rustc_feature)
    Checking rustc_parse_format v0.0.0 (/checkout/compiler/rustc_parse_format)
error[E0277]: can't compare `str` with `&str`
   --> compiler/rustc_target/src/abi/call/mod.rs:648:29
    |
648 |     if target_spec.arch[..] == "wasm32" {
    |                             ^^ no implementation for `str == &str`
    |
    = help: the trait `PartialEq<&str>` is not implemented for `str`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `rustc_target`
error: could not compile `rustc_target`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_driver" "-p" "rustc_ast" "-p" "rustc_lexer" "-p" "rustc_macros" "-p" "rustc_index" "-p" "rustc_plugin_impl" "-p" "rustc_parse" "-p" "rustc_mir" "-p" "rustc_apfloat" "-p" "coverage_test_macros" "-p" "rustc_infer" "-p" "rustc_trait_selection" "-p" "rustc_parse_format" "-p" "rustc_graphviz" "-p" "rustc_attr" "-p" "rustc_data_structures" "-p" "rustc_hir" "-p" "rustc_middle" "-p" "rustc_arena" "-p" "rustc_query_system" "-p" "rustc_type_ir" "-p" "rustc_target" "-p" "rustc_span" "-p" "rustc_session" "-p" "rustc_fs_util" "-p" "rustc_lint_defs" "-p" "rustc_save_analysis" "-p" "rustc_metadata" "-p" "rustc_expand" "-p" "rustc_ast_passes" "-p" "rustc_lint" "-p" "rustc_interface" "-p" "rustc_mir_build" "-p" "rustc_ty_utils" "-p" "rustc_resolve" "-p" "rustc_ast_lowering" "-p" "rustc_typeck" "-p" "rustc_privacy" "-p" "rustc_builtin_macros" "-p" "rustc_passes" "-p" "rustc_symbol_mangling" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_traits" "-p" "rustc_incremental" "-p" "rustc_feature" "-p" "rustc_hir_pretty" "-p" "rustc_errors" "-p" "rustc_serialize" "-p" "rustc_error_codes" "-p" "rustc_ast_pretty" "-p" "rustc_codegen_ssa" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:50

bjorn3 · 2021-01-25T20:40:51Z

I expect that the wasm-bindgen abi is slower due to making it impossible to construct large arguments in place and passing a pointer rather than requiring the wasm compiler to copy them to the arguments location. In addition as noted in #81386 (comment) the abi is actually much more complex than it seems. LLVM does a lot of adjustments behind the back, including one for which I can't think of a clear rule as evidenced by the example there where a struct containing an i8 wrapped inside a struct and two standalone i32 turn into 6! i32 arguments at the abi level. I wouldn't expect more than three arguments.

rust-log-analyzer · 2021-01-25T20:44:15Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_target/src/abi/call/mod.rs at line 631:
             "nvptx64" => nvptx64::compute_abi_info(self),
             "hexagon" => hexagon::compute_abi_info(self),
             "riscv32" | "riscv64" => riscv::compute_abi_info(cx, self),
-            "wasm32" => if use_wasm_bindgen_compat_abi(cx.target_spec()) {
-                wasm32_bindgen_compat::compute_abi_info(self)
-            } else {
-                wasm32::compute_abi_info(cx, self)
-            },
+            "wasm32" => {
+                if use_wasm_bindgen_compat_abi(cx.target_spec()) {
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_target/src/abi/call/mod.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
+                    wasm32_bindgen_compat::compute_abi_info(self)
+                } else {
+                    wasm32::compute_abi_info(cx, self)
+            }
+            }
             "asmjs" => wasm32::compute_abi_info(cx, self),
             a => return Err(format!("unrecognized arch \"{}\" in target specification", a)),
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:16

bjorn3 · 2021-01-26T10:06:47Z

In addition as noted in #81386 (comment) the abi is actually much more complex than it seems. LLVM does a lot of adjustments behind the back, including one for which I can't think of a clear rule as evidenced by the example there where a struct containing an i8 wrapped inside a struct and two standalone i32 turn into 6! i32 arguments at the abi level. I wouldn't expect more than three arguments.

I think I figured the abi out. There are 3 i8's used as padding in this case. These get promoted to i32.

#[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) {}

%Foo = type { [0 x i8], %Bar, [3 x i8], i32, [0 x i32], i32, [0 x i32] }
%Bar = type { [0 x i8], i8, [0 x i8] }

; Function Attrs: norecurse nounwind readnone
define void @foo(%Foo %0) unnamed_addr #0 {
start:
  ret void
}

I think the rule is that it will flatten nested structs and extend any i8 used as padding to i32. I don't think cast_to supports this extension of padding bytes. Looking at all #[repr(C)] structs in wasm-bindgen it seems that padding is never used though, so I think I can keep compatibility with wasm-bindgen using cast_to.

This reverts commit e7a056f.

This reverts commit 4d2766e.

bjorn3 · 2021-01-26T10:33:45Z

I pushed a new version that doesn't change the abi independent part. It should have the same ABI for all structs without padding. As far as I could see wasm-bindgen doesn't ever use any types with padding, nor could I find any code to handle padding, so this should be compatible with wasm-bindgen.

This reverts commit 903c553.

bjorn3 · 2021-01-26T12:46:02Z

The more I think about it the less I am certain that defaulting Abi::Aggregate to PassMode::Indirect doesn't change the abi for more targets. I have switched it back to PassMode::Direct. At least for x86_64 PassMode::Direct will never be used for aggregates I think, so it shouldn't have much negative effects for cg_clif. I am now fairly certain that there are no latent changes to any non-rust target due to #80594.

alexcrichton · 2021-01-26T15:27:25Z

This is unfortunately well beyond my ability to review at this point.

nikomatsakis · 2021-01-28T21:18:46Z

@bors r+ p=1

bors · 2021-01-28T21:18:48Z

📌 Commit c1c06f3 has been approved by nikomatsakis

bors · 2021-01-28T22:01:48Z

⌛ Testing commit c1c06f3 with merge b05fd2a...

bors · 2021-01-29T00:58:34Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing b05fd2a to master...

Ensure sanity of all computed ABIs This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`. So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument. To achieve this, this re-lands the parts of rust-lang#80594 that got reverted in rust-lang#81388. To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default! Cc `@bjorn3` Fixes rust-lang#115845

Ensure sanity of all computed ABIs This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang/rust#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`. So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument. To achieve this, this re-lands the parts of rust-lang/rust#80594 that got reverted in rust-lang/rust#81388. To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default! Cc `@bjorn3` Fixes rust-lang/rust#115845

Fix abi for wasm-bindgen

4d2766e

bjorn3 added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 25, 2021

rust-highfive assigned alexcrichton Jan 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2021

bjorn3 mentioned this pull request Jan 25, 2021

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

Closed

Share wasm-bindgen compat abi selection code

e7a056f

bjorn3 force-pushed the wasm_bindgen_fix branch from 0d1a319 to e7a056f Compare January 25, 2021 20:33

bjorn3 added 3 commits January 26, 2021 11:09

Revert "Share wasm-bindgen compat abi selection code"

36df9c5

This reverts commit e7a056f.

Revert "Fix abi for wasm-bindgen"

ecbc661

This reverts commit 4d2766e.

Wasm-bindgen abi compat using cast_to

903c553

Revert "Wasm-bindgen abi compat using cast_to"

eb99ea5

This reverts commit 903c553.

This comment has been minimized.

Sign in to view

bjorn3 force-pushed the wasm_bindgen_fix branch from 9a3cec1 to a8da209 Compare January 26, 2021 13:31

Use PassMode::Direct for Abi::Aggregate by default

c1c06f3

bjorn3 force-pushed the wasm_bindgen_fix branch from a8da209 to c1c06f3 Compare January 26, 2021 13:50

pnkfelix requested a review from nikomatsakis January 28, 2021 15:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 29, 2021

bors merged commit b05fd2a into rust-lang:master Jan 29, 2021

rustbot added this to the 1.51.0 milestone Jan 29, 2021

bjorn3 mentioned this pull request Mar 14, 2021

Function arguments when returning a struct bytecodealliance/wasmtime#2726

Closed

bjorn3 deleted the wasm_bindgen_fix branch March 30, 2021 18:07

RalfJung mentioned this pull request Nov 1, 2023

Ensure sanity of all computed ABIs #117500

Merged

Manishearth mentioned this pull request Aug 23, 2024

Rust Wasm ABI treats two-field structs like scalars and doesn't require padding rust-diplomat/diplomat#657

Closed

Uh oh!

Conversation

bjorn3 commented Jan 25, 2021

Uh oh!

alexcrichton commented Jan 25, 2021

Uh oh!

bjorn3 commented Jan 25, 2021

Uh oh!

alexcrichton commented Jan 25, 2021

Uh oh!

bjorn3 commented Jan 25, 2021

Uh oh!

alexcrichton commented Jan 25, 2021

Uh oh!

rust-log-analyzer commented Jan 25, 2021

Uh oh!

bjorn3 commented Jan 25, 2021

Uh oh!

rust-log-analyzer commented Jan 25, 2021

Uh oh!

bjorn3 commented Jan 26, 2021

Uh oh!

bjorn3 commented Jan 26, 2021

Uh oh!

bjorn3 commented Jan 26, 2021

Uh oh!

This comment has been minimized.

alexcrichton commented Jan 26, 2021

Uh oh!

nikomatsakis commented Jan 28, 2021

Uh oh!

bors commented Jan 28, 2021

Uh oh!

bors commented Jan 28, 2021

Uh oh!

bors commented Jan 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants