Page MenuHomePhabricator

[AIX] Implement AIX special alignment rule about double/long double
ClosedPublic

Authored by Xiangling_L on May 11 2020, 9:06 AM.

Details

Summary

Implement AIX default power alignment rule by adding PreferredAlignment and PreferredNVAlignment in ASTRecordLayout class.

The implementation aims at returning correct value for __alignof(x) and alignof(x) under power alignment rules.

The AIX power alignment rules apply the natural alignment of the
"first member" if it is of a floating-point data type (or is an aggregate
whose recursively "first" member or element is such a type). The alignment
associated with these types for subsequent members use an alignment value
where the floating-point data type is considered to have 4-byte alignment.

For the purposes of the foregoing: vtable pointers, non-empty base classes,
and zero-width bit-fields count as prior members; members of empty class
types marked no_unique_address are not considered to be prior members.

Notes:

  1. ASTRecordLayout Layout.Alignment records the min alignment/ABI align of object, which is also the value _Alignof(x) should return;

2.Layout.NonVirtualAlignment records min alignment of nv part of object

  1. The size/nvsize of object will be rounded up to PreferredAlignment/PreferredNVAlignment on AIX.

4.getPreferredTypeAlign = __alignof(x) = preferred alignment of object

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Xiangling_L updated this revision to Diff 275728.EditedJul 6 2020, 8:49 AM
Xiangling_L marked 3 inline comments as done.

Rebased on the latest master;
Fixed -Wpacked warning issue;
Fixed EmptySubobjects related offset issue;
Fixed zero-extent array in a base class related issue;
Addressed other comments;

clang/lib/AST/ASTContext.cpp
2456

Please add a comment regarding the situations where the ABIAlign value is greater than the PreferredAlignment value. It may be appropriate to assert that, absent those cases, the PreferredAlignment value is at least that of ABIAlign.

2456

It does not appear that the maximum of the two values is the correct answer:

struct C {
  double x;
} c;
typedef struct C __attribute__((__aligned__(2))) CC;

CC cc;
extern char x[__alignof__(cc)];
extern char x[2]; // this is okay with IBM XL C/C++
clang/lib/AST/RecordLayoutBuilder.cpp
1929

It seems that overriding the value should only be done after additional checks:

typedef double __attribute__((__aligned__(2))) Dbl;
struct A {
  Dbl x;
} a;
extern char x[__alignof__(a)];
extern char x[2]; // this is okay with IBM XL C/C++

I am getting concerned that the logic here overlaps quite a bit with getPreferredTypeAlign and refactoring to make the code here more common with getPreferredTypeAlign is necessary.

clang/lib/AST/RecordLayoutBuilder.cpp
1926

castAs should be declared:
https://clang.llvm.org/doxygen/classclang_1_1Type.html#a436b8b08ae7f2404b4712d37986194ce

Also, the additional point is that the if here should be unnecessary. BTy should not be null. We should use castAs to gain the guarantee (with assertions) that BTy is not null.

Xiangling_L marked 6 inline comments as done.

Fixed the typedef related issues;
Added more testcases;

clang/lib/AST/ASTContext.cpp
2456

Please add a comment regarding the situations where the ABIAlign value is greater than the PreferredAlignment value.

I added a if condition to guard the situation where ABIAlign should be returned instead of adding a comment. Please let me know if that is sufficient.

clang/lib/AST/RecordLayoutBuilder.cpp
1929

Fixed the typedef related cases with my new changes, and the overlaps were not a lot as I expected. So I didn't do any refactoring yet. Please let me know if you still think it's necessary to refactor the code somehow.

clang/lib/AST/RecordLayoutBuilder.cpp
666

Minor nit: s/as base/as a base/;

703

Minor nit: Please fix the formatting.

1222

Thanks; verified that this is correct with xlclang++ from IBM XL C/C++ for AIX with:

struct A {
  char x;
};
struct B {
  int x;
};
struct __attribute__((__packed__)) C : A, B {} c;

Length is 5:

[10]    m   0x00000004      .bss     1  extern                    c
[11]    a4  0x00000005       0    0     CM       RW    0    0

@Xiangling_L, I suggest adding a case for this to the tests.

1268

Unions cannot have base classes. Please assert !IsUnion.

1272

Suggestion:

// The maximum field alignment overrides the base align/(AIX-only) preferred
// base align.
clang/test/Layout/aix-power-alignment-typedef.cpp
19

Instead of checking the value of a, the alignment can be checked more directly from the IR for cc.

clang/lib/AST/ASTContext.cpp
2461

Minor nit: s/is/should be/;

clang/lib/AST/RecordLayoutBuilder.cpp
1208

Just set HandledFirstNonOverlappingEmptyField to true with a comment before the if:
By handling a base class that is not empty, we're handling the "first (inherited) member".

1821

Add a comment before the if:

// We're going to handle the "first member" based on
// `FoundNonOverlappingEmptyFieldToHandle` during the current invocation of
// this function; record it as handled for future invocations.

Given the rationale from the comment, move the subject if to immediately after the determination of FoundNonOverlappingEmptyFieldToHandle. That way, the setting of HandledFirstNonOverlappingEmptyField becomes less complicated to track.

1852

It seems that the code to set AlignIsRequired is missing from this path.

typedef double Dbl __attribute__((__aligned__(2)));
typedef Dbl DblArr[];

union U {
  DblArr fam;
  char x;
};

U u[2];
extern char x[sizeof(u)];
extern char x[4];
1925

I think the if condition above is too complicated as a filter for setting HandledFirstNonOverlappingEmptyField. I would prefer if we don't need to set HandledFirstNonOverlappingEmptyField here. Please see my other comment about HandledFirstNonOverlappingEmptyField.

clang/lib/AST/RecordLayoutBuilder.cpp
1927

"Qualified" is a term of art in the context of C/C++ types. Please remove "IfQualified" from the name. The lambda just needs to be named for what it does. When naming it for the conditions where it should be called, "if" (as opposed to "assuming") implies that the function checks the condition itself.

1944

Please don't write a check on a variable right after making an assertion on what its value should be.

Xiangling_L marked 14 inline comments as done.

Fixed typedef issue on incomplete array field and add a test for it;
Added a test for where pack attribute on object also apply on base classes;
Addressed other comments;

clang/lib/AST/RecordLayoutBuilder.cpp
1222

Sure, I will add it.

clang/lib/AST/RecordLayoutBuilder.cpp
1068

I would suggest setting HandledFirstNonOverlappingEmptyField to true here with an assertion that the current type is not a union.

1804

The condition is still more complex than I think it should be.

If we have found a "first" other-than-overlapping-empty-field, then we should set HandledFirstNonOverlappingEmptyField to true for non-union cases.

If HandledFirstNonOverlappingEmptyField being false is not enough for FieldOffset == CharUnits::Zero() to be true, then I think the correction would be to set HandledFirstNonOverlappingEmptyField in more places.

I would like to remove the check on FieldOffset == CharUnits::Zero() from here and instead have an assertion that !HandledFirstNonOverlappingEmptyField implies FieldOffset == CharUnits::Zero().

Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX cases, we should remove the DefaultsToAIXPowerAlignment condition for what is currently named FoundFirstNonOverlappingEmptyFieldToHandle (adjusting uses of it as necessary) and rename FoundFirstNonOverlappingEmptyFieldToHandle to FoundFirstNonOverlappingEmptyField.

1851–1852

ATy seems to be an unused variable now.

1853

I guess this works (we have a test for it), but the previous code made a point to use the element type and not the array type (and the comment above says we can't directly query getTypeInfo with the array type). @Xiangling_L, can you confirm if the comment is out-of-date and update it?

1923

It should now be the case that FoundFirstNonOverlappingEmptyFieldToHandle is true for all union members that are not empty, meaning that the IsUnion part of the check only serves to admit attempts to handle types that are empty (and thus does not have subobjects that would induce an alignment upgrade).

clang/lib/AST/RecordLayoutBuilder.cpp
1269

This needs to check HandledFirstNonOverlappingEmptyField:

struct A {
  char x[0];
};
struct B {
  double d;
};
struct C : A, B { char x; } c;
1270

Note that PreferredBaseAlign is only truly meaningful after this line, and related fields, such as UnpackedPreferredBaseAlign, require similar treatment. With UnpackedAlignTo being dependent on a meaningful value of UnpackedPreferredBaseAlign, it needs an update here as well.

Consider the following source. Noting that C and D are identical except for the packed attribute and yield identical layout properties despite that difference, we should be getting a -Wpacked warning with -Wpacked (but it is missing).

struct A {
  double d;
};

struct B {
  char x[8];
};

struct [[gnu::packed]] C : B, A { // expected-warning {{packed attribute is unnecessary}}
  char x alignas(4)[8];
} c;

struct D : B, A {
  char x alignas(4)[8];
} d;
*** Dumping AST Record Layout
         0 | struct C
         0 |   struct B (base)
         0 |     char [8] x
         8 |   struct A (base)
         8 |     double d
        16 |   char [8] x
           | [sizeof=24, dsize=24, align=4, preferredalign=4,
           |  nvsize=24, nvalign=4, preferrednvalign=4]
*** Dumping AST Record Layout
         0 | struct D
         0 |   struct B (base)
         0 |     char [8] x
         8 |   struct A (base)
         8 |     double d
        16 |   char [8] x
           | [sizeof=24, dsize=24, align=4, preferredalign=4,
           |  nvsize=24, nvalign=4, preferrednvalign=4]
Xiangling_L marked 9 inline comments as done.Jul 8 2020, 12:18 PM
Xiangling_L added inline comments.
clang/lib/AST/RecordLayoutBuilder.cpp
1804

Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX cases, we should remove the DefaultsToAIXPowerAlignment condition for what is currently named FoundFirstNonOverlappingEmptyFieldToHandle

I am not sure if we want to remove the DefaultsToAIXPowerAlignment condition and bother with maintaining correct status of HandledFirstNonOverlappingEmptyField for other targets.

We are actually claiming HandledFirstNonOverlappingEmptyField is an auxiliary flag used for AIX only in its definition comments.

Besides, if we do want to manage HandledFirstNonOverlappingEmptyField in non-AIX cases, I noticed that we have to set this flag to true somewhere for objective-C++ cases.

1853

I am sure getTypeInfo can recognize the element type for IncompleteArray. I will update the comments.

Xiangling_L marked 2 inline comments as done.

Fixed a -Wpacked related case and added the case to the tests;
Fixed the base class related code issue;
Addressed other comments;

clang/lib/AST/RecordLayoutBuilder.cpp
1804

Okay, the other option I'm open is setting HandledFirstNonOverlappingEmptyField to true up front when not dealing with AIX power alignment.

Xiangling_L marked an inline comment as done.Jul 8 2020, 1:03 PM
Xiangling_L added inline comments.
clang/lib/AST/RecordLayoutBuilder.cpp
1804

Thanks, that works too. I will address it in the next commit.

clang/lib/AST/RecordLayoutBuilder.cpp
1264–1265

It seems this is a leftover copy of the code that has been moved above?

1803

The rename I suggested in my previous round of review was in coordination with maintaining the value not just for AIX. Since we're only maintaining the value for AIX, I prefer the previous name (or FoundFirstNonOverlappingEmptyFieldForAIX).

1804

Please merge the if conditions to reduce nesting:

if (DefaultsToAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField) {
1811

Keep this reference to the variable up-to-date with its name.

1814

Change the condition of the if here to !IsOverlappingEmptyField and move the setting of FoundFirstNonOverlappingEmptyField to true into this if.

Move the previous comment and merge it with this one here:

[ ... ] record it as handled for future invocations (except for unions, because the current field does not represent all "firsts").

1924

Sorry for not seeing this earlier (I only notice some things when I hide the inline comments). I think performBuiltinTypeAlignmentUpgrade would read better at the call site (and better capture the checking, which is based on the kind of built-in type, that is within the lambda).

clang/test/Layout/aix-Wpacked.cpp
9 ↗(On Diff #276528)

Clang diagnostics are normally checked using -verify (as opposed to FileCheck). To use it, I think this needs to be split into the "expecting no diagnostics" and the "expecting diagnostics" cases. As it is, I think the CHECK-NOT has a problem because it checks for plain 'Q'.

clang/lib/AST/RecordLayoutBuilder.cpp
1210

We need some sort of IsFirstNonEmptyBase to record that the current base qualifies for the alignment upgrade:

struct A { double x; };
struct B : A {} b;
1261

Query !IsFirstNonEmptyBase instead of HandledFirstNonOverlappingEmptyField here.

Xiangling_L marked 9 inline comments as done.

Fixed a base class related case by adding IsFirstNonEmpty flag;
Split the aix-Wpacked.cpp testcase into two;
Addressed other comments;

clang/lib/AST/RecordLayoutBuilder.cpp
1245

Move the comment to above the previous if and make the following if the else of the previous if.

1804
clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
20

Minor nit: Remove the space before the semicolon.

clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
16

Is there a reason to drop the FileCheck checking for the layout?

Xiangling_L marked 6 inline comments as done.

Set Handled... = true for non-AIX power alignment;
Addressed other comments;

clang/lib/AST/RecordLayoutBuilder.cpp
1243

IsFirstNonEmptyBase can be removed. It is set, but not used.

Removed unused var;

Xiangling_L added inline comments.Jul 13 2020, 7:46 AM
clang/test/Layout/aix-Wpacked-no-diagnostics.cpp
16

I dropped the FileCheck because the layout of QQ and Q are fairly simple and wanted the test focus more on no-diagnostics side. But I can add it back if that helps.

I've done another pass over the code (but did not get through the tests). I have no comments about the code at this time. My understanding is that @jasonliu will be doing another pass over this patch, so he can approve while I'm away on vacation.

jasonliu added inline comments.Jul 22 2020, 11:32 AM
clang/lib/AST/ASTContext.cpp
2454

Should this if statement go above the if (const auto *RT = T->getAs<RecordType>()) ?
When Target does not allow larger prefered type alignment, then we should return ABIAlign immediately without going through the RecordType query?

clang/lib/AST/RecordLayoutBuilder.cpp
1234

Add const?

1801

const ?

clang/lib/Basic/Targets/PPC.h
427

SuitableAlign is set in line 412 as well. Please consider combining the two if statements if grouping things together makes code easier to parse.

clang/test/Layout/aix-double-struct-member.cpp
3

You are not using < %s here. So -x c++ is redundant?

306

b is not necessary when you take the sizeof of B below?

clang/test/Layout/aix-no-unique-address-with-double.cpp
139

I think this case is interesting and may worth adding too:

struct F {
  [[no_unique_address]] Empty emp, emp2;
  double d;
};
clang/test/Layout/aix-power-alignment-typedef.cpp
13

remove c ?

26

remove a?

44

remove u?

Xiangling_L marked 13 inline comments as done.Jul 22 2020, 1:16 PM
Xiangling_L added inline comments.
clang/lib/AST/ASTContext.cpp
2454

Agree. I will update this.

clang/test/Layout/aix-double-struct-member.cpp
3

Yeah, thanks, I will remove it.

clang/test/Layout/aix-no-unique-address-with-double.cpp
139

Sure.

Xiangling_L marked 3 inline comments as done.

Add one more testcase;
Addressed other comments;

Thanks. No further comments from me.

jasonliu accepted this revision.Jul 22 2020, 1:37 PM
  • Simplified the test command line;
  • Split the typedef related tests into two to address the LIT testcase failure on windows platform;
This revision was automatically updated to reflect the committed changes.
ebevhan added a subscriber: ebevhan.Aug 3 2020, 4:43 AM
ebevhan added inline comments.
clang/lib/AST/RecordLayoutBuilder.cpp
1778

It seems that in factoring out this to setDeclInfo, it was changed from using getTypeInfoInChars to using getTypeInfo+toCharUnitsFromBits. This is causing some downstream issues for us with non-bytesize-multiple types.

Changing setDeclInfo to use the original function instead seems to work without issues. Would it be acceptable to change this?

efriedma added inline comments.Aug 3 2020, 11:41 AM
clang/lib/AST/RecordLayoutBuilder.cpp
1778

In general, we want to avoid representing type size/alignment in bits where it isn't necessary; refactoring along those lines is welcome.

I submitted a patch with the changes at D85191.