This is an archive of the discontinued LLVM Phabricator instance.

Fix LIT failure on native aix
ClosedPublic

Authored by Xiangling_L on May 18 2021, 1:01 PM.

Details

Diff Detail

Unit TestsFailed

Event Timeline

Xiangling_L requested review of this revision.May 18 2021, 1:01 PM
Xiangling_L created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 1:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Xiangling_L edited the summary of this revision. (Show Details)May 18 2021, 5:05 PM

Add comments to the block;

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];

Adjusted the testcase so that it also tests the property well on aix;

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.

This revision is now accepted and ready to land.May 19 2021, 9:28 AM
aaron.ballman added inline comments.May 19 2021, 9:54 AM
clang/test/Sema/struct-packed-align.c
171

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

Xiangling_L marked an inline comment as done.May 19 2021, 10:36 AM
Xiangling_L added inline comments.
clang/test/Sema/struct-packed-align.c
171

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

Xiangling_L retitled this revision from [AIX] Fix LIT failure on native aix to Fix LIT failure on native aix.May 19 2021, 10:37 AM
Xiangling_L marked an inline comment as done.
Xiangling_L edited the summary of this revision. (Show Details)

Adjust the comment;

aaron.ballman added inline comments.May 19 2021, 10:55 AM
clang/test/Sema/struct-packed-align.c
171

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

Xiangling_L added inline comments.May 19 2021, 11:10 AM
clang/test/Sema/struct-packed-align.c
171

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.

clang/test/Sema/struct-packed-align.c
171

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.

aaron.ballman accepted this revision.May 19 2021, 11:30 AM

LGTM!

clang/test/Sema/struct-packed-align.c
171

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.

This revision was automatically updated to reflect the committed changes.