Page MenuHomePhabricator

[AIX] Implement AIX special bitfield related alignment rules
Needs ReviewPublic

Authored by Xiangling_L on Sep 2 2020, 7:24 AM.

Details

Summary

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

Diff Detail

Event Timeline

Xiangling_L created this revision.Sep 2 2020, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2020, 7:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Xiangling_L requested review of this revision.Sep 2 2020, 7:24 AM

Rebased on the pragma/pack patch;
Added packed related testcase for bitfield;

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
626–627
Xiangling_L marked an inline comment as done.Thu, Sep 24, 12:39 PM

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.

Sure, will do.

Rebased on the NFC patch;

sfertile added inline comments.Mon, Oct 5, 8:01 AM
clang/lib/Sema/SemaDecl.cpp
16447 ↗(On Diff #294157)

Can this change can be split out into its own patch? If it can i would suggest doing so.

clang/test/Layout/aix-oversized-bitfield.cpp
1 ↗(On Diff #294157)

Is it possible to run this test as both C and C++?

sfertile added inline comments.Mon, Oct 5, 11:45 AM
clang/lib/AST/RecordLayoutBuilder.cpp
1644

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?

sfertile added inline comments.Mon, Oct 5, 12:43 PM
clang/test/Layout/aix-bitfield-alignment.cpp
119

We should have a similar test for an overaligned long or long long.

Xiangling_L added inline comments.Mon, Oct 5, 1:53 PM
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?

Xiangling_L marked 4 inline comments as done.
  • Fixed the bug of getting underlying type of enum;
  • Fixed the bug to respect align attribute;
  • Add more testcases;
sfertile added inline comments.Tue, Oct 6, 2:07 PM
clang/lib/Sema/SemaDecl.cpp
16447 ↗(On Diff #294157)

I was expecting our buildbot can pick up all bitfield related changes at one time.

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.

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.

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.

Xiangling_L marked an inline comment as done.Wed, Oct 7, 10:31 AM
Xiangling_L added inline comments.
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.

Xiangling_L edited the summary of this revision. (Show Details)Thu, Oct 8, 6:57 AM
Xiangling_L marked an inline comment as done.

Remove emit errors for oversized long long bitfield and related testcase;

sfertile added inline comments.Fri, Oct 9, 8:04 AM
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?
Considering StorageUnitSize:

  • for types smaller then unsigned we always increase the storage size to that of unsigned.
  • for int, and long in 32-bit the storage size is already 32 and we don't adjust it.
  • for long long in 32-bit we decrease the size to 32.
  • For long long and long in 64-bit we leave the storage size as 64.

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.
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.

1638

to unsigned --> to alignof(unsigned)

clang/test/Layout/aix-bitfield-alignment.cpp
1

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.