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

[Alexey Bataev via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL257462: PR18513: make gcc compatible layout for bit-fields 
with explicit aligned… (authored by ABataev).

Changed prior to commit:
  http://reviews.llvm.org/D14980?vs=41866=44609#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D14980

Files:
  cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
  cfe/trunk/test/Sema/bitfield-layout.c

Index: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
===
--- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
+++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
@@ -1552,7 +1552,8 @@
 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);
   }
@@ -1601,14 +1602,21 @@
 (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
Index: cfe/trunk/test/Sema/bitfield-layout.c
===
--- cfe/trunk/test/Sema/bitfield-layout.c
+++ cfe/trunk/test/Sema/bitfield-layout.c
@@ -1,41 +1,73 @@
 // 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 
 
-#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;};
@@ -56,3 +88,153 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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);

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

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


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

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


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

Added TODO, any other comments or suggestions?


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

rjmccall added inline comments.


Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ +1605,3 @@
+} else if (ExplicitFieldAlign &&
+   Context.getTargetInfo().useBitFieldTypeAlignment())
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);

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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added inline comments.


Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ +1605,3 @@
+} else if (ExplicitFieldAlign &&
+   Context.getTargetInfo().useBitFieldTypeAlignment())
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);

rjmccall wrote:
> 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.
So do you suggest just add TODO here and remove checks for APCS as it was in 
previous patch set?


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41866.
DmitryPolukhin added a comment.

Added TODO.


http://reviews.llvm.org/D14980

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/Sema/bitfield-layout.c

Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,41 +1,73 @@
 // 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 
 
-#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;};
@@ -56,3 +88,153 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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 

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

[John McCall via cfe-commits]

rjmccall added a comment.

Yes, I think that's a reasonable approach.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41748.

http://reviews.llvm.org/D14980

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/Sema/bitfield-layout.c

Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,41 +1,73 @@
 // 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 
 
-#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;};
@@ -56,3 +88,153 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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__) 

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added inline comments.


Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 

rjmccall wrote:
> 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.)
'-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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

rjmccall added inline comments.


Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 

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


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41611.
DmitryPolukhin marked an inline comment as done.
DmitryPolukhin added a comment.

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


http://reviews.llvm.org/D14980

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/Sema/bitfield-layout.c

Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,41 +1,73 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=arm
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=aarch64
 // expected-no-diagnostics
+#include 
 
-#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;};
@@ -56,3 +88,153 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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);

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

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



Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 

rjmccall wrote:
> 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.
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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin updated this revision to Diff 41481.
DmitryPolukhin added a comment.

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.


http://reviews.llvm.org/D14980

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/Sema/bitfield-layout.c

Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,41 +1,72 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=armv7
 // expected-no-diagnostics
+#include 
 
-#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##2[offsetof(kind name, member) == offset ? 1 : -1];
 
 // Zero-width bit-fields
 struct a {char x; int : 0; char y;};
+#ifdef __ARM_EABI__
+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; };
+#ifdef __ARM_EABI__
+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;};
+#ifdef __ARM_EABI__
+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;};
+#ifdef __ARM_EABI__
+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;};
+#ifdef __ARM_EABI__
+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;};
@@ -56,3 +87,99 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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);
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- 

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

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.



Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 

rsmith wrote:
> You should round up to `FieldAlign` here.
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).


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin added a comment.

Friendly ping, any comments about this patch?


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via cfe-commits]

rsmith added a subscriber: rsmith.
rsmith added a comment.

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?



Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 

You should round up to `FieldAlign` here.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

rjmccall added a comment.

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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

rjmccall added a comment.

In http://reviews.llvm.org/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.


http://reviews.llvm.org/D14980



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

On Mon, Nov 30, 2015 at 2:31 PM, Richard Smith 
wrote:

> On Mon, Nov 30, 2015 at 11:33 AM, John McCall  wrote:
>
>> rjmccall added a comment.
>>
>> In http://reviews.llvm.org/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?
>
>
> Not exactly. For instance, given
>
> struct X {
>   __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2;
> } x = {0, 1, 2, 3};
>
> ... GCC produces
>
> x:
> .byte   0
> .byte   1
> .byte   2
> .byte   3
>
> The result is the same for any other bit-field type (except that we get
> tail padding for types wider than 4 bytes), and in particular, we don't
> allocate a new storage unit for 'b' and 'd' when the type is 'short'; we
> just insert padding to get to a 1-byte-aligned boundary.
>

This is what I meant by starting new storage: it never fills space in the
previous byte.  And now I realize that of course that's the major effect of
this patch.

The rule that a bit-field can't straddle a naturally-aligned storage unit
> for the underlying type still applies (for non-packed structs).
>

This is going to require significant surgery to this function.  And it
looks like this rule should apply to ms_struct layout as well.

In other words:
  - The offset of the field is rounded up to the next multiple of the
explicit alignment that (unless the field is packed) does not cause the
field to straddle its natural alignment boundary, which can happen if its
explicit alignment is less than its natural alignment.
  - On platforms (like ARM) that ignore the natural alignment of
bit-fields, this seems to have no effect except to always start a new
byte.  We seem to be getting this wrong, too.
  - Both the field's natural alignment and its explicit alignment, as
capped by the max field alignment, contribute to the alignment of the
aggregate, unless the field is packed.  Again, currently wrong.

John.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via cfe-commits]

On Mon, Nov 30, 2015 at 11:33 AM, John McCall  wrote:

> rjmccall added a comment.
>
> In http://reviews.llvm.org/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?


Not exactly. For instance, given

struct X {
  __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2;
} x = {0, 1, 2, 3};

... GCC produces

x:
.byte   0
.byte   1
.byte   2
.byte   3

The result is the same for any other bit-field type (except that we get
tail padding for types wider than 4 bytes), and in particular, we don't
allocate a new storage unit for 'b' and 'd' when the type is 'short'; we
just insert padding to get to a 1-byte-aligned boundary.

The rule that a bit-field can't straddle a naturally-aligned storage unit
for the underlying type still applies (for non-packed structs). For
instance,

struct Y {
  char : 1;
  __attribute__((aligned(1))) int n : 24;
} y = { 0x010203 };

struct Z {
  char : 1;
  __attribute__((aligned(1))) int n : 25;
} z = { 0x010203 };

gives:

y:
.zero   1
.byte   3
.byte   2
.byte   1
z:
.zero   4
.byte   3
.byte   2
.byte   1
.byte   0

... and if the structs are packed, y is unchanged and z changes to this:

z:
.zero   1
.byte   3
.byte   2
.byte   1
.byte   0
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via cfe-commits]

DmitryPolukhin created this revision.
DmitryPolukhin added a reviewer: rjmccall.
DmitryPolukhin added a subscriber: cfe-commits.

Fix binary compatibility issue with GCC.

http://reviews.llvm.org/D14980

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/Sema/bitfield-layout.c

Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,8 +1,13 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
 // expected-no-diagnostics
+#include 
 
-#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##2[offsetof(kind name, member) == offset ? 1 : -1];
 
 // Zero-width bit-fields
 struct a {char x; int : 0; char y;};
@@ -56,3 +61,35 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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);
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1552,7 +1552,8 @@
 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);
   }
@@ -1601,10 +1602,12 @@
 (AllowPadding &&
  (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) {
   FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign);
-}
+} else if (ExplicitFieldAlign)
+  FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
 
 // Repeat the computation for diagnostic purposes.
 if (FieldSize == 0 ||
+ExplicitFieldAlign ||
 (AllowPadding &&
  (UnpackedFieldOffset & (UnpackedFieldAlign-1)) + FieldSize > 
TypeSize))
   UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset,


Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ test/Sema/bitfield-layout.c
@@ -1,8 +1,13 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
 // expected-no-diagnostics
+#include 
 
-#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##2[offsetof(kind name, member) == offset ? 1 : -1];
 
 // Zero-width bit-fields
 struct a {char x; int : 0; char y;};
@@ -56,3 +61,35 @@
 CHECK_SIZE(struct, s0, 0x3218)
 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);
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1552,7 +1552,8 @@
 FieldAlign = 1;
 
   // But, if there's an 'aligned' attribute on the field,