dzhidzhoev updated this revision to Diff 491807.
dzhidzhoev added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139741/new/

https://reviews.llvm.org/D139741

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/no-unique-address-3.cpp

Index: clang/test/CodeGenCXX/no-unique-address-3.cpp
===================================================================
--- /dev/null
+++ clang/test/CodeGenCXX/no-unique-address-3.cpp
@@ -0,0 +1,209 @@
+// RUN: %clang_cc1 -triple aarch64-unknown-linux-gnu -emit-llvm -fdump-record-layouts -std=c++17 %s -o %t | FileCheck %s
+
+// CHECK-LABEL:          0 | class Empty (empty)
+// CHECK-NEXT:             | [sizeof=1, dsize=1, align=1,
+// CHECK-NEXT:             |  nvsize=1, nvalign=1]
+// CHECK-LABEL:          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-LABEL:          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]
+
+class Empty {};
+
+// CHECK-LABEL: LLVMType:%class.Second = type { i8, i8 }
+// CHECK-NEXT:  NonVirtualBaseLLVMType:%class.Second.base = type { i8 }
+class Second : Empty {
+  short A : 1;
+};
+
+// CHECK-LABEL:   LLVMType:%class.Foo = type { [2 x i8], %class.Second.base, i8 }
+// CHECK-NEXT:    NonVirtualBaseLLVMType:%class.Foo = type { [2 x i8], %class.Second.base, i8 }
+class Foo : Empty {
+  [[no_unique_address]] Second NZNoUnique;
+  char B;
+};
+Foo I;
+
+// CHECK-LABEL:          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]
+class SecondEmpty: Empty {
+};
+
+// CHECK-LABEL:          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-LABEL:  LLVMType:%class.Bar = type { i8, i8 }
+// CHECK-NEXT:   NonVirtualBaseLLVMType:%class.Bar = type { i8, i8 }
+class Bar : Empty {
+  [[no_unique_address]] SecondEmpty ZNoUnique;
+  char C;
+};
+Bar J;
+
+// CHECK-LABEL:          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]
+
+// CHECK-LABEL:   LLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second.base, i32 }
+// CHECK-NEXT:    NonVirtualBaseLLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second.base, i32 }
+class IntFieldClass : Empty {
+  [[no_unique_address]] Second Field;
+  int C;
+};
+IntFieldClass K;
+
+// CHECK-LABEL:         0 | class UnionClass
+// CHECK-NEXT:          0 |   class Empty (base) (empty)
+// CHECK-NEXT:          0 |   union UnionClass
+// CHECK-NEXT:          0 |     int I
+// CHECK-NEXT:          0 |     char C
+// CHECK-NEXT:          4 |   int C
+// CHECK-NEXT:            | [sizeof=8, dsize=8, align=4,
+// CHECK-NEXT:            |  nvsize=8, nvalign=4]
+
+// CHECK-LABEL:  LLVMType:%class.UnionClass = type { %union.anon, i32 }
+// CHECK-NEXT:   NonVirtualBaseLLVMType:%class.UnionClass = type { %union.anon, i32 }
+// CHECK-NEXT:   IsZeroInitializable:1
+// CHECK-NEXT:   BitFields:[
+// CHECK-NEXT: ]>
+class UnionClass : Empty {
+  [[no_unique_address]] union {
+    int I;
+    char C;
+  } U;
+  int C;
+};
+UnionClass L;
+
+// CHECK-LABEL:         0 | class EnumClass
+// CHECK-NEXT:          0 |   class Empty (base) (empty)
+// CHECK-NEXT:          0 |   enum E A
+// CHECK-NEXT:          4 |   int C
+// CHECK-NEXT:            | [sizeof=8, dsize=8, align=4,
+// CHECK-NEXT:            |  nvsize=8, nvalign=4]
+
+// CHECK-LABEL:  LLVMType:%class.EnumClass = type { i32, i32 }
+// CHECK-NEXT:   NonVirtualBaseLLVMType:%class.EnumClass = type { i32, i32 }
+// CHECK-NEXT:   IsZeroInitializable:1
+// CHECK-NEXT:   BitFields:[
+// CHECK-NEXT: ]>
+class EnumClass : Empty {
+  [[no_unique_address]] enum class E { X, Y, Z } A;
+  int C;
+};
+EnumClass M;
+
+// CHECK-LABEL:         0 | class NoBaseField
+// CHECK-NEXT:          0 |   class Empty (base) (empty)
+// CHECK-NEXT:          1 |   class Empty A (empty)
+// CHECK-NEXT:          0 |   int B
+// CHECK-NEXT:            | [sizeof=4, dsize=4, align=4,
+// CHECK-NEXT:            |  nvsize=4, nvalign=4]
+
+// CHECK-LABEL:  LLVMType:%class.NoBaseField = type { i32 }
+// CHECK-NEXT:   NonVirtualBaseLLVMType:%class.NoBaseField = type { i32 }
+// CHECK-NEXT:   IsZeroInitializable:1
+// CHECK-NEXT:   BitFields:[
+// CHECK-NEXT: ]>
+class NoBaseField : Empty {
+  [[no_unique_address]] Empty A;
+  int B;
+};
+NoBaseField N;
+
+// CHECK-LABEL:        0 | class FinalEmpty (empty)
+// CHECK-NEXT:           | [sizeof=1, dsize=1, align=1,
+// CHECK-NEXT:           |  nvsize=1, nvalign=1]
+
+// CHECK-LABEL:        0 | class FinalClass
+// CHECK-NEXT:         0 |   class Empty (base) (empty)
+// CHECK-NEXT:         0 |   class FinalEmpty A (empty)
+// CHECK-NEXT:         0 |   int B
+// CHECK-NEXT:           | [sizeof=4, dsize=4, align=4,
+// CHECK-NEXT:           |  nvsize=4, nvalign=4]
+class FinalEmpty final {};
+class FinalClass final : Empty {
+  [[no_unique_address]] FinalEmpty A;
+  int B;
+} O;
+
+
+// CHECK-LABEL:        0 | union Union2Class::PaddedUnion
+// CHECK-NEXT:         0 |   class Empty A (empty)
+// CHECK-NEXT:         0 |   char B
+// CHECK-NEXT:           | [sizeof=2, dsize=1, align=2,
+// CHECK-NEXT:           |  nvsize=1, nvalign=2]
+
+// CHECK-LABEL:        0 | class Union2Class
+// CHECK-NEXT:         0 |   class Empty (base) (empty)
+// CHECK-NEXT:         2 |   union Union2Class::PaddedUnion U
+// CHECK-NEXT:         2 |     class Empty A (empty)
+// CHECK-NEXT:         2 |     char B
+// CHECK-NEXT:         3 |   char C
+// CHECK-NEXT:           | [sizeof=4, dsize=4, align=2,
+// CHECK-NEXT:           |  nvsize=4, nvalign=2]
+class Union2Class : Empty {
+  [[no_unique_address]] union PaddedUnion {
+  private:
+    Empty A;
+    alignas(2) char B;
+  } U;
+  char C;
+} P;
+
+// CHECK-LABEL:          0 | struct NotEmptyWithBitfield
+// CHECK-NEXT:           0 |   class Empty (base) (empty)
+// CHECK-NEXT:           0 |   char[2] A
+// CHECK-NEXT:       2:0-0 |   int B
+// CHECK-NEXT:             | [sizeof=4, dsize=3, align=4,
+// CHECK-NEXT:             |  nvsize=3, nvalign=4]
+
+// CHECK-LABEL:          0 | union C::
+// CHECK-NEXT:           0 |   short C
+// CHECK-NEXT:           0 |   struct NotEmptyWithBitfield A
+// CHECK-NEXT:           0 |     class Empty (base) (empty)
+// CHECK-NEXT:           0 |     char[2] A
+// CHECK-NEXT:       2:0-0 |     int B
+// CHECK-NEXT:             | [sizeof=4, dsize=3, align=4,
+// CHECK-NEXT:             |  nvsize=3, nvalign=4]
+
+// CHECK-LABEL:          0 | struct C
+// CHECK-NEXT:           0 |   union C::
+// CHECK-NEXT:           0 |     short C
+// CHECK-NEXT:           0 |     struct NotEmptyWithBitfield A
+// CHECK-NEXT:           0 |       class Empty (base) (empty)
+// CHECK-NEXT:           0 |       char[2] A
+// CHECK-NEXT:       2:0-0 |       int B
+// CHECK-NEXT:             | [sizeof=4, dsize=3, align=4,
+// CHECK-NEXT:             |  nvsize=3, nvalign=4]
+struct NotEmptyWithBitfield : Empty {
+  char A[2];
+  int B : 1;
+};
+struct C {
+  [[no_unique_address]] union {
+    short C;
+    [[no_unique_address]] NotEmptyWithBitfield A;
+  } U;
+} Q;
Index: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
===================================================================
--- clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -182,7 +182,7 @@
                        llvm::Type *StorageType);
   /// Lowers an ASTRecordLayout to a llvm type.
   void lower(bool NonVirtualBaseType);
-  void lowerUnion();
+  void lowerUnion(bool isNoUniqueAddress);
   void accumulateFields();
   void accumulateBitFields(RecordDecl::field_iterator Field,
                            RecordDecl::field_iterator FieldEnd);
@@ -280,7 +280,7 @@
   //    CodeGenTypes::ComputeRecordLayout.
   CharUnits Size = NVBaseType ? Layout.getNonVirtualSize() : Layout.getSize();
   if (D->isUnion()) {
-    lowerUnion();
+    lowerUnion(NVBaseType);
     computeVolatileBitfields();
     return;
   }
@@ -308,8 +308,9 @@
   computeVolatileBitfields();
 }
 
-void CGRecordLowering::lowerUnion() {
-  CharUnits LayoutSize = Layout.getSize();
+void CGRecordLowering::lowerUnion(bool isNoUniqueAddress) {
+  CharUnits LayoutSize =
+      isNoUniqueAddress ? Layout.getDataSize() : Layout.getSize();
   llvm::Type *StorageType = nullptr;
   bool SeenNamedMember = false;
   // Iterate through the fields setting bitFieldInfo and the Fields array. Also
@@ -365,7 +366,8 @@
   FieldTypes.push_back(StorageType);
   appendPaddingBytes(LayoutSize - getSize(StorageType));
   // Set packed if we need it.
-  if (LayoutSize % getAlignment(StorageType))
+  const auto StorageAlignment = getAlignment(StorageType);
+  if (LayoutSize % StorageAlignment || Layout.getDataSize() % StorageAlignment)
     Packed = true;
 }
 
@@ -379,9 +381,14 @@
       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;
@@ -882,7 +889,7 @@
 
   // If we're in C++, compute the base subobject type.
   llvm::StructType *BaseTy = nullptr;
-  if (isa<CXXRecordDecl>(D) && !D->isUnion() && !D->hasAttr<FinalAttr>()) {
+  if (isa<CXXRecordDecl>(D)) {
     BaseTy = Ty;
     if (Builder.Layout.getNonVirtualSize() != Builder.Layout.getSize()) {
       CGRecordLowering BaseBuilder(*this, D, /*Packed=*/Builder.Packed);
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1853,9 +1853,8 @@
 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();
@@ -1916,7 +1915,7 @@
 
     // 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());
Index: clang/lib/AST/Decl.cpp
===================================================================
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4355,6 +4355,10 @@
   return true;
 }
 
+bool FieldDecl::isPotentiallyOverlapping() const {
+  return hasAttr<NoUniqueAddressAttr>() && getType()->getAsCXXRecordDecl();
+}
+
 unsigned FieldDecl::getFieldIndex() const {
   const FieldDecl *Canonical = getCanonicalDecl();
   if (Canonical != this)
Index: clang/include/clang/AST/Decl.h
===================================================================
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -3072,6 +3072,10 @@
   /// [[no_unique_address]] attribute.
   bool isZeroSize(const ASTContext &Ctx) const;
 
+  /// Determine if this field is of potentially-overlapping class type, 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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to