rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process.
#103830
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // ignore-x86 | ||
| // ignore-aarch64 | ||
| // ignore-aarch64_be | ||
| // ignore-arm | ||
| // ignore-armeb | ||
| // ignore-avr | ||
| // ignore-bpfel | ||
| // ignore-bpfeb | ||
| // ignore-hexagon | ||
| // ignore-mips | ||
| // ignore-mips64 | ||
| // ignore-msp430 | ||
| // ignore-powerpc64 | ||
| // ignore-powerpc64le | ||
| // ignore-powerpc | ||
| // ignore-r600 | ||
| // ignore-amdgcn | ||
| // ignore-sparc | ||
| // ignore-sparcv9 | ||
| // ignore-sparcel | ||
| // ignore-s390x | ||
| // ignore-tce | ||
| // ignore-thumb | ||
| // ignore-thumbeb | ||
| // ignore-xcore | ||
| // ignore-nvptx | ||
| // ignore-nvptx64 | ||
| // ignore-le32 | ||
| // ignore-le64 | ||
| // ignore-amdil | ||
| // ignore-amdil64 | ||
| // ignore-hsail | ||
| // ignore-hsail64 | ||
| // ignore-spir | ||
| // ignore-spir64 | ||
| // ignore-kalimba | ||
| // ignore-shave | ||
|
There was a problem hiding this comment. This looks really fragile. I'd suggest to implement this in the same way as https://github.com/rust-lang/rust/blob/master/tests/codegen/abi-repr-ext.rs, i.e. a no_core test explicitly specifying targets. That should also allow you to test a non-windows target for the issue below. |
||
| // | ||
| // Tests that `byval` alignment is properly specified (#80127). | ||
| // The only targets that use `byval` are m68k, wasm, x86-64, and x86. Note that | ||
| // x86 has special rules (see #103830), and it's therefore ignored here. | ||
|
|
||
| #[repr(C)] | ||
| #[repr(align(16))] | ||
| struct Foo { | ||
| a: [i32; 16], | ||
| } | ||
|
|
||
| extern "C" { | ||
| // CHECK: declare void @f({{.*}}byval(%Foo) align 16{{.*}}) | ||
|
There was a problem hiding this comment. The Windows x64 ABI mandates passing ; Function Attrs: uwtable
declare void @f(ptr noalias nocapture noundef align 16 dereferenceable(64)) unnamed_addr #1Can we conditionally check for |
||
| fn f(foo: Foo); | ||
|
There was a problem hiding this comment. You have to exceed a certain number of parameters for the original bug to surface. Enough to fill up the registers. I think the function signature in #80127 was the minimal repro; I remember trying to minimize it further and having not much luck. I'm trying to use godbolt to figure it out but I'm having frustrating usability problems, and my personal device isn't x86_64 -_- There was a problem hiding this comment. I think testing for the align attribute directly is better, because the test case in #80127 might randomly start to succeed if future changes to regalloc cause things to accidentally align, no? There was a problem hiding this comment. If it's a regression test it should never start to succeed, it could only ever start to fail right? I guess I'm confused by what you're saying. Also, my memory is quite fuzzy, but I thought that part of the underlying cause of the bug was that our logic for choosing how to pass args on x86_64 was wrong for stack arguments (which would require your test to have enough arguments to get to that point I think). But judging by you saying "I think testing for the align attribute directly is better", is it just that we were never putting There was a problem hiding this comment. Idk, I think I fundamentally don't trust a test that just checks the LLVM IR, because I don't understand it well enough for that to be convincing over a real integration test that proves that the C code can operate with the Rust code. But I recognize that that's probably quite hard to test in the rust repo. Perhaps someone else knows a way? There was a problem hiding this comment.
I'm saying it could get broken accidentally but we wouldn't notice it because it could accidentally start to pass. I can add an interop test though. There was a problem hiding this comment. That would be awesome. Thank you for tackling this bug that has existed for so so long 🙏🙏 |
||
| } | ||
|
|
||
| pub fn main() { | ||
| unsafe { f(Foo { a: [1; 16] }) } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| include ../tools.mk | ||
|
|
||
| all: $(call NATIVE_STATICLIB,test) | ||
| $(RUSTC) test.rs | ||
| $(call RUN,test) || exit 1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #include <assert.h> | ||
| #include <stdbool.h> | ||
| #include <stdint.h> | ||
| #include <string.h> | ||
|
|
||
| struct TwoU64s | ||
| { | ||
| uint64_t a; | ||
| uint64_t b; | ||
| } __attribute__((aligned(16))); | ||
|
|
||
| struct BoolAndU32 | ||
| { | ||
| bool a; | ||
| uint32_t b; | ||
| }; | ||
|
|
||
| int32_t many_args( | ||
| void *a, | ||
| void *b, | ||
| const char *c, | ||
| uint64_t d, | ||
| bool e, | ||
| struct BoolAndU32 f, | ||
| void *g, | ||
| struct TwoU64s h, | ||
| void *i, | ||
| void *j, | ||
| void *k, | ||
| void *l, | ||
| const char *m) | ||
| { | ||
| assert(strcmp(m, "Hello world") == 0); | ||
| return 0; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| // Issue #80127: Passing structs via FFI should work with explicit alignment. | ||
|
|
||
| use std::ffi::CString; | ||
| use std::ptr::null_mut; | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| #[repr(C)] | ||
| #[repr(align(16))] | ||
| pub struct TwoU64s { | ||
| pub a: u64, | ||
| pub b: u64, | ||
| } | ||
|
|
||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct BoolAndU32 { | ||
| pub a: bool, | ||
| pub b: u32, | ||
| } | ||
|
|
||
| #[link(name = "test", kind = "static")] | ||
| extern "C" { | ||
| fn many_args( | ||
| a: *mut (), | ||
| b: *mut (), | ||
| c: *const i8, | ||
| d: u64, | ||
| e: bool, | ||
| f: BoolAndU32, | ||
| g: *mut (), | ||
| h: TwoU64s, | ||
| i: *mut (), | ||
| j: *mut (), | ||
| k: *mut (), | ||
| l: *mut (), | ||
| m: *const i8, | ||
| ) -> i32; | ||
| } | ||
|
|
||
| fn main() { | ||
| let two_u64s = TwoU64s { a: 1, b: 2 }; | ||
| let bool_and_u32 = BoolAndU32 { a: true, b: 3 }; | ||
| let string = CString::new("Hello world").unwrap(); | ||
| unsafe { | ||
| many_args( | ||
| null_mut(), | ||
| null_mut(), | ||
| null_mut(), | ||
| 4, | ||
| true, | ||
| bool_and_u32, | ||
| null_mut(), | ||
| two_u64s, | ||
| null_mut(), | ||
| null_mut(), | ||
| null_mut(), | ||
| null_mut(), | ||
| string.as_ptr(), | ||
| ); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.