Fix abi for wasm-bindgen

[bjorn3]

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

r? @alexcrichton

[alexcrichton]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[bjorn3]

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.

[bjorn3]

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]

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

[nikomatsakis]

@bors r+ p=1

[bors]

📌 Commit c1c06f3 has been approved by nikomatsakis

[bors]

⌛ Testing commit c1c06f3 with merge b05fd2a...

[bors]

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