[PATCH] D102715: Fix LIT failure on native aix

[Aaron Ballman via Phabricator via cfe-commits]

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