[PATCH] D14980: PR18513: make gcc compatible layout for bit-fields with explicit aligned attribute

[John McCall via cfe-commits]

rjmccall added a comment.

I don't mean the actual layout used by Windows targets; I mean the layout used by the ms_struct pragma / attribute.


================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+    } else if (ExplicitFieldAlign)
+      FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 
----------------
It does still seem to be limited by platforms that ignore natural alignment, though, and by #pragma pack (2) and so on, so at least some of the modifications made to FieldAlign need to be reflected in ExplicitFieldAlign.

I had to spend some time trying to work out counter-examples, but I did convince myself that the effect of the fallback here is correct.  My concern was that there was a way we might round up to FieldAlign when we only needed to round up to ExplicitFieldAlign, which could matter if there were another properly-aligned position within the current storage unit.  However, the second clause of the first condition will only trigger at the very end of a storage unit, which means that there cannot be any other properly-aligned positions within it.

It might be wrong for zero-width bit-fields, though, since we'll always round up to FieldAlign instead of ExplicitFieldAlign.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1614
@@ -1610,3 +1613,3 @@
       UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset,
                                                      UnpackedFieldAlign);
   }
----------------
This should follow the same logic that you use for FieldOffset, unless I'm missing something.


http://reviews.llvm.org/D14980