[clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

[dzhidzhoev]

Summary

RecordLayoutBuilder assumes the size of a potentially-overlapping field
with non-zero size as the size of the base subobject type corresponding
to the field type.
Make CGRecordLayoutBuilder to acknowledge that in order to avoid incorrect
padding insertion.

Without this patch, test fails on assertion

Assertion failed: (Offset >= Size), function insertPadding, file CGRecordLayoutBuilder.cpp, line 802.

[efriedma]

clang/include/clang/AST/Decl.h

3063

Maybe clarify that this also checks the object is of class type? That's technically not part of the C++ standard definition of a "potentially-overlapping subobject". (I'm not sure what rule, if any, prevents non-class objects from overlapping.)

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

386

Extra parentheses.

clang/test/CodeGenCXX/no-unique-address-3.cpp

60

It would be nice to also explicitly check the LLVM IR types, instead of just checking that emitting LLVM IR doesn't crash.

(We also try to avoid using -emit-obj from clang tests, since that only works if the relevant backend is linked into clang.)

[dzhidzhoev]

Addressed @efriedma comments.

[efriedma]

LGTM

[dzhidzhoev]

clang-format'ed.

[rupprecht]

I'm not sure what the libcxx failure was that caused you to revert this, but we also saw a clang crasher as a result of this. clang/lib/AST/Decl.cpp:4300 in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: isBitField() && "not a bitfield". I'll try to reduce it.

[rupprecht]

In D139741#4000201, @rupprecht wrote:

I'm not sure what the libcxx failure was that caused you to revert this, but we also saw a clang crasher as a result of this. clang/lib/AST/Decl.cpp:4300 in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: isBitField() && "not a bitfield". I'll try to reduce it.

Here:

template <class F, class X, class Y>
struct Foo {
  Foo(F, X, Y y) : y_(y) {}
  Y y_;
  union FooHolder {
    int foo;
  };
  [[no_unique_address]] FooHolder foo_holder_;
};
template <typename Factory, typename X, typename Y>
void MakeFoo(Factory f, X x, Y y) {
  Foo(f, x, y);
}
void Start() {
  MakeFoo(0, int(), [] {});
}

[dzhidzhoev]

In D139741#4000201, @rupprecht wrote:

I'm not sure what the libcxx failure was that caused you to revert this, but we also saw a clang crasher as a result of this. clang/lib/AST/Decl.cpp:4300 in unsigned int clang::FieldDecl::getBitWidthValue(const ASTContext &) const: isBitField() && "not a bitfield". I'll try to reduce it.

Thank you!
The problem was that we should not try to get the base subobject class of union fields.

[dzhidzhoev]

Ignore union fields. Get base subobject of struct and class types only.

[efriedma]

Does the following work with the updated patch?

class Empty {};
class UnionClass : Empty {
  [[no_unique_address]] union X {
  private:
    Empty x;
    alignas(2) char C;
  } U;
  char C;
};
UnionClass L;

[dzhidzhoev]

In D139741#4002244, @efriedma wrote:

Does the following work with the updated patch?

class Empty {};
class UnionClass : Empty {
  [[no_unique_address]] union X {
  private:
    Empty x;
    alignas(2) char C;
  } U;
  char C;
};
UnionClass L;

Unfortunately no, it fails on the same assert as the current trunk as soon as unions are ignored.

*** Dumping AST Record Layout
         0 | class Empty (empty)
           | [sizeof=1, dsize=1, align=1,
           |  nvsize=1, nvalign=1]

*** Dumping AST Record Layout
         0 | union UnionClass::X
         0 |   class Empty x (empty)
         0 |   char C
           | [sizeof=2, dsize=1, align=2,
           |  nvsize=1, nvalign=2]

*** Dumping AST Record Layout
         0 | class UnionClass
         0 |   class Empty (base) (empty)
         2 |   union UnionClass::X U
         2 |     class Empty x (empty)
         2 |     char C
         3 |   char C
           | [sizeof=4, dsize=4, align=2,
           |  nvsize=4, nvalign=2]

[dzhidzhoev]

Fixed crashes reported by @efriedma and @rupprecht.
"Base subobject type" layout (e.g. type layout without suffix padding)
is calculated for unions and final classes too here, in order to
lay out fields with [[no_unique_address]].

[dzhidzhoev]

Clang-format'ted

[efriedma]

LGTM

[dzhidzhoev]

Fixed crash for 'struct C' in no-unique-address-3.cpp.
Without the fix, packedness for regular union layout in provided test
and for [[no_unique_address]] union layout turns out to be different.
This causes assertion on CGRecordLayoutBuilder.cpp:902.
Fix is similar to what is done on CGRecordLayoutBuilder.cpp:790,794.

[efriedma]

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

369

Should this be if (Layout.getSize() % StorageAlignment || Layout.getDataSize() % StorageAlignment)? The dependency on isNoUniqueAddress is a bit confusing.

[dzhidzhoev]

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

369

For base class subobjects, base class DataSize is used to compute the whole object size in RecordLayoutBuilder.cpp, and base subobject LLVM type (with ".base" suffix) is computed in addition to standard layout LLVM type in CGRecordLayoutBuilder.cpp.

Currently, both base subobject and standard layout LLVM types of the same class are supposed to agree on packedness, as stated in bb51300970b7. Thus, if one is packed, both are marked as packed, as done in CGRecordLowering::determinePacked.

For data members of class/struct type, declared with [[no_unique_address]] attribute, DataSize is used to compute the whole object size in RecordLayoutBuilder.cpp, but standard layout LLVM type is used to represent the field in the whole class's LLVM type.
This patch proposes to use the base subobject LLVM type instead of the default LLVM type of a class to lay out a no_unique_address member of this class type.

Nextly, the patch proposes to create an unpadded LLVM type for unions, in addition to the default LLVM type. It is used to lay out the union members having no_unique_address attribute. Existing code for creating base subobject layout of classes are reused for unions, therefore, packedness of potentially-overlapping union LLVM type and of default union LLVM types must agree as well as for classes.

[dzhidzhoev]

Existing code creating base subobject layout of classes is reused for unions *

[efriedma]

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

369

That doesn't really get at what I was asking.

LayoutSize % StorageAlignment || Layout.getDataSize() % StorageAlignment is basically equivalent to isNoUniqueAddress ? Layout.getDataSize() % StorageAlignment != 0 : (Layout.getSize() % StorageAlignment || Layout.getDataSize() % StorageAlignment). From my understanding, you want isNoUniqueAddress=true to agree with isNoUniqueAddress=false on whether the result is packed. So why are the checks different?

[dzhidzhoev]

Simplified condition in lowerUnion.

[dzhidzhoev]

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

369

Thanks, sorry for misunderstanding.

It seems that if Layout.getSize() isn't aligned by StorageAlignment, Layout.getDataSize() isn't aligned by it too, thus Layout.getDataSize() % StorageAlignment is enough in that condition (at least, it doesn't break any check-clang tests).
I have added an assert on that assumption.

[efriedma]

LGTM

clang/include/clang/AST/Decl.h

public:
   /// bit-fields.
   bool isZeroLengthBitField(const ASTContext &Ctx) const;
 
   /// Determine if this field is a subobject of zero size, that is, either a
   /// zero-length bit-field or a field of empty class type with the
   /// [[no_unique_address]] attribute.
   bool isZeroSize(const ASTContext &Ctx) const;
 
+  /// Determine if this field is a potentially-overlapping, that is,
+  /// subobject with the [[no_unique_address]] attribute
+  bool isPotentiallyOverlapping() const;
+
   /// Get the kind of (C++11) default member initializer that this field has.
   InClassInitStyle getInClassInitStyle() const {
     InitStorageKind storageKind = InitStorage.getInt();
     return (storageKind == ISK_CapturedVLAType
               ? ICIS_NoInit : (InClassInitStyle) storageKind);
   }
 
   /// Determine whether this member has a C++11 default member initializer.

clang/lib/AST/Decl.cpp

bool FieldDecl::isZeroSize(const ASTContext &Ctx) const {
 
   // Otherwise, [...] the circumstances under which the object has zero size
   // are implementation-defined.
   // FIXME: This might be Itanium ABI specific; we don't yet know what the MS
   // ABI will do.
   return true;
 }
 
+bool FieldDecl::isPotentiallyOverlapping() const {
+  return hasAttr<NoUniqueAddressAttr>() && getType()->getAsCXXRecordDecl();
+}
+
 unsigned FieldDecl::getFieldIndex() const {
   const FieldDecl *Canonical = getCanonicalDecl();
   if (Canonical != this)
     return Canonical->getFieldIndex();
 
   if (CachedFieldIndex) return CachedFieldIndex - 1;
 
   unsigned Index = 0;

clang/lib/AST/RecordLayoutBuilder.cpp

UnadjustedAlignment =
       std::max(UnadjustedAlignment, Context.toCharUnitsFromBits(FieldAlign));
   UpdateAlignment(Context.toCharUnitsFromBits(FieldAlign),
                   Context.toCharUnitsFromBits(UnpackedFieldAlign));
 }
 
 void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
                                              bool InsertExtraPadding) {
   auto *FieldClass = D->getType()->getAsCXXRecordDecl();
-  bool PotentiallyOverlapping = D->hasAttr<NoUniqueAddressAttr>() && FieldClass;
   bool IsOverlappingEmptyField =
-      PotentiallyOverlapping && FieldClass->isEmpty();
+      D->isPotentiallyOverlapping() && FieldClass->isEmpty();
 
   CharUnits FieldOffset =
       (IsUnion || IsOverlappingEmptyField) ? CharUnits::Zero() : getDataSize();
 
   const bool DefaultsToAIXPowerAlignment =
       Context.getTargetInfo().defaultsToAIXPowerAlignment();
   bool FoundFirstNonOverlappingEmptyFieldForAIX = false;
   if (DefaultsToAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField) {
void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
 
   if (D->getType()->isIncompleteArrayType()) {
     setDeclInfo(true /* IsIncompleteArrayType */);
   } else {
     setDeclInfo(false /* IsIncompleteArrayType */);
 
     // A potentially-overlapping field occupies its dsize or nvsize, whichever
     // is larger.
-    if (PotentiallyOverlapping) {
+    if (D->isPotentiallyOverlapping()) {
       const ASTRecordLayout &Layout = Context.getASTRecordLayout(FieldClass);
       EffectiveFieldSize =
           std::max(Layout.getNonVirtualSize(), Layout.getDataSize());
     }
 
     if (IsMsStruct) {
       // If MS bitfield layout is required, figure out what type is being
       // laid out and align the field to the width of that type.

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

void CGRecordLowering::lowerUnion() {
   // If our storage size was bigger than our required size (can happen in the
   // case of packed bitfields on Itanium) then just use an I8 array.
   if (LayoutSize < getSize(StorageType))
     StorageType = getByteArrayType(LayoutSize);
   FieldTypes.push_back(StorageType);
   appendPaddingBytes(LayoutSize - getSize(StorageType));
   // Set packed if we need it.
   if (LayoutSize % getAlignment(StorageType))
     Packed = true;
 }
 
 void CGRecordLowering::accumulateFields() {
   for (RecordDecl::field_iterator Field = D->field_begin(),
                                   FieldEnd = D->field_end();
     Field != FieldEnd;) {
     if (Field->isBitField()) {
       RecordDecl::field_iterator Start = Field;
       // Iterate to gather the list of bitfields.
       for (++Field; Field != FieldEnd && Field->isBitField(); ++Field);
       accumulateBitFields(Start, Field);
     } else if (!Field->isZeroSize(Context)) {
+      // Use base subobject layout for the potentially-overlapping field,
+      // as it is done in RecordLayoutBuilder
       Members.push_back(MemberInfo(
           bitsToCharUnits(getFieldBitOffset(*Field)), MemberInfo::Field,
-          getStorageType(*Field), *Field));
+          (Field->isPotentiallyOverlapping()
+               ? getStorageType(Field->getType()->getAsCXXRecordDecl())
+               : getStorageType(*Field)),
+          *Field));
       ++Field;
     } else {
       ++Field;
     }
   }
 }
 
 void

clang/test/CodeGenCXX/no-unique-address-3.cpp

+// RUN: %clang_cc1 -triple aarch64-unknown-linux-gnu -emit-obj -fdump-record-layouts -std=c++17 %s -o %t | FileCheck %s
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class Empty (empty)
+// CHECK-NEXT:             | [sizeof=1, dsize=1, align=1,
+// CHECK-NEXT:             |  nvsize=1, nvalign=1]
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class Second
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:       0:0-0 |   short A
+// CHECK-NEXT:             | [sizeof=2, dsize=1, align=2,
+// CHECK-NEXT:             |  nvsize=1, nvalign=2]
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class Foo
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:           2 |   class Second NZNoUnique
+// CHECK-NEXT:           2 |     class Empty (base) (empty)
+// CHECK-NEXT:       2:0-0 |     short A
+// CHECK-NEXT:           3 |   char B
+// CHECK-NEXT:             | [sizeof=4, dsize=4, align=2,
+// CHECK-NEXT:             |  nvsize=4, nvalign=2]
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class SecondEmpty (empty)
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:             | [sizeof=1, dsize=0, align=1,
+// CHECK-NEXT:             |  nvsize=1, nvalign=1]
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class Bar
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:           1 |   class SecondEmpty ZNoUnique (empty)
+// CHECK-NEXT:           1 |     class Empty (base) (empty)
+// CHECK-NEXT:           0 |   char C
+// CHECK-NEXT:             | [sizeof=2, dsize=1, align=1,
+// CHECK-NEXT:             |  nvsize=2, nvalign=1]
+
+// CHECK:       *** Dumping AST Record Layout
+// CHECK-NEXT:           0 | class IntFieldClass
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:           2 |   class Second Field
+// CHECK-NEXT:           2 |     class Empty (base) (empty)
+// CHECK-NEXT:       2:0-0 |     short A
+// CHECK-NEXT:           4 |   int C
+// CHECK-NEXT:             | [sizeof=8, dsize=8, align=4,
+// CHECK-NEXT:             |  nvsize=8, nvalign=4]
+
+class Empty {};
+
+class Second : Empty {
+  short A : 1;
+};
+
+class Foo : Empty {
+  [[no_unique_address]] Second NZNoUnique;
+  char B;
+};
+Foo I;
+
+class SecondEmpty: Empty {
+};
+class Bar : Empty {
+  [[no_unique_address]] SecondEmpty ZNoUnique;
+  char C;
+};
+Bar J;
+
+class IntFieldClass : Empty {
+  [[no_unique_address]] Second Field;
+  int C;
+};
+IntFieldClass K;