Repository navigation
riscv64gc-unknown-linux-gnu extern "C" ABI violates repr(transparent) on unions #115481
Description
The types (i32, f32) and MaybeUninit<T> do not have the same ABI, as demonstrated by this testcase:
#![feature(rustc_attrs)]
type T = (i32, f32);
#[rustc_abi(debug)]
extern "C" fn test1(_x: T) {}
#[rustc_abi(debug)]
extern "C" fn test2(_x: std::mem::MaybeUninit<T>) {}
fn main() {}The first is
mode: Cast(
CastTarget {
prefix: [
Some(
Reg {
kind: Integer,
size: Size(4 bytes),
},
),
None,
None,
None,
None,
None,
None,
None,
],
rest: Uniform {
unit: Reg {
kind: Float,
size: Size(4 bytes),
},
total: Size(4 bytes),
},
attrs: ArgAttributes {
regular: (empty),
arg_ext: None,
pointee_size: Size(0 bytes),
pointee_align: None,
},
},
false,
),
The second is
mode: Cast(
CastTarget {
prefix: [
None,
None,
None,
None,
None,
None,
None,
None,
],
rest: Uniform {
unit: Reg {
kind: Integer,
size: Size(8 bytes),
},
total: Size(8 bytes),
},
attrs: ArgAttributes {
regular: (empty),
arg_ext: None,
pointee_size: Size(0 bytes),
pointee_align: None,
},
},
false,
),
Not sure whom to ping for RISC-V issues.
Activity
RalfJung commentedon Sep 2, 2023
I think the fix will have to be to treat repr(transparent) unions specially somewhere around here
rust/compiler/rustc_target/src/abi/call/riscv.rs
Lines 90 to 94 in 1bd0430
RalfJung commentedon Sep 3, 2023
Cc @msizanoen1
msizanoen1 commentedon Sep 3, 2023
@RalfJung I'm afraid that the semantics of
#[repr(transparent)]on unions are not clearly defined enough to figure out exactly how they should be specially treated (which variant should be used? etc.). Can you ping other people who have more knowledge on this? (Ifrepr(transparent)on unions turns out to be incompatible with ABIs that havestructcontent destructuring semantics that depends on the types of the field in thestruct(e.g. the RISC-V C psABI) then we definitely have a bigger problem.)RalfJung commentedon Sep 3, 2023
I have no idea who would know anything about RISC-V calling conventions. 🤷 and no other ABI is failing this test.
Basically, when the ABI description says "unions do X", then this should only apply to repr(C) and repr(Rust) unions. A repr(transparent) union must be treated exactly like its non-ZST field.
apiraino commentedon Sep 4, 2023
WG-prioritization assigning priority (Zulip discussion).
@rustbot label -I-prioritize +P-medium
4 remaining items
Auto merge of rust-lang#115372 - RalfJung:abi-assert-eq, r=davidtwco
Auto merge of rust-lang#115372 - RalfJung:abi-assert-eq, r=davidtwco
Auto merge of rust-lang#115372 - RalfJung:abi-assert-eq, r=davidtwco
Auto merge of #115372 - RalfJung:abi-assert-eq, r=davidtwco
rustc_target/riscv: Fix passing of transparent unions with only one n…
Rollup merge of rust-lang#115499 - msizanoen1:riscv-fix-transparent-u…
Unrolled build for rust-lang#115499
tgross35 commentedon Sep 19, 2023
@RalfJung now that this is fixed, is there a CI test to catch regressions?
RalfJung commentedon Sep 19, 2023
There is a test in
tests/ui/abi/compatibility.rsbut we don't run tests for riscv, since it's a tier 2 target.RalfJung commentedon Sep 19, 2023
Also see #115609 for another ABI issue that affects riscv (and more).
tgross35 commentedon Sep 19, 2023
Are tier 2 tests run once before release or anything like that? It seems like at least a subset of tests (this one) are important enough that we should raise a red flag if we brake them somehow on any targets. But that doesn't fit well into the tiering structure...
RalfJung commentedon Sep 20, 2023
I'm not aware of any process like that. Part of the point of being tier 2 is not to burden all contributors with keeping the target working, is it? If we do want to accept that burden, we could consider running something like
./x.py test codegen && ./x.py test ui --test-args abifor tier 2 targets.But having this discussion in a closed issue makes little sense. Do you want to open a new issue or Zulip thread for discussing tier 2 target test coverage?
RalfJung commentedon Sep 21, 2023
Inspired by #116004 I found a way to have a test for this: #116030.
tgross35 commentedon Sep 21, 2023
Awesome! Thanks for the update here