Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More ABI size and alignment fixes #12145

Merged
merged 8 commits into from
Aug 19, 2022
Merged

More ABI size and alignment fixes #12145

merged 8 commits into from
Aug 19, 2022

Conversation

andrewrk
Copy link
Member

Continuation of #12127. After this branch, there are no more instances of disagreements between LLVM ABI size and Zig ABI size when building stage3. However, it did not solve #11450.

The remaining ABI size mismatches that happen during behavior tests are:

error(codegen): when lowering behavior.ptrcast.testReinterpretExternStructAsExternStruct.S2, Zig ABI size = 6 but LLVM ABI size = 8
error(codegen): when lowering behavior.ptrcast.test.lower reinterpreted comptime field ptr (with under-aligned fields).S, Zig ABI size = 6 but LLVM ABI size = 8
error(codegen): when lowering builtin.Type.Fn.Param, Zig ABI size = 2 but LLVM ABI size = 3
error(codegen): when lowering [2]builtin.Type.Fn.Param, Zig ABI size = 4 but LLVM ABI size = 6

@andrewrk
Copy link
Member Author

andrewrk commented Jul 17, 2022

Hmm, here's the dilemma:

This branch makes i128 ABI size and alignment agree with LLVM (8), which I think is an OK decision. However the previous value agreed with Clang (16), which made the alignment of extern structs and 128-bit integers correct for -ofmt=c and zig with no differences. So with this change, the C backend tests are failing because C needs 128-bit integers to be 16-byte aligned when we are making them 8-byte aligned only. In order to fix this, we would need to do a major refactor of moving ObjectFormat into std.Target, like this:

--- a/lib/std/target.zig
+++ b/lib/std/target.zig
@@ -9,6 +9,7 @@ pub const Target = struct {
     cpu: Cpu,
     os: Os,
     abi: Abi,
+    ofmt: ObjectFormat,
 
     pub const Os = struct {
         tag: Tag,

So those are the two choices we have:

  1. Go back to u128 being 16 bytes aligned, how it is in master branch, and accept the fact that LLVM ABI size of i128 will be different than Zig ABI size of i128. I think this is actually fine, and maybe our size safety check is not as helpful as previously thought. This is the status quo for Clang.
  2. Do a big reworking to put ofmt into Target, and have -ofmt=c cause there to be differences in ABI alignment and ABI size of 128-bit integers.

@topolarity
Copy link
Contributor

topolarity commented Jul 17, 2022

Go back to u128 being 16 bytes aligned, how it is in master branch, and accept the fact that LLVM ABI size of i128 will be different than Zig ABI size of i128. I think this is actually fine, and maybe our size safety check is not as helpful as previously thought. This is the status quo for Clang.

I think we'd have to oversize any alloca for types like u293 where alignment affects their padded size, but otherwise I think this would be OK. We might also have to do some rigging with how arguments are passed to make sure alignment is respected.

Edit: We'll need some layout fix-ups, too. For example, the lowered type for an extern struct currently assumes that LLVM's ABI size matches our own

What about FFI implications? Even Clang doesn't seem to be compliant with the SysV ABI on x86-64 for i128 arguments (rust-lang/rust#54341 (comment)). However, on platforms like AArch64 it looks like the 16-byte alignment is expected by the system ABI, and clang/gcc respect it.

@topolarity
Copy link
Contributor

topolarity commented Jul 17, 2022

There's another problem with i128 on x86: LLVM will lower libcalls for us with the wrong calling convention (https://bugs.llvm.org/show_bug.cgi?id=50198)

So even if we manage to enforce align(16) on the Zig side of things, we'd be forced to build compiler-rt with the ABI that LLVM expects, which is incorrect 😕

The right outcome really seems like it would be a fix upstream in LLVM (either to update the alignment of i128 to match the system ABI, or else a hack to lower the libcalls correctly, similar to what was done for Windows in https://reviews.llvm.org/D110413)

There still more instances of this check being tripped during behavior
tests that we need to fix before we can enable this.
 * riscv64: adjust alignment and size of 128-bit integers.
 * take ofmt=c into account for ABI alignment of 128-bit integers and
   structs.
 * Type: make packed struct support intInfo
 * fix f80 alignment for i386-windows-msvc
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Aug 19, 2022
@andrewrk andrewrk merged commit 2ccaa54 into master Aug 19, 2022
@andrewrk andrewrk deleted the fixes branch August 19, 2022 08:56
@topolarity topolarity mentioned this pull request Oct 22, 2022
@matklad
Copy link
Contributor

matklad commented Mar 20, 2024

This branch makes i128 ABI size and alignment agree with LLVM (8)

According to rust-lang/rust#116672, LLVM 18 will incerase u128 alignment to 16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants