[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sfertile marked an inline comment as done.
Closed by commit rGb8f612e780e5: [PowerPC][AIX] Packed zero-width bitfields do
not affect alignment. (authored by sfertile).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
Files:
clang/lib/AST/RecordLayoutBuilder.cpp
clang/test/Layout/aix-packed-bitfields.c
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
struct A {
int a1 : 30;
@@ -75,3 +79,35 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ long long : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// 32BIT-NEXT:4:- | long long
+// 32BIT-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
+// 64BIT-NEXT:8:- | long long
+// 64BIT-NEXT: sizeof=8, {{(dsize=8, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,18 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but do not increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked)
+ FieldAlign = 1;
+if (!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign =
+ std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+}
+ }
// Diagnose differences in layout due to padding or packing.
if (!UseExternalLayout)
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
stevewan accepted this revision. stevewan added a comment. LGTM. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106900/new/ https://reviews.llvm.org/D106900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile updated this revision to Diff 364097.
sfertile added a comment.
Don't update the unpacked field align based on IsPacked.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
Files:
clang/lib/AST/RecordLayoutBuilder.cpp
clang/test/Layout/aix-packed-bitfields.c
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
struct A {
int a1 : 30;
@@ -75,3 +79,35 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ long long : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// 32BIT-NEXT:4:- | long long
+// 32BIT-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
+// 64BIT-NEXT:8:- | long long
+// 64BIT-NEXT: sizeof=8, {{(dsize=8, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,18 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but do not increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked)
+ FieldAlign = 1;
+if (!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign =
+ std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+}
+ }
// Diagnose differences in layout due to padding or packing.
if (!UseExternalLayout)
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
+
// RUN: %clang_cc1
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile marked 2 inline comments as done.
sfertile added inline comments.
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1783
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if (!MaxFieldAlignment.isZero()) {
stevewan wrote:
> `UnpackedFieldAlign` is used to check if the packed attribute is unnecessary
> (-Wpacked). here the attribute is making a difference, we probably shouldn't
> set `FieldAlign = UnpackedFieldAlign`?
Good catch, updated.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
stevewan added inline comments.
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1783
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if (!MaxFieldAlignment.isZero()) {
`UnpackedFieldAlign` is used to check if the packed attribute is unnecessary
(-Wpacked). here the attribute is making a difference, we probably shouldn't
set `FieldAlign = UnpackedFieldAlign`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
stevewan added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1781 - // pragma align(packed). - if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize) -FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits); sfertile wrote: > stevewan wrote: > > Just noting that the comment says `MaxFieldAlignment - The maximum allowed > > field alignment. This is set by #pragma pack`, but `__attribute__(packed)` > > also seems to set it to some large value that is at least as large as the > > FieldAlign. Maybe edit the comment accordingly for now, and a future > > follow-on patch if necessary. > Sorry Steven, not sure what you are asking for. > > attribute packed will set 'FieldPacked` to true, in which case we ignore the > max field alignment (and why the new comment says, or 1 if packed). IIUC the > MaXFieldAlign is set by only by pragma pack and pragma align in the Itanium > recored layout builder, and the place where it is set based on attribute > packed is int he Microsoft record layout builder and doesn't affect us here. Sorry Sean, my previous comment was indeed confusing. The old buggy behaviour confused me, and for some reason I thought the code must had went down this if statement when attribute packed is set, hence the previous comment. Now that I take a second look, I realize the problem was actually that we never went into the `if` to fix the FieldAlign. Sorry for the churn. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106900/new/ https://reviews.llvm.org/D106900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile updated this revision to Diff 363147.
sfertile added a comment.
Fixed spelling and added a 'long long' zero width bitfield to the testing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
Files:
clang/lib/AST/RecordLayoutBuilder.cpp
clang/test/Layout/aix-packed-bitfields.c
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,64BIT %s
struct A {
int a1 : 30;
@@ -75,3 +79,35 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ long long : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// 32BIT-NEXT:4:- | long long
+// 32BIT-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
+// 64BIT-NEXT:8:- | long long
+// 64BIT-NEXT: sizeof=8, {{(dsize=8, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,18 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but do not increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if (!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign =
+ std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+}
+ }
// Diagnose differences in layout due to padding or packing.
if (!UseExternalLayout)
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -1,14 +1,18 @@
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c++ %s | \
+// RUN: FileCheck --check-prefixes=CHECK,32BIT %s
+
// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
-// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | FileCheck %s
-//
+// RUN: -fsyntax-only -fxl-pragma-pack -x c %s | \
+// RUN: FileCheck
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1781 - // pragma align(packed). - if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize) -FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits); stevewan wrote: > Just noting that the comment says `MaxFieldAlignment - The maximum allowed > field alignment. This is set by #pragma pack`, but `__attribute__(packed)` > also seems to set it to some large value that is at least as large as the > FieldAlign. Maybe edit the comment accordingly for now, and a future > follow-on patch if necessary. Sorry Steven, not sure what you are asking for. attribute packed will set 'FieldPacked` to true, in which case we ignore the max field alignment (and why the new comment says, or 1 if packed). IIUC the MaXFieldAlign is set by only by pragma pack and pragma align in the Itanium recored layout builder, and the place where it is set based on attribute packed is int he Microsoft record layout builder and doesn't affect us here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106900/new/ https://reviews.llvm.org/D106900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
ZarkoCA accepted this revision. ZarkoCA added a comment. LGTM also. Just a comment typo. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1779 + // On AIX, zero-width bitfields pad out to the natural alignment boundary, + // but dont increase the alignment greater than the MaxFieldAlignment, or 1 + // if packed. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1779 + // On AIX, zero-width bitfields pad out to the natural alignment boundary, + // but dont increase the alignment greater than the MaxFieldAlignment, or 1 + // if packed. ZarkoCA wrote: > Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106900/new/ https://reviews.llvm.org/D106900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
stevewan accepted this revision.
stevewan added a comment.
This revision is now accepted and ready to land.
LGTM with some nits.
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1781
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
Just noting that the comment says `MaxFieldAlignment - The maximum allowed
field alignment. This is set by #pragma pack`, but `__attribute__(packed)` also
seems to set it to some large value that is at least as large as the
FieldAlign. Maybe edit the comment accordingly for now, and a future follow-on
patch if necessary.
Comment at: clang/test/Layout/aix-packed-bitfields.c:95
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
nit: might be helpful to use a different type for the zero-width bitfield here.
(e.g., `long long : 0`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile updated this revision to Diff 362365.
sfertile added a comment.
clang-formatted
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106900/new/
https://reviews.llvm.org/D106900
Files:
clang/lib/AST/RecordLayoutBuilder.cpp
clang/test/Layout/aix-packed-bitfields.c
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -75,3 +75,33 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ int : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,18 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but dont increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if (!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign =
+ std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+}
+ }
// Diagnose differences in layout due to padding or packing.
if (!UseExternalLayout)
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -75,3 +75,33 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ int : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,18 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but dont increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if (!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign =
+
[PATCH] D106900: [PowerPC][AIX] Packed zero-width bitfields do not affect alignment.
sfertile created this revision.
sfertile added reviewers: stevewan, Jake-Egan.
Herald added subscribers: shchenz, nemanjai.
sfertile requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Zero-width bitfields on AIX pad out to the natral alignment boundary but do not
change the containing records alignment when they are packed.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D106900
Files:
clang/lib/AST/RecordLayoutBuilder.cpp
clang/test/Layout/aix-packed-bitfields.c
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -75,3 +75,33 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ int : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,17 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but dont increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if (isAIXLayout(Context) && !FieldSize) {
+if (FieldPacked) {
+ FieldAlign = UnpackedFieldAlign = 1;
+} else if(!MaxFieldAlignment.isZero()) {
+ UnpackedFieldAlign = std::min(UnpackedFieldAlign,
MaxFieldAlignmentInBits);
+ FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+}
+ }
// Diagnose differences in layout due to padding or packing.
if (!UseExternalLayout)
Index: clang/test/Layout/aix-packed-bitfields.c
===
--- clang/test/Layout/aix-packed-bitfields.c
+++ clang/test/Layout/aix-packed-bitfields.c
@@ -75,3 +75,33 @@
// CHECK-NEXT: 3:6-35 | int a2
// CHECK-NEXT: 7:4-7 | int a3
// CHECK-NEXT: sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+ char f1;
+ int : 0;
+ short : 3;
+ char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttr
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: 4:0-2 | short
+// CHECK-NEXT: 4:3-4 | char f4
+// CHECK-NEXT: sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+ char f1;
+ int : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT: 0 | struct PackedAttrAndPragma
+// CHECK-NEXT: 0 | char f1
+// CHECK-NEXT:4:- | int
+// CHECK-NEXT: sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1775,11 +1775,17 @@
!D->getIdentifier())
FieldAlign = UnpackedFieldAlign = 1;
- // On AIX, zero-width bitfields pad out to the alignment boundary, but then
- // do not affect overall record alignment if there is a pragma pack or
- // pragma align(packed).
- if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
-FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+ // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+ // but dont increase the alignment greater than the MaxFieldAlignment, or 1
+ // if packed.
+ if
