PR18513: make gcc compatible layout for bit-fields with explicit aligned attribute

[DmitryPolukhin]

Summary

Fix binary compatibility issue with GCC.

[DmitryPolukhin]

Friendly ping, any comments about this patch?

[rsmith]

GCC's behavior (aligned on a field specifies the alignment of the start of that field) makes a little more sense to me than Clang's behavior (the type and alignment of a field specify a flavour of storage unit, and the field goes in the next such storage unit that it fits into), but both seem defensible.

John, are we intentionally deviating from GCC's behaviour here?

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41120)

You should round up to FieldAlign here.

[rjmccall]

Well, this is a really nasty bug.

Please check how this interacts with #pragma pack, which normally takes precedence over even explicit alignment attributes. If that's the case here, then what you really want to do is just add the same "ExplicitFieldAlign ||" clause to the main computation that you do to the diagnostic computation.

[rjmccall]

In D14980#298754, @rsmith wrote:

GCC's behavior (aligned on a field specifies the alignment of the start of that field) makes a little more sense to me than Clang's behavior (the type and alignment of a field specify a flavour of storage unit, and the field goes in the next such storage unit that it fits into), but both seem defensible.

Are you saying that aligned on a bit-field always starts new storage on GCC?

John, are we intentionally deviating from GCC's behaviour here?

No. I consider this to be GCC's extension, with which we are required to be ABI-compatible. This is just a bug.

[DmitryPolukhin]

Added more testcases to cover combination of explicit alignment of bit-filed with packed attribute. Also added testing on ARM for bit-filed layout test.

[DmitryPolukhin]

I added more testcases and they all pass identically on GCC and clang with my patch. Please let me know if you think, that some cases are not covered or doesn't work with my patch. Perhaps we can reduce number of test-cases because some of them almost duplicates but for now I added them all.

As for MS compatibility mode, I think we shouldn't worry about them because MS does't support attrbute(aligned) and #pragma pack seems to have no influence on bit-filed layout as far as I tested. If you think that attrbute(aligned) should work in MS compatibility mode and use GCC semantic, please let me know, I'll update my patch accordingly.

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41120)

Unfortunately, I cannot use FieldAlign here because FieldAlign = max(ExplicitFieldAlign, alignof(field_type)). But GCC allows to specify alignment less than normal type alignment for bit-field, see 'struct g2' case in my testcase (it will fail if I round up to FieldAlign here).

[rjmccall]

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

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41481)

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.

1614 ↗

(On Diff #41481)

This should follow the same logic that you use for FieldOffset, unless I'm missing something.

[DmitryPolukhin]

Fixed logic for warning calculation and added even more test-cases.

[DmitryPolukhin]

This CL doesn't changes anything for ms_struct cases and ms_struct seems to be broken for bit-fields even for very simple cases so filed separate bug https://llvm.org/bugs/show_bug.cgi?id=25707

PTAL

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41481)

I think compiler can ignore natural type alignment here because it is bit-field and therefore compiler has to generate shift/mask sequence for accessing unaligned field anyway. I tried examples like:

struct attribute((packed)) B {

char AField;
__attribute__((aligned(1))) __int128 i : 124;
char BField;

};

Both GCC and Clang with my patch generates the same result on x86 and ARM:
sizeof(struct B) = 18
offsetof(struct B, BField) = 17
__alignof(struct B) = 1

Also tried zero-width bit-filed, my patch works fine. So I was not able to find arch that requires round up for ExplicitFieldAlign and what is required. All examples that I can think of work identical on GCC and Clang.

[rjmccall]

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41611)

Be sure to test specifically with an APCS ARM target; newer ARM ABIs don't ignore bit-field layout. Sorry, I should have been more explicit about that. (You can figure this stuff out by looking at the various targets in Targets.cpp that set UseBitFieldTypeAlignment.)

[DmitryPolukhin]

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41611)

'-triple=arm-apcs-gnu' does give difference with GCC '-target=arm -mabi=apcs-gnu -mfloat-abi=softfp'. But even without my patch I see bunch of difference in layout (i.e. structs a2, a3, c, d, s0). So I just disabled my changes for APCS because it requires more attention than just fix explicit alignment case and I'm not sure that it is used enough nowadays.

[rjmccall]

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41748)

No, that's what I meant by needing a rebuilt GCC; GCC apparently doesn't apply the APCS struct layout rules correctly when you just set -mabi, because the GCC design configures those things at build time.

I agree that it's most reasonable not to worry about it; however, I wouldn't disable your changes under APCS, because that suggests something misleading: it suggests you actually did the investigation to decide that APCS layout ignores explicit alignment. I find it unlikely that GCC's APCS layout completely ignores these attributes.

[DmitryPolukhin]

lib/AST/RecordLayoutBuilder.cpp

1606 ↗

(On Diff #41748)

So do you suggest just add TODO here and remove checks for APCS as it was in previous patch set?

[DmitryPolukhin]

Added TODO.

[rjmccall]

Yes, I think that's a reasonable approach.

[DmitryPolukhin]

Added TODO, any other comments or suggestions?

[DmitryPolukhin]

John and Richard,

I think this patch fixes important ABI compatibility issue with GCC and if there are no more comments, I think it makes sense to commit it. Could you please approve this CL?

Thanks,
Dmitry

[DmitryPolukhin]

Friendly ping.

I don't think this change makes APCS mode worse. As an alternative I could return to the variant that doesn't change anything for APCS case. Please let me know if APCS case must be resolved and TODO is not enough for committing this change.

[DmitryPolukhin]

John and Richard,

I would like to proceed with this patch one way or another. If this patch cannot be accepted in upstream, I'll discard it. On the other hand I'm ready to improve this patch further if it is OK in principle but needs more work. Please let me know how you would like to proceed?

Thanks,
Dmitry

[rjmccall]

Sorry, holidays. I'm comfortable with taking this patch as-is.

cfe/trunk/lib/AST/RecordLayoutBuilder.cpp

void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   // Remember the alignment we would have used if the field were not packed.
   unsigned UnpackedFieldAlign = FieldAlign;
 
   // Ignore the field alignment if the field is packed unless it has zero-size.
   if (!IsMsStruct && FieldPacked && FieldSize != 0)
     FieldAlign = 1;
 
   // But, if there's an 'aligned' attribute on the field, honor that.
-  if (unsigned ExplicitFieldAlign = D->getMaxAlignment()) {
+  unsigned ExplicitFieldAlign = D->getMaxAlignment();
+  if (ExplicitFieldAlign) {
     FieldAlign = std::max(FieldAlign, ExplicitFieldAlign);
     UnpackedFieldAlign = std::max(UnpackedFieldAlign, ExplicitFieldAlign);
   }
 
   // But, if there's a #pragma pack in play, that takes precedent over
   // even the 'aligned' attribute, for non-zero-width bitfields.
   if (!MaxFieldAlignment.isZero() && FieldSize) {
     unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment);
if (IsMsStruct) {
     // #pragma pack, with any value, suppresses the insertion of padding.
     bool AllowPadding = MaxFieldAlignment.isZero();
 
     // Compute the real offset.
     if (FieldSize == 0 || 
         (AllowPadding &&
          (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) {
       FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign);
+    } else if (ExplicitFieldAlign) {
+      // TODO: figure it out what needs to be done on targets that don't honor
+      // bit-field type alignment like ARM APCS ABI.
+      FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
     }
 
     // Repeat the computation for diagnostic purposes.
     if (FieldSize == 0 ||
         (AllowPadding &&
          (UnpackedFieldOffset & (UnpackedFieldAlign-1)) + FieldSize > TypeSize))
       UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset,
                                                      UnpackedFieldAlign);
+    else if (ExplicitFieldAlign)
+      UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset,
+                                                     ExplicitFieldAlign);
   }
 
   // If we're using external layout, give the external layout a chance
   // to override this information.
   if (UseExternalLayout)
     FieldOffset = updateExternalFieldOffset(D, FieldOffset);
 
   // Okay, place the bitfield at the calculated offset.

cfe/trunk/test/Sema/bitfield-layout.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=arm-linux-gnueabihf
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=aarch64-linux-gnu
 // expected-no-diagnostics
+#include <stddef.h>
 
-#define CHECK_SIZE(kind, name, size) extern int name##1[sizeof(kind name) == size ? 1 : -1];
-#define CHECK_ALIGN(kind, name, size) extern int name##2[__alignof(kind name) == size ? 1 : -1];
+#define CHECK_SIZE(kind, name, size) \
+  extern int name##_1[sizeof(kind name) == size ? 1 : -1];
+#define CHECK_ALIGN(kind, name, size) \
+  extern int name##_2[__alignof(kind name) == size ? 1 : -1];
+#define CHECK_OFFSET(kind, name, member, offset) \
+  extern int name##_3[offsetof(kind name, member) == offset ? 1 : -1];
 
 // Zero-width bit-fields
 struct a {char x; int : 0; char y;};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, a, 8)
+CHECK_ALIGN(struct, a, 4)
+#else
 CHECK_SIZE(struct, a, 5)
 CHECK_ALIGN(struct, a, 1)
+#endif
 
 // Zero-width bit-fields with packed
 struct __attribute__((packed)) a2 { short x : 9; char : 0; int y : 17; };
 CHECK_SIZE(struct, a2, 5)
 CHECK_ALIGN(struct, a2, 1)
 
 // Zero-width bit-fields at the end of packed struct
 struct __attribute__((packed)) a3 { short x : 9; int : 0; };
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, a3, 4)
+CHECK_ALIGN(struct, a3, 4)
+#else
 CHECK_SIZE(struct, a3, 4)
 CHECK_ALIGN(struct, a3, 1)
+#endif
 
 // For comparison, non-zero-width bit-fields at the end of packed struct
 struct __attribute__((packed)) a4 { short x : 9; int : 1; };
 CHECK_SIZE(struct, a4, 2)
 CHECK_ALIGN(struct, a4, 1)
 
 union b {char x; int : 0; char y;};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(union, b, 4)
+CHECK_ALIGN(union, b, 4)
+#else
 CHECK_SIZE(union, b, 1)
 CHECK_ALIGN(union, b, 1)
+#endif
 
 // Unnamed bit-field align
 struct c {char x; int : 20;};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, c, 4)
+CHECK_ALIGN(struct, c, 4)
+#else
 CHECK_SIZE(struct, c, 4)
 CHECK_ALIGN(struct, c, 1)
+#endif
 
 union d {char x; int : 20;};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(union, d, 4)
+CHECK_ALIGN(union, d, 4)
+#else
 CHECK_SIZE(union, d, 3)
 CHECK_ALIGN(union, d, 1)
+#endif
 
 // Bit-field packing
 struct __attribute__((packed)) e {int x : 4, y : 30, z : 30;};
 CHECK_SIZE(struct, e, 8)
 CHECK_ALIGN(struct, e, 1)
 
 // Alignment on bit-fields
 struct f {__attribute((aligned(8))) int x : 30, y : 30, z : 30;};
 CHECK_SIZE(struct, f, 24)
 CHECK_ALIGN(struct, f, 8)
 
 // Large structure (overflows i32, in bits).
 struct s0 {
   char a[0x32100000];
   int x:30, y:30;
 };
 
 CHECK_SIZE(struct, s0, 0x32100008)
 CHECK_ALIGN(struct, s0, 4)
 
+// Bit-field with explicit align bigger than normal.
+struct g0 {
+  char a;
+  __attribute__((aligned(16))) int b : 1;
+  char c;
+};
+
+CHECK_SIZE(struct, g0, 32);
+CHECK_ALIGN(struct, g0, 16);
+CHECK_OFFSET(struct, g0, c, 17);
+
+// Bit-field with explicit align smaller than normal.
+struct g1 {
+  char a;
+  __attribute__((aligned(2))) int b : 1;
+  char c;
+};
+
+CHECK_SIZE(struct, g1, 4);
+CHECK_ALIGN(struct, g1, 4);
+CHECK_OFFSET(struct, g1, c, 3);
+
+// Same as above but without explicit align.
+struct g2 {
+  char a;
+  int b : 1;
+  char c;
+};
+
+CHECK_SIZE(struct, g2, 4);
+CHECK_ALIGN(struct, g2, 4);
+CHECK_OFFSET(struct, g2, c, 2);
+
+// Explicit attribute align on bit-field has precedence over packed attribute
+// applied too the struct.
+struct __attribute__((packed)) g3 {
+  char a;
+  __attribute__((aligned(16))) int b : 1;
+  char c;
+};
+
+CHECK_SIZE(struct, g3, 32);
+CHECK_ALIGN(struct, g3, 16);
+CHECK_OFFSET(struct, g3, c, 17);
+
+struct __attribute__((packed)) g4 {
+  char a;
+  __attribute__((aligned(2))) int b : 1;
+  char c;
+};
+
+CHECK_SIZE(struct, g4, 4);
+CHECK_ALIGN(struct, g4, 2);
+CHECK_OFFSET(struct, g4, c, 3);
+
+struct g5 {
+  char : 1;
+  __attribute__((aligned(1))) int n : 24;
+};
+CHECK_SIZE(struct, g5, 4);
+CHECK_ALIGN(struct, g5, 4);
+
+struct __attribute__((packed)) g6 {
+  char : 1;
+  __attribute__((aligned(1))) int n : 24;
+};
+CHECK_SIZE(struct, g6, 4);
+CHECK_ALIGN(struct, g6, 1);
+
+struct g7 {
+  char : 1;
+  __attribute__((aligned(1))) int n : 25;
+};
+CHECK_SIZE(struct, g7, 8);
+CHECK_ALIGN(struct, g7, 4);
+
+struct __attribute__((packed)) g8 {
+  char : 1;
+  __attribute__((aligned(1))) int n : 25;
+};
+CHECK_SIZE(struct, g8, 5);
+CHECK_ALIGN(struct, g8, 1);
+
+struct g9 {
+  __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2, e : 2;
+  int i;
+};
+CHECK_SIZE(struct, g9, 12);
+CHECK_ALIGN(struct, g9, 4);
+
+struct __attribute__((packed)) g10 {
+  __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2, e : 2;
+  int i;
+};
+CHECK_SIZE(struct, g10, 9);
+CHECK_ALIGN(struct, g10, 1);
+
+struct g11 {
+  char a;
+  __attribute__((aligned(1))) long long b : 62;
+  char c;
+};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, g11, 24);
+CHECK_ALIGN(struct, g11, 8);
+CHECK_OFFSET(struct, g11, c, 16);
+#else
+CHECK_SIZE(struct, g11, 16);
+CHECK_ALIGN(struct, g11, 4);
+CHECK_OFFSET(struct, g11, c, 12);
+#endif
+
+struct __attribute__((packed)) g12 {
+  char a;
+  __attribute__((aligned(1))) long long b : 62;
+  char c;
+};
+CHECK_SIZE(struct, g12, 10);
+CHECK_ALIGN(struct, g12, 1);
+CHECK_OFFSET(struct, g12, c, 9);
+
+struct g13 {
+  char a;
+  __attribute__((aligned(1))) long long : 0;
+  char c;
+};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, g13, 16);
+CHECK_ALIGN(struct, g13, 8);
+CHECK_OFFSET(struct, g13, c, 8);
+#else
+CHECK_SIZE(struct, g13, 5);
+CHECK_ALIGN(struct, g13, 1);
+CHECK_OFFSET(struct, g13, c, 4);
+#endif
+
+struct __attribute__((packed)) g14 {
+  char a;
+  __attribute__((aligned(1))) long long : 0;
+  char c;
+};
+#if defined(__arm__) || defined(__aarch64__)
+CHECK_SIZE(struct, g14, 16);
+CHECK_ALIGN(struct, g14, 8);
+CHECK_OFFSET(struct, g14, c, 8);
+#else
+CHECK_SIZE(struct, g14, 5);
+CHECK_ALIGN(struct, g14, 1);
+CHECK_OFFSET(struct, g14, c, 4);
+#endif