[PATCH] D102715: Fix LIT failure on native aix
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd74b6635ef38: Fix LIT failure on native aix (authored by
Xiangling_L).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102715/new/
https://reviews.llvm.org/D102715
Files:
clang/test/Sema/struct-packed-align.c
Index: clang/test/Sema/struct-packed-align.c
===
--- clang/test/Sema/struct-packed-align.c
+++ clang/test/Sema/struct-packed-align.c
@@ -146,18 +146,17 @@
// GCC 4.4 but the change can lead to differences in the structure layout.
// See the documentation of -Wpacked-bitfield-compat for more information.
struct packed_chars {
- char a:4;
+ char a : 8, b : 8, c : 8, d : 4;
#ifdef __ORBIS__
// Test for pre-r254596 clang behavior on the PS4 target. PS4 must maintain
// ABI backwards compatibility.
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute ignored for field of type
'char'}}
- char c:4;
#else
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute was ignored on bit-fields with
single-byte alignment in older versions of GCC and Clang}}
- char c:4;
#endif
+ char f : 4, g : 8, h : 8, i : 8;
};
#if (defined(_WIN32) || defined(__ORBIS__)) && !defined(__declspec) //
_MSC_VER is unavailable in cc1.
@@ -165,9 +164,13 @@
//
// Additionally, test for pre-r254596 clang behavior on the PS4 target. PS4
// must maintain ABI backwards compatibility.
-extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 9 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#elif defined(_AIX)
+// On AIX, char bitfields have the same alignment as unsigned int.
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 4 ? 1 : -1];
#else
-extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#endif
Index: clang/test/Sema/struct-packed-align.c
===
--- clang/test/Sema/struct-packed-align.c
+++ clang/test/Sema/struct-packed-align.c
@@ -146,18 +146,17 @@
// GCC 4.4 but the change can lead to differences in the structure layout.
// See the documentation of -Wpacked-bitfield-compat for more information.
struct packed_chars {
- char a:4;
+ char a : 8, b : 8, c : 8, d : 4;
#ifdef __ORBIS__
// Test for pre-r254596 clang behavior on the PS4 target. PS4 must maintain
// ABI backwards compatibility.
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute ignored for field of type 'char'}}
- char c:4;
#else
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute was ignored on bit-fields with single-byte alignment in older versions of GCC and Clang}}
- char c:4;
#endif
+ char f : 4, g : 8, h : 8, i : 8;
};
#if (defined(_WIN32) || defined(__ORBIS__)) && !defined(__declspec) // _MSC_VER is unavailable in cc1.
@@ -165,9 +164,13 @@
//
// Additionally, test for pre-r254596 clang behavior on the PS4 target. PS4
// must maintain ABI backwards compatibility.
-extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 9 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#elif defined(_AIX)
+// On AIX, char bitfields have the same alignment as unsigned int.
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 4 ? 1 : -1];
#else
-extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102715: Fix LIT failure on native aix
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Comment at: clang/test/Sema/struct-packed-align.c:170 +#elif defined(_AIX) +// On AIX, [bool, char, short] bitfields have the same alignment as unsigned +// int. hubert.reinterpretcast wrote: > Xiangling_L wrote: > > aaron.ballman wrote: > > > Xiangling_L wrote: > > > > aaron.ballman wrote: > > > > > We're not really testing the behavior of `bool` or `short` anywhere > > > > > and it'd be nice to verify that. Perhaps instead of modifying an > > > > > existing test to add more fields, it'd make sense to make a new test > > > > > structure? > > > > > > > > > > While thinking of other potentially smaller-than-int types, I > > > > > wondered whether `wchar_t` has special behavior here as well (I have > > > > > no idea how that type is defined for AIX, so it's entirely possible > > > > > it's size and alignment already match `int`). > > > > > We're not really testing the behavior of bool or short anywhere and > > > > > it'd be nice to verify that. > > > > > > > > The comment is to explain why char has 4-byte alignment mainly. And the > > > > testcase here is, as comments mentioned, to test `Packed attribute > > > > shouldn't be ignored for bit-field of char types`. Perhaps I should > > > > remove `bool` and `short` so that people wouldn't be confused. > > > > > > > > And the special alignment regarding bool, short etc. has been tested > > > > when the special rule introduced on aix here: > > > > https://reviews.llvm.org/D87029. > > > > > > > > > > > > > > > > > Perhaps instead of modifying an existing test to add more fields, > > > > > it'd make sense to make a new test structure? > > > > > > > > I don't think it's necessary to make a new test structure. The modified > > > > testcase test the same property as the original one. And it is more > > > > capable as it can also test the property for AIX target. > > > > > > > > > > > > > > > > > > > > > I wondered whether wchar_t has special behavior here as well > > > > > > > > I think `wchar_t` has the same special behavior. Basically any type > > > > smaller than 4 bytes will be aligned to 4 when it comes to bitfield. > > > > Please correct me if I am wrong @hubert.reinterpretcast > > > > > > > > > > > > Perhaps I should remove bool and short so that people wouldn't be > > > > confused. > > > > > > That might not be a bad idea. I saw the comment and went to look for the > > > declarations of `bool` and `short` type to verify they were behaving the > > > same way, hence the confusion. > > > > > > > The modified testcase test the same property as the original one. > > > > > > The part that worries me is that it shifts the offset for `e`. Before, > > > the packed field could be packed into the previous allocation unit (4 > > > bits + 8 bits fit comfortably within a 32-bit allocation unit), but now > > > the packed field is in an awkward spot (28 bits + 8 bits no longer fits > > > into a 32-bit allocation unit). So I think it could be subtly changing > > > the behavior of the test, but perhaps not in an observable way that > > > matters (I admit that I don't know all the ins and outs of our packing > > > strategies). > > > but now the packed field is in an awkward spot (28 bits + 8 bits no > > > longer fits into a 32-bit allocation unit) > > > > > > I think this is exactly the purpose of the test. We'd like to tell if the > > `packed` attribute has effect or not. > > > > Before the modification, on AIX, no matter the packed works or not, you > > will see the size = 4, align = 4 since char has 4-byte alignment. > My understanding is that the "awkward spot" was always the intention of the > test. It's just that the assumption around "allocation unit" size being 1 for > `char` was encoded into the test. Ah, thank you both for pointing that out to me. This just shifts the awkward spot to make the test scenario more obvious. That makes a lot more sense to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102715/new/ https://reviews.llvm.org/D102715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102715: Fix LIT failure on native aix
hubert.reinterpretcast added inline comments. Comment at: clang/test/Sema/struct-packed-align.c:170 +#elif defined(_AIX) +// On AIX, [bool, char, short] bitfields have the same alignment as unsigned +// int. Xiangling_L wrote: > aaron.ballman wrote: > > Xiangling_L wrote: > > > aaron.ballman wrote: > > > > We're not really testing the behavior of `bool` or `short` anywhere and > > > > it'd be nice to verify that. Perhaps instead of modifying an existing > > > > test to add more fields, it'd make sense to make a new test structure? > > > > > > > > While thinking of other potentially smaller-than-int types, I wondered > > > > whether `wchar_t` has special behavior here as well (I have no idea how > > > > that type is defined for AIX, so it's entirely possible it's size and > > > > alignment already match `int`). > > > > We're not really testing the behavior of bool or short anywhere and > > > > it'd be nice to verify that. > > > > > > The comment is to explain why char has 4-byte alignment mainly. And the > > > testcase here is, as comments mentioned, to test `Packed attribute > > > shouldn't be ignored for bit-field of char types`. Perhaps I should > > > remove `bool` and `short` so that people wouldn't be confused. > > > > > > And the special alignment regarding bool, short etc. has been tested when > > > the special rule introduced on aix here: https://reviews.llvm.org/D87029. > > > > > > > > > > > > > Perhaps instead of modifying an existing test to add more fields, it'd > > > > make sense to make a new test structure? > > > > > > I don't think it's necessary to make a new test structure. The modified > > > testcase test the same property as the original one. And it is more > > > capable as it can also test the property for AIX target. > > > > > > > > > > > > > > > > I wondered whether wchar_t has special behavior here as well > > > > > > I think `wchar_t` has the same special behavior. Basically any type > > > smaller than 4 bytes will be aligned to 4 when it comes to bitfield. > > > Please correct me if I am wrong @hubert.reinterpretcast > > > > > > > > > Perhaps I should remove bool and short so that people wouldn't be > > > confused. > > > > That might not be a bad idea. I saw the comment and went to look for the > > declarations of `bool` and `short` type to verify they were behaving the > > same way, hence the confusion. > > > > > The modified testcase test the same property as the original one. > > > > The part that worries me is that it shifts the offset for `e`. Before, the > > packed field could be packed into the previous allocation unit (4 bits + 8 > > bits fit comfortably within a 32-bit allocation unit), but now the packed > > field is in an awkward spot (28 bits + 8 bits no longer fits into a 32-bit > > allocation unit). So I think it could be subtly changing the behavior of > > the test, but perhaps not in an observable way that matters (I admit that I > > don't know all the ins and outs of our packing strategies). > > but now the packed field is in an awkward spot (28 bits + 8 bits no longer > > fits into a 32-bit allocation unit) > > > I think this is exactly the purpose of the test. We'd like to tell if the > `packed` attribute has effect or not. > > Before the modification, on AIX, no matter the packed works or not, you will > see the size = 4, align = 4 since char has 4-byte alignment. My understanding is that the "awkward spot" was always the intention of the test. It's just that the assumption around "allocation unit" size being 1 for `char` was encoded into the test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102715/new/ https://reviews.llvm.org/D102715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102715: Fix LIT failure on native aix
Xiangling_L added inline comments. Comment at: clang/test/Sema/struct-packed-align.c:170 +#elif defined(_AIX) +// On AIX, [bool, char, short] bitfields have the same alignment as unsigned +// int. aaron.ballman wrote: > Xiangling_L wrote: > > aaron.ballman wrote: > > > We're not really testing the behavior of `bool` or `short` anywhere and > > > it'd be nice to verify that. Perhaps instead of modifying an existing > > > test to add more fields, it'd make sense to make a new test structure? > > > > > > While thinking of other potentially smaller-than-int types, I wondered > > > whether `wchar_t` has special behavior here as well (I have no idea how > > > that type is defined for AIX, so it's entirely possible it's size and > > > alignment already match `int`). > > > We're not really testing the behavior of bool or short anywhere and it'd > > > be nice to verify that. > > > > The comment is to explain why char has 4-byte alignment mainly. And the > > testcase here is, as comments mentioned, to test `Packed attribute > > shouldn't be ignored for bit-field of char types`. Perhaps I should remove > > `bool` and `short` so that people wouldn't be confused. > > > > And the special alignment regarding bool, short etc. has been tested when > > the special rule introduced on aix here: https://reviews.llvm.org/D87029. > > > > > > > > > Perhaps instead of modifying an existing test to add more fields, it'd > > > make sense to make a new test structure? > > > > I don't think it's necessary to make a new test structure. The modified > > testcase test the same property as the original one. And it is more capable > > as it can also test the property for AIX target. > > > > > > > > > > > I wondered whether wchar_t has special behavior here as well > > > > I think `wchar_t` has the same special behavior. Basically any type smaller > > than 4 bytes will be aligned to 4 when it comes to bitfield. Please correct > > me if I am wrong @hubert.reinterpretcast > > > > > > Perhaps I should remove bool and short so that people wouldn't be confused. > > That might not be a bad idea. I saw the comment and went to look for the > declarations of `bool` and `short` type to verify they were behaving the same > way, hence the confusion. > > > The modified testcase test the same property as the original one. > > The part that worries me is that it shifts the offset for `e`. Before, the > packed field could be packed into the previous allocation unit (4 bits + 8 > bits fit comfortably within a 32-bit allocation unit), but now the packed > field is in an awkward spot (28 bits + 8 bits no longer fits into a 32-bit > allocation unit). So I think it could be subtly changing the behavior of the > test, but perhaps not in an observable way that matters (I admit that I don't > know all the ins and outs of our packing strategies). > but now the packed field is in an awkward spot (28 bits + 8 bits no longer > fits into a 32-bit allocation unit) I think this is exactly the purpose of the test. We'd like to tell if the `packed` attribute has effect or not. Before the modification, on AIX, no matter the packed works or not, you will see the size = 4, align = 4 since char has 4-byte alignment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102715/new/ https://reviews.llvm.org/D102715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102715: Fix LIT failure on native aix
aaron.ballman added inline comments. Comment at: clang/test/Sema/struct-packed-align.c:170 +#elif defined(_AIX) +// On AIX, [bool, char, short] bitfields have the same alignment as unsigned +// int. Xiangling_L wrote: > aaron.ballman wrote: > > We're not really testing the behavior of `bool` or `short` anywhere and > > it'd be nice to verify that. Perhaps instead of modifying an existing test > > to add more fields, it'd make sense to make a new test structure? > > > > While thinking of other potentially smaller-than-int types, I wondered > > whether `wchar_t` has special behavior here as well (I have no idea how > > that type is defined for AIX, so it's entirely possible it's size and > > alignment already match `int`). > > We're not really testing the behavior of bool or short anywhere and it'd be > > nice to verify that. > > The comment is to explain why char has 4-byte alignment mainly. And the > testcase here is, as comments mentioned, to test `Packed attribute shouldn't > be ignored for bit-field of char types`. Perhaps I should remove `bool` and > `short` so that people wouldn't be confused. > > And the special alignment regarding bool, short etc. has been tested when the > special rule introduced on aix here: https://reviews.llvm.org/D87029. > > > > > Perhaps instead of modifying an existing test to add more fields, it'd make > > sense to make a new test structure? > > I don't think it's necessary to make a new test structure. The modified > testcase test the same property as the original one. And it is more capable > as it can also test the property for AIX target. > > > > > > I wondered whether wchar_t has special behavior here as well > > I think `wchar_t` has the same special behavior. Basically any type smaller > than 4 bytes will be aligned to 4 when it comes to bitfield. Please correct > me if I am wrong @hubert.reinterpretcast > > > Perhaps I should remove bool and short so that people wouldn't be confused. That might not be a bad idea. I saw the comment and went to look for the declarations of `bool` and `short` type to verify they were behaving the same way, hence the confusion. > The modified testcase test the same property as the original one. The part that worries me is that it shifts the offset for `e`. Before, the packed field could be packed into the previous allocation unit (4 bits + 8 bits fit comfortably within a 32-bit allocation unit), but now the packed field is in an awkward spot (28 bits + 8 bits no longer fits into a 32-bit allocation unit). So I think it could be subtly changing the behavior of the test, but perhaps not in an observable way that matters (I admit that I don't know all the ins and outs of our packing strategies). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102715/new/ https://reviews.llvm.org/D102715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102715: Fix LIT failure on native aix
Xiangling_L updated this revision to Diff 346503.
Xiangling_L marked an inline comment as done.
Xiangling_L edited the summary of this revision.
Xiangling_L added a comment.
Adjust the comment;
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102715/new/
https://reviews.llvm.org/D102715
Files:
clang/test/Sema/struct-packed-align.c
Index: clang/test/Sema/struct-packed-align.c
===
--- clang/test/Sema/struct-packed-align.c
+++ clang/test/Sema/struct-packed-align.c
@@ -146,18 +146,17 @@
// GCC 4.4 but the change can lead to differences in the structure layout.
// See the documentation of -Wpacked-bitfield-compat for more information.
struct packed_chars {
- char a:4;
+ char a : 8, b : 8, c : 8, d : 4;
#ifdef __ORBIS__
// Test for pre-r254596 clang behavior on the PS4 target. PS4 must maintain
// ABI backwards compatibility.
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute ignored for field of type
'char'}}
- char c:4;
#else
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute was ignored on bit-fields with
single-byte alignment in older versions of GCC and Clang}}
- char c:4;
#endif
+ char f : 4, g : 8, h : 8, i : 8;
};
#if (defined(_WIN32) || defined(__ORBIS__)) && !defined(__declspec) //
_MSC_VER is unavailable in cc1.
@@ -165,9 +164,13 @@
//
// Additionally, test for pre-r254596 clang behavior on the PS4 target. PS4
// must maintain ABI backwards compatibility.
-extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 9 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#elif defined(_AIX)
+// On AIX, char bitfields have the same alignment as unsigned int.
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 4 ? 1 : -1];
#else
-extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#endif
Index: clang/test/Sema/struct-packed-align.c
===
--- clang/test/Sema/struct-packed-align.c
+++ clang/test/Sema/struct-packed-align.c
@@ -146,18 +146,17 @@
// GCC 4.4 but the change can lead to differences in the structure layout.
// See the documentation of -Wpacked-bitfield-compat for more information.
struct packed_chars {
- char a:4;
+ char a : 8, b : 8, c : 8, d : 4;
#ifdef __ORBIS__
// Test for pre-r254596 clang behavior on the PS4 target. PS4 must maintain
// ABI backwards compatibility.
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute ignored for field of type 'char'}}
- char c:4;
#else
- char b:8 __attribute__ ((packed));
+ char e : 8 __attribute__((packed));
// expected-warning@-1 {{'packed' attribute was ignored on bit-fields with single-byte alignment in older versions of GCC and Clang}}
- char c:4;
#endif
+ char f : 4, g : 8, h : 8, i : 8;
};
#if (defined(_WIN32) || defined(__ORBIS__)) && !defined(__declspec) // _MSC_VER is unavailable in cc1.
@@ -165,9 +164,13 @@
//
// Additionally, test for pre-r254596 clang behavior on the PS4 target. PS4
// must maintain ABI backwards compatibility.
-extern int o1[sizeof(struct packed_chars) == 3 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 9 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
+#elif defined(_AIX)
+// On AIX, char bitfields have the same alignment as unsigned int.
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
+extern int o2[__alignof(struct packed_chars) == 4 ? 1 : -1];
#else
-extern int o1[sizeof(struct packed_chars) == 2 ? 1 : -1];
+extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 1 ? 1 : -1];
#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
