Page MenuHomePhabricator

Fix i386 struct and union parameter alignment
AcceptedPublic

Authored by wxiao3 on Apr 15 2019, 7:33 PM.

Details

Summary

According to i386 System V ABI 2.1: Structures and unions assume the alignment
of their most strictly aligned component. But current implementation always
takes them as 4-byte aligned which will result in incorrect code, e.g:

 1 #include <immintrin.h>
 2 typedef union {
 3         int d[4];
 4         __m128 m;
 5 } M128;
 6 extern void foo(int, ...);
 7 void test(void)
 8 {
 9   M128 a;
10   foo(1, a);
11   foo(1, a.m);
12 }

The first call (line 10) takes the second arg as 4-byte aligned while the
second call (line 11) takes the second arg as 16-byte aligned. There is
oxymoron for the alignment of the 2 calls because they should be the same.

This patch fixes the bug by following i386 System V ABI and apply it to Linux
only since other System V OS (e.g Darwin, PS4 and FreeBSD) don't want to spend
any effort dealing with the ramifications of ABI breaks at present.

Diff Detail

Repository
rC Clang

Event Timeline

wxiao3 created this revision.Apr 15 2019, 7:33 PM
rnk added a reviewer: rjmccall.
rnk added a comment.Apr 19 2019, 11:06 AM

This is, obviously, an ABI break. I think Sony would probably want you to preserve the existing behavior of intentionally underaligning such byval parameters for PS4 targets. +@rjmccall in case he has other ABI thoughts.

lib/CodeGen/TargetInfo.cpp
1507

@rjmccall, does this comment need updating in an AVX world?

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

Agreed.

wxiao3 updated this revision to Diff 196967.Apr 27 2019, 7:43 AM
wxiao3 edited the summary of this revision. (Show Details)

Ok, I have excluded Darwin and PS4 for the changes.
The fix mainly targets at Linux so that we can compile a project with parts by GCC and parts by LLVM given that they follow the same ABI.

wxiao3 updated this revision to Diff 199231.May 13 2019, 3:34 AM
wxiao3 updated this revision to Diff 199232.

Any other comments?

dim added subscribers: emaste, dim.May 13 2019, 3:57 AM

Please also exclude FreeBSD from these changes, since we care a lot about backwards compatibility, and specifically about alignment requirements. (We have run into many issues in our ports collection where upstream assumes everything is 16-byte aligned on i386, which is *NOT* ABI compliant.)

dim added a comment.May 13 2019, 3:58 AM

In fact, it is probably better to turn the OS check around, e.g. *only* increase the alignment for Linux, and nowhere else.

wxiao3 updated this revision to Diff 199360.May 13 2019, 8:28 PM
wxiao3 edited the summary of this revision. (Show Details)

Yes, the ABI bug will cause SEGV in Linux where a lot of libraries are built by GCC.
I have restricted the fix to Linux only in the latest revision.

Any other comments?

Ok for merge now?

rjmccall accepted this revision.Tue, May 28, 10:36 AM

Yes, LGTM.

This revision is now accepted and ready to land.Tue, May 28, 10:36 AM
This revision was automatically updated to reflect the committed changes.
krytarowski added inline comments.
lib/CodeGen/TargetInfo.cpp
1501

Darwin and BSD are not System V.

CC: @joerg @mgorny for NetBSD. Do we need to do something here?

joerg added inline comments.Thu, May 30, 4:50 AM
lib/CodeGen/TargetInfo.cpp
1501

It's a misnomer. The ABI standard for i386 was the SysV ABI before GNU decided to silently break the stack alignment and calling it the new ABI. That said, I'm not sure how much copy-by-value of vector types actually happens and that's the only situation affected by this.

I don't think this was correct (where by "correct", there, I mean "what GCC does", as this patch is intended to match GCC behavior).

I think this change may well break more cases than it fixes, so IMO, this should be reverted, until it's implemented properly.

Consider one example:

#include <immintrin.h>

typedef __attribute__((aligned(16))) int alignedint;

struct __attribute__((aligned(64))) X {
    int x;
//    alignedint y;
//    __m128 y;
};
void g(int x, struct X);

_Static_assert(_Alignof(struct X) == 64);

struct X gx;

void f() {
    g(1, gx);
}

Note that when compiling this as is GCC does _not_ align X when calling g(). But, as of this change, now clang does. If you uncomment either the __m128 or alignedint lines, and now GCC aligns to 64 bytes too.

This is because GCC's algorithm is a whole lot more complex than what you've implemented. See its function ix86_function_arg_boundary.

The way I interpret GCC, it's doing effectively the following:
StackAlignmentForType(T):

  1. If T's alignment is < 16 bytes, return 4.
  2. If T is a struct/union/array type, then:
    • recursively call StackAlignmentForType() on each member's type (note -- this ignores any attribute((aligned(N))) directly on the fields of a struct, but not those that appear on typedefs, or the underlying types).
    • If all of those calls return alignments < 16, then return 4.
  3. Otherwise, return the alignment of T.
This revision is now accepted and ready to land.Thu, May 30, 6:53 PM

Thanks for the information!
We have reverted the patch and will resubmit it when we have a complete fix.