From 28d152935e5f62ecbb6d2c99865de39c1993189b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 7 Sep 2023 16:04:22 +0200 Subject: [PATCH] the wasm ABI behavior is a bug --- compiler/rustc_codegen_llvm/src/abi.rs | 15 ++++++--------- compiler/rustc_target/src/abi/call/mod.rs | 7 +++++-- compiler/rustc_target/src/abi/call/wasm.rs | 4 ++++ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/abi.rs b/compiler/rustc_codegen_llvm/src/abi.rs index 1e72b7db1e4..64587f98b8a 100644 --- a/compiler/rustc_codegen_llvm/src/abi.rs +++ b/compiler/rustc_codegen_llvm/src/abi.rs @@ -351,15 +351,12 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> { // guarnateeing that we generate ABI-compatible LLVM IR. Things get tricky for // aggregates... if matches!(arg.layout.abi, abi::Abi::Aggregate { .. }) { - // This is the most critical case for ABI compatibility, since - // `immediate_llvm_type` will use `layout.fields` to turn this Rust type - // into an LLVM type. ABI-compatible Rust types can have different `fields`, - // so we need to be very sure that LLVM wil treat those different types in - // an ABI-compatible way. Mostly we do this by disallowing - // `PassMode::Direct` for aggregates, but we actually do use that mode on - // wasm. wasm doesn't have aggregate types so we are fairly sure that LLVM - // will treat `{ i32, i32, i32 }` and `{ { i32, i32, i32 } }` the same way - // for ABI purposes. + // This really shouldn't happen, since `immediate_llvm_type` will use + // `layout.fields` to turn this Rust type into an LLVM type. This means all + // sorts of Rust type details leak into the ABI. However wasm sadly *does* + // currently use this mode so we have to allow it -- but we absolutely + // shouldn't let any more targets do that. + // (Also see .) assert!( matches!(&*cx.tcx.sess.target.arch, "wasm32" | "wasm64"), "`PassMode::Direct` for aggregates only allowed on wasm targets\nProblematic type: {:#?}", diff --git a/compiler/rustc_target/src/abi/call/mod.rs b/compiler/rustc_target/src/abi/call/mod.rs index 42483b7c6a5..d4619bb6753 100644 --- a/compiler/rustc_target/src/abi/call/mod.rs +++ b/compiler/rustc_target/src/abi/call/mod.rs @@ -36,7 +36,10 @@ pub enum PassMode { Ignore, /// Pass the argument directly. /// - /// The argument has a layout abi of `Scalar`, `Vector` or in rare cases (e.g. on wasm) `Aggregate`. + /// The argument has a layout abi of `Scalar` or `Vector`. + /// Unfortunately due to past mistakes, in rare cases on wasm, it can also be `Aggregate`. + /// This is bad since it leaks LLVM implementation details into the ABI. + /// (Also see .) Direct(ArgAttributes), /// Pass a pair's elements directly in two arguments. /// @@ -527,7 +530,7 @@ impl<'a, Ty> ArgAbi<'a, Ty> { scalar_attrs(&layout, b, a.size(cx).align_to(b.align(cx).abi)), ), Abi::Vector { .. } => PassMode::Direct(ArgAttributes::new()), - // The `Aggregate` ABI is almost always adjusted later. + // The `Aggregate` ABI should always be adjusted later. Abi::Aggregate { .. } => PassMode::Direct(ArgAttributes::new()), }; ArgAbi { layout, mode } diff --git a/compiler/rustc_target/src/abi/call/wasm.rs b/compiler/rustc_target/src/abi/call/wasm.rs index 0eb2309ecb2..796b752ff9d 100644 --- a/compiler/rustc_target/src/abi/call/wasm.rs +++ b/compiler/rustc_target/src/abi/call/wasm.rs @@ -61,6 +61,10 @@ where /// The purpose of this ABI is for matching the WebAssembly standard. This /// intentionally diverges from the C ABI and is specifically crafted to take /// advantage of LLVM's support of multiple returns in WebAssembly. +/// +/// This ABI is *bad*! It uses `PassMode::Direct` for `abi::Aggregate` types, which leaks LLVM +/// implementation details into the ABI. It's just hard to fix because ABIs are hard to change. +/// Also see . pub fn compute_wasm_abi_info(fn_abi: &mut FnAbi<'_, Ty>) { if !fn_abi.ret.is_ignore() { classify_ret(&mut fn_abi.ret);