1.[bool, char, short] bitfields have the same alignment as unsigned int
2.Adjust alignment on typedef field decls/honor align attribute
3.Fix alignment for scoped enum class
4.Long long bitfield has 4bytes alignment and StorageUnitSize under 32 bit compile mode
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
I think it would help the review if we could put the NFC portion(e.g. TypeSize -> StorageUnitSize) to a new patch, and give some rationale about the NFC change.
| clang/lib/AST/RecordLayoutBuilder.cpp | ||
|---|---|---|
| 624 | ||
| clang/lib/AST/RecordLayoutBuilder.cpp | ||
|---|---|---|
| 1643 | I get different results for using a long long bitfield, and using an enum with an underlying long long type: struct S {
unsigned long long field : 32;
};
extern "C" int printf(const char*, ...);
int main(void) {
printf("%lu %lu\n", sizeof(struct S), alignof(struct S));
}*** Dumping AST Record Layout
0 | struct S
0:0-31 | unsigned long long field
| [sizeof=4, dsize=4, align=4, preferredalign=4,
| nvsize=4, nvalign=4, preferrednvalign=4]enum e : unsigned long long { E = 1 };
struct S {
enum e field : 32;
};
extern "C" int printf(const char*, ...);
int main(void) {
printf("%lu %lu\n", sizeof(struct S), alignof(struct S));
}*** Dumping AST Record Layout
0 | struct S
0:0-31 | enum e field
| [sizeof=8, dsize=8, align=8, preferredalign=8,
| nvsize=8, nvalign=8, preferrednvalign=8]Shouldn't we be truncating the size/align of the enum typed bitfield as well? | |
| clang/test/Layout/aix-bitfield-alignment.cpp | ||
|---|---|---|
| 119 | We should have a similar test for an overaligned long or long long. | |
| clang/lib/Sema/SemaDecl.cpp | ||
|---|---|---|
| 16447 ↗ | (On Diff #294157) | I was expecting our buildbot can pick up all bitfield related changes at one time. Also if we split this out, that means we either need to wait for this second part to land first or need to add more LIT to oversized long long to the first part, which then needs to be removed whenever this second part land. It seems we are complicating the patch. Can you give me your rationale about why we want to split out this part? |
Comment Actions
- Fixed the bug of getting underlying type of enum;
- Fixed the bug to respect align attribute;
- Add more testcases;
| clang/lib/Sema/SemaDecl.cpp | ||
|---|---|---|
| 16447 ↗ | (On Diff #294157) |
IIUC clang/test/Layout/aix-oversized-bitfield.cpp works with just this change and isn't dependent on D87702. Its disjoint from the other changes in this patch, and packaging it into a commit with unrelated changes even if they are on the same theme is not beneficial. Its better to have those run through the build bot (or be bisectable) as distinct changes.
I don't understand why it would need to wait or require extra testing to be added. Its a diagnostic and your lit test shows the error for 32-bit (where we want it emitted) and expected layout for 64-bit. The whole point of splitting it out is that its simple,does exactly one thing, is testable on its own, and we don't need the context of the other changes packaged with it to properly review it. I am asking to split it out because I see it as making this easier to review and commit. |
| clang/lib/Sema/SemaDecl.cpp | ||
|---|---|---|
| 16447 ↗ | (On Diff #294157) | Sure, I will split this patch into two as you suggested. By either need to wait for this second part to land first or need to add more LIT , I thought we would like to also add test coverage and later remove it for oversize bitfield. Since StorageUnitSize > 32 && Context.getTargetInfo().getTriple().isArch32Bit() does affect how oversize bitfield get laid out on AIX. But I guess it's more convenient to just split this patch as you suggested. |
| clang/lib/AST/RecordLayoutBuilder.cpp | ||
|---|---|---|
| 1633 | When can BTy be null? Should this instead be an assert? Instead can we implement this without looking at the underlying type of the bitfield?
This could be implemented as if (StorageUnitSize < Context.getTypeSize(Context.UnsignedIntTy) || (StorageUnitSize > Context.getTypeSize(Context.UnsignedIntTy) && Context.getTargetInfo().getTriple().isArch32Bit())) StorageUnitSize = Context.getTypeSize(Context.UnsignedIntTy); Without relying on extracting the underlying type and having to worry about the case where BTy is null. | |
| 1638 | to unsigned --> to alignof(unsigned) | |
| clang/test/Layout/aix-bitfield-alignment.cpp | ||
| 2 | I suggest we split the test cases which are C++ only into a sperate file, and run the cases that are valid in both C and C++ as both to ensure there are no differences between C and C++ layout fir bit fields. | |
Comment Actions
Split testcases;
Simplified code;
Don't need to respect attribute align within typedef when the alignment value is less than bitcontainer size;
| clang/test/Layout/aix-bitfield-alignment.cpp | ||
|---|---|---|
| 2 | Good idea. I split the testcase as you suggested. But since C and C++ layout dumping are slightly different in format, the CHECKs are adjusted accordingly to avoid making testcase too messy. | |
When can BTy be null? Should this instead be an assert?
Instead can we implement this without looking at the underlying type of the bitfield?
Considering StorageUnitSize:
This could be implemented as
Without relying on extracting the underlying type and having to worry about the case where BTy is null.
If we can also handle FieldAlign without knowing the underlying type then I think we should do so and avoid trying to extract the underlying type altogether.