Page MenuHomePhabricator

Fix i386 struct and union parameter alignment
Needs ReviewPublic

Authored by LiuChen3 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

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
1496 ↗(On Diff #195292)

@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.May 28 2019, 10:36 AM

Yes, LGTM.

This revision is now accepted and ready to land.May 28 2019, 10:36 AM
This revision was automatically updated to reflect the committed changes.
krytarowski added inline comments.
lib/CodeGen/TargetInfo.cpp
1501 ↗(On Diff #201840)

Darwin and BSD are not System V.

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

joerg added inline comments.May 30 2019, 4:50 AM
lib/CodeGen/TargetInfo.cpp
1501 ↗(On Diff #201840)

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.May 30 2019, 6:53 PM

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

RKSimon requested changes to this revision.Sep 6 2019, 8:24 AM

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

@wxiao3 Have you investigated this yet?

This revision now requires changes to proceed.Sep 6 2019, 8:24 AM

@RKSimon I'm busy with other stuff and my colleague: LiuChen3 will help finish the work.

LiuChen3 commandeered this revision.Wed, Sep 18, 8:02 PM
LiuChen3 updated this revision to Diff 220793.
LiuChen3 edited reviewers, added: wxiao3; removed: LiuChen3.
emaste added a subscriber: kib.Tue, Sep 24, 11:36 AM
In D60748#1499756, @dim wrote:

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 that said I think we'd expect to be able to mix gcc- and clang-built objects, and it seems this is addressing somewhat of a corner case?

kib added a comment.Tue, Sep 24, 12:05 PM

In fact, can we have an option controlling this ? Does it have anything to do with -malign-data gcc switch ?

We do want to be able to optionally generate code ABI-compatible with modern gcc, per user discretion.

In D60748#1681178, @kib wrote:

In fact, can we have an option controlling this ? Does it have anything to do with -malign-data gcc switch ?

We do want to be able to optionally generate code ABI-compatible with modern gcc, per user discretion.

I found -malign-data option only affects data alignment in data segment. -malign-data has three options: “compat”,“ abi” and “cacheline”. The default in GCC is ”compat,“ and clang’s behavior is consistent with "abi".
And the data alignment on stack and parameters Passing on stack is not affected. This patch only affects the alignment of passing parameter.
Should we add an option just like -malign-data?