Page MenuHomePhabricator

Adds an option "malign-pass-aggregate" to make the alignment of the struct and union parameters compatible with the default gcc
AbandonedPublic

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 adds an option to pass struct and union 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.Sep 18 2019, 8:02 PM
LiuChen3 updated this revision to Diff 220793.
LiuChen3 edited reviewers, added: wxiao3; removed: LiuChen3.
emaste added a subscriber: kib.Sep 24 2019, 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.Sep 24 2019, 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?

Oh, I see you just updated your patch months ago without ever mentioning that it was ready for review.

It sounds to me like GCC retroactively added a switch specifying which version of the ABI to follow on this point, somewhat confusingly called -malign-data. That's probably the right move here for us, too, especially since FreeBSD says they'd like to use it. That also means the condition of when to use your new logic will have to change; basically, we need a CodeGenOption for this that will default to the old ABI, and the driver will pass down a different default on Linux.

Oh, I see you just updated your patch months ago without ever mentioning that it was ready for review.

It sounds to me like GCC retroactively added a switch specifying which version of the ABI to follow on this point, somewhat confusingly called -malign-data. That's probably the right move here for us, too, especially since FreeBSD says they'd like to use it. That also means the condition of when to use your new logic will have to change; basically, we need a CodeGenOption for this that will default to the old ABI, and the driver will pass down a different default on Linux.

Thanks for review.
-malign-data is another topic. Just like what I said above, at least -malign-data will not affect the calling convention of struct and union. I agree with you that adding an option to control this new logi. I'll working on it.

Oh, I see you just updated your patch months ago without ever mentioning that it was ready for review.

It sounds to me like GCC retroactively added a switch specifying which version of the ABI to follow on this point, somewhat confusingly called -malign-data. That's probably the right move here for us, too, especially since FreeBSD says they'd like to use it. That also means the condition of when to use your new logic will have to change; basically, we need a CodeGenOption for this that will default to the old ABI, and the driver will pass down a different default on Linux.

Thanks for review.
-malign-data is another topic. Just like what I said above, at least -malign-data will not affect the calling convention of struct and union. I agree with you that adding an option to control this new logi. I'll working on it.

Oh, I see, sorry. Yes, I think a different option would be good. We can debate the name when the patch is ready.

LiuChen3 updated this revision to Diff 251290.Mar 18 2020, 11:59 PM
LiuChen3 retitled this revision from Fix i386 struct and union parameter alignment to Adds an option "malign-pass-aggregate" to make the alignment of the struct and union parameters compatible with the default gcc.
LiuChen3 edited the summary of this revision. (Show Details)

Add an option "malign-pass-aggregate" to compatible with gcc default passing struct and union.

Since the ABI this is trying to match is not documented literally anywhere, I think we need to have some confidence that what this implements is actually the same as what GCC does. While I wrote up what I think the algorithm is, without some sort of script to allow testing it against a bunch of examples, I wouldn't say I'm confident of its correctness.

I'm not sure if you can reverse-engineer what the alignment must have been from the assembly output, or from some debug flags. Or if maybe doing something silly like modifying the source to insert a printf would be the best method to test this.

clang/lib/CodeGen/TargetInfo.cpp
1542–1544

This comment isn't useful. While it may be what the System V ABI document says, that's clearly incorreect, and is not what the code is or should be doing. Please document what is actually implemented, instead.

1559

Also needs to call getTypeStackAlignInBytes?

1567

I think this is wrong and that it should only return Align. The computation of the alignment of the elements is only to see if their alignment is >= 16.

If the alignment of the elements' types is >= 16, but the alignment of the structure is less than the alignment of one of its elements (e.g. due to __attribute__ packed), we should return the alignment of the structure.

1570

If I understood GCC's algorithm correctly, I think this needs to come first?

clang/test/CodeGen/x86_32-align-linux.cpp
9

Confused me that this was a different X1 than in the test-case above. I'm not sure why the tests need to be duplicated here in a .cpp file in the first place?

rnk added inline comments.Mar 19 2020, 2:52 PM
clang/include/clang/Basic/LangOptions.def
353 ↗(On Diff #251290)

If only codegen needs to know, a CodeGenOption would be better.

clang/lib/CodeGen/TargetInfo.cpp
1554

Any time you crack open a record to look at the fields, the code is probably wrong the first time you write it. :( In this case, I suspect you are not looking at base classes. Consider:

struct A {
  MyAlignedType Field;
};
struct B : A {};
void passbyval(B o);
LiuChen3 marked 3 inline comments as done.Mar 19 2020, 7:14 PM

Since the ABI this is trying to match is not documented literally anywhere, I think we need to have some confidence that what this implements is actually the same as what GCC does. While I wrote up what I think the algorithm is, without some sort of script to allow testing it against a bunch of examples, I wouldn't say I'm confident of its correctness.

I'm not sure if you can reverse-engineer what the alignment must have been from the assembly output, or from some debug flags. Or if maybe doing something silly like modifying the source to insert a printf would be the best method to test this.

I think at least the initial patch is correct.

clang/include/clang/Basic/LangOptions.def
353 ↗(On Diff #251290)

The backend does not need this option information.

clang/lib/CodeGen/TargetInfo.cpp
1542–1544

Sorry I forget to change it.

1554

I'm not sure if I understand what you mean.

typedef __attribute__((aligned(16))) int alignedint16;
typedef __attribute__((aligned(64))) int alignedint64;
struct __attribute__((aligned(64))) X2 {
  struct  __attribute__((aligned(32))) {
    int a1;
    alignedint16 a2;
  } a;
  int b;
};
struct B : X2{};
void test(B b)
{
  std::cout << b.a.a2 << std::endl;
}

This can pass.

clang/test/CodeGen/x86_32-align-linux.cpp
9

Sorry that I don't know much about front-end tests. I thought class, struct and union all need to be tested.

I think at least the initial patch is correct.

I re-read your comment above, please ignore this sentence. Sorry for the noise.
My question now is that since we cannot guarantee that we are doing the right thing, is this patch necessary?

LiuChen3 marked 3 inline comments as done.Mar 20 2020, 2:02 AM
LiuChen3 added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
1559

I think this is enough.

struct __attribute__((aligned(16))) X6 {
 int x;
 struct X1 x1[5];
// alignedint64 y;
};

void test(int a, struct X6 x6)
{
  printf(%u\n", __alignof__(x6));
}

This will output 64.

1567

I write a test, I do n’t know if it matches your meaning.

struct __attribute__((aligned(4))) X6 {
 int x;
 alignedint64 y;
};

void test(int a, struct X6 x6)
{
  printf("%u\n", __alignof__(x6));
}

The output of gcc is 64.

If we use packed attribute:

struct __attribute__((packed)) X6 {
 int x;
 alignedint64 y;
};

Both gcc and clang with this patch output "1". And I found that the packed struct is not processed by this function.
So I think it should return the MaxAlignment .

1570

You mean it should be ?

if (MaxAlignment < 16)
  retrun 4
else
 return std::max(MaxAlignment, Align);
LiuChen3 marked an inline comment as done.Mar 23 2020, 5:29 PM
LiuChen3 added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
1570

I found that the test I wrote above was wrong. Sorry for the noise.

@wxiao3 @LiuChen3 Are you still looking at this or should it be abandoned?

LiuChen3 abandoned this revision.Thu, Oct 8, 7:48 PM

@wxiao3 @LiuChen3 Are you still looking at this or should it be abandoned?

I will abandon this patch for it is difficult to confirm the behavior of gcc.