On AIX, char bitfields have the same alignment as unsigned int.
Reference: https://reviews.llvm.org/D87029
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
From the comment, it seems the code as-is fails to test the property intended. It seems we want this:
struct packed_chars {
char a : 8, b : 8, c : 8, d : 4;
char e : 8 __attribute__((packed));
char f : 4, g : 8, h : 8, i : 8;
};
extern int o1[sizeof(struct packed_chars) == 8 ? 1 : -1];
extern int o2[__alignof(struct packed_chars) == 4 ? 1 : -1];Comment Actions
This LGTM. I've added additional reviewers based on the history of the file in case they will have comments. Please hold on committing this until tomorrow.
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 | 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). | |
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 |
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.
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 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 | |
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 |
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 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). | |
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 |
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. | |
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 | 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. | |
Comment Actions
LGTM!
| clang/test/Sema/struct-packed-align.c | ||
|---|---|---|
| 170 | 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. | |
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).