arphaman created this revision.
arphaman added reviewers: ahatanak, rjmccall.
Herald added a subscriber: jkorous-apple.
Clang currently generates wrong record layout for `-mms-bitfield` and `#pragma
pack`. https://godbolt.org/g/nQ4rVW shows how MSVC and GCC generate different
layout to Clang.
This patch fixes the issue by ensuring that bitfields are not packed, but the
record's alignment is still updated. This seems to be in line with logic used
by GCC and MSVC.
rdar://36343145
Repository:
rC Clang
https://reviews.llvm.org/D42660
Files:
lib/AST/RecordLayoutBuilder.cpp
test/CodeGen/mms-bitfields.c
test/Sema/mms-bitfields.c
Index: test/Sema/mms-bitfields.c
===================================================================
--- test/Sema/mms-bitfields.c
+++ test/Sema/mms-bitfields.c
@@ -11,3 +11,18 @@
// MS pads out bitfields between different types.
static int arr[(sizeof(t) == 8) ? 1 : -1];
+
+#pragma pack (push,1)
+
+typedef unsigned int UINT32;
+
+struct Inner {
+ UINT32 A : 1;
+ UINT32 B : 1;
+ UINT32 C : 1;
+ UINT32 D : 30;
+} Inner;
+
+#pragma pack (pop)
+
+static int arr2[(sizeof(Inner) == 8) ? 1 : -1];
Index: test/CodeGen/mms-bitfields.c
===================================================================
--- test/CodeGen/mms-bitfields.c
+++ test/CodeGen/mms-bitfields.c
@@ -20,3 +20,46 @@
} s3;
// CHECK: %struct.s3 = type { i32, [4 x i8], %struct.s1 }
+
+// PR32482:
+
+#pragma pack (push,1)
+
+typedef unsigned int UINT32;
+
+struct Inner {
+ UINT32 A : 1;
+ UINT32 B : 1;
+ UINT32 C : 1;
+ UINT32 D : 30;
+} Inner;
+
+#pragma pack (pop)
+
+// CHECK: %struct.Inner = type { i32, i32 }
+
+// CHECK: %struct.A = type { i32, i32, i32 }
+
+#pragma pack(push, 1)
+
+union HEADER {
+ struct A {
+ int : 3; // Bits 2:0
+ int a : 9; // Bits 11:3
+ int : 12; // Bits 23:12
+ int b : 17; // Bits 40:24
+ int : 7; // Bits 47:41
+ int c : 4; // Bits 51:48
+ int : 4; // Bits 55:52
+ int d : 3; // Bits 58:56
+ int : 5; // Bits 63:59
+ } Bits;
+} HEADER;
+
+#pragma pack(pop)
+
+struct Inner variable = { 1,0,1, 21 };
+union HEADER hdr = {{1,2,3,4}};
+
+// CHECK: @variable = global { i8, [3 x i8], i8, i8, i8, i8 } { i8 5, [3 x i8] undef, i8 21, i8 0, i8 0, i8 0 }, align 1
+// CHECK: @hdr = global { { i8, i8, [2 x i8], i8, i8, i8, i8, i8, [3 x i8] } } { { i8, i8, [2 x i8], i8, i8, i8, i8, i8, [3 x i8] } { i8 8, i8 0, [2 x i8] undef, i8 2, i8 0, i8 0, i8 3, i8 4, [3 x i8] undef } }, align 1
Index: lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1561,12 +1561,21 @@
// But, if there's a #pragma pack in play, that takes precedent over
// even the 'aligned' attribute, for non-zero-width bitfields.
unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment);
+ unsigned RecordFieldAlign = FieldAlign;
if (!MaxFieldAlignment.isZero() && FieldSize) {
- UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
- if (FieldPacked)
- FieldAlign = UnpackedFieldAlign;
- else
- FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // #pragma pack does not affect bitfield layout in ms_struct, but the
+ // record's alignment is still changed.
+ if (IsMsStruct) {
+ RecordFieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ } else {
+ UnpackedFieldAlign =
+ std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+ if (FieldPacked)
+ FieldAlign = UnpackedFieldAlign;
+ else
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ RecordFieldAlign = FieldAlign;
+ }
}
// But, ms_struct just ignores all of that in unions, even explicit
@@ -1699,7 +1708,7 @@
setSize(std::max(getSizeInBits(), getDataSizeInBits()));
// Remember max struct/class alignment.
- UpdateAlignment(Context.toCharUnitsFromBits(FieldAlign),
+ UpdateAlignment(Context.toCharUnitsFromBits(RecordFieldAlign),
Context.toCharUnitsFromBits(UnpackedFieldAlign));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
