[PATCH] D60748: Adds an option "malign-pass-aggregate" to make the alignment of the struct and union parameters compatible with the default gcc

[LiuChen via Phabricator via cfe-commits]

LiuChen3 marked 3 inline comments as done.
LiuChen3 added a comment.

In D60748#1931440 <https://reviews.llvm.org/D60748#1931440>, @jyknight wrote:

> 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.



================
Comment at: clang/include/clang/Basic/LangOptions.def:353
 
+VALUE_LANGOPT(AlignPassingAggregate, 1, 0, "Compatible with gcc default passing struct and union (x86 only).")
+
----------------
rnk wrote:
> If only codegen needs to know, a CodeGenOption would be better.
The backend does not need this option information.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1556
+
+      for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
+           i != e; ++i) {
----------------
rnk wrote:
> 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);
> ```
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1542-1544
+    // i386 System V ABI 2.1: Structures and unions assume the alignment of their
+    // most strictly aligned component.
+    //
----------------
jyknight wrote:
> 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.
Sorry I forget to change it.


================
Comment at: clang/test/CodeGen/x86_32-align-linux.cpp:9
+
+class __attribute__((aligned(64))) X1 {
+  class  __attribute__((aligned(32))) {
----------------
jyknight wrote:
> 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?
Sorry that I don't know much about front-end tests. I thought class, struct and union all need to be tested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748