This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] fix lit test related to alignment
ClosedPublic

Authored by NancyWang2222 on Mar 17 2021, 10:24 AM.

Details

Summary

This patch is to fix lit test case failure relate to alignment, on z/OS, maximum alignment value for 64 bit mode is 16 and also fixed clang/test/Layout/itanium-union-bitfield.cpp, attribute ((aligned(4))) is needed for bit-field member in Union for z/OS because single bit-field has one byte alignment, this will make sure size and alignment will be correct value on z/OS.

Diff Detail

Event Timeline

NancyWang2222 requested review of this revision.Mar 17 2021, 10:24 AM
NancyWang2222 created this revision.
NancyWang2222 edited the summary of this revision. (Show Details)Mar 17 2021, 10:27 AM
NancyWang2222 edited the summary of this revision. (Show Details)Mar 17 2021, 10:32 AM
NancyWang2222 added a reviewer: Kai.

@NancyWang2222, the patch description doesn't seem to say much about the bit-field case. Can you expand it?

clang/test/Layout/itanium-union-bitfield.cpp
4–5

This comment is supposed to be true for both of the bit-field declarations, no?

7

I think the macro should be in all caps.

20

I can't help but notice that this does not have the attribute applied.

35

This output does not match the behaviour of the xlclang++ compiler I tried:

union B {
  char f1: 35;
  B();
};

B::B() {}

extern "C" int printf(const char *, ...);
int main(void) {
   B b[1];
   printf("%lld\n", (long long)((char *)(b + 1) - (char *)b));
}

Gives me:

5

as the size of the union.

Hi Nancy, can you set this patch as a child revision of https://reviews.llvm.org/D98864 which sets the maximum alignment.

NancyWang2222 added inline comments.Mar 18 2021, 9:38 AM
clang/test/Layout/itanium-union-bitfield.cpp
7

I will fix it. Thanks

20

it doesnt fail on union B. dump shows:

  • Dumping AST Record Layout 0 | union B 0:0-34 | char f1 | [sizeof=8, dsize=5, align=4, | nvsize=5, nvalign=4]

which is what test case expects

35

You are right. xlclang++ has different output from clang. will need to look into it.

clang/test/Layout/itanium-union-bitfield.cpp
20

Just pointing out that the comment above and the xlclang++ behaviour below both seem to imply that adding the alignment attribute here should be done. If adding it is currently harmless, then I think it makes sense to add it.

Hi Nancy, can you set this patch as a child revision of https://reviews.llvm.org/D98864 which sets the maximum alignment.

added. thanks

clang/test/Layout/itanium-union-bitfield.cpp
20

sure. I will add attributes for union B as well. Thanks for pointing it out.

NancyWang2222 edited the summary of this revision. (Show Details)
NancyWang2222 edited the summary of this revision. (Show Details)
NancyWang2222 edited the summary of this revision. (Show Details)Mar 18 2021, 11:40 AM
NancyWang2222 edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Mar 18 2021, 3:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 10:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript