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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.



================
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.
+    //
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1559
+          if (const auto *AT = QT->getAsArrayTypeUnsafe())
+            TempAlignment = getContext().getTypeAlign(AT->getElementType()) / 8;
+          else // recursively to get each type's alignment
----------------
Also needs to call getTypeStackAlignInBytes?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1567
+      if (MaxAlignment >= 16)
+        return std::max(MaxAlignment, Align);
+      else // return 4 when all the type alignments less than 16 bytes
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1570
+        return 4;
+    } else if (Align < 16)
+      return MinABIStackAlignInBytes;
----------------
If I understood GCC's algorithm correctly, I think this needs to come first?


================
Comment at: clang/test/CodeGen/x86_32-align-linux.cpp:9
+
+class __attribute__((aligned(64))) X1 {
+  class  __attribute__((aligned(32))) {
----------------
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?


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

https://reviews.llvm.org/D60748