[clang] Implement [[msvc::no_unique_address]] (PR #65675)

Amy Huang via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 12 16:31:17 PDT 2023


https://github.com/amykhuang updated https://github.com/llvm/llvm-project/pull/65675:

>From 923a43cd6386f6e57023fd8928eed0dc0ab04d57 Mon Sep 17 00:00:00 2001
From: Amy Huang <akhuang at google.com>
Date: Fri, 21 Jul 2023 16:30:30 -0700
Subject: [PATCH 1/3] Implement [[msvc::no_unique_address]]

This attribute should match the behavior of MSVC's [[msvc::no_unique_address]] attribute.

Bug: https://github.com/llvm/llvm-project/issues/49358

Differential Revision: https://reviews.llvm.org/D157762
---
 clang/include/clang/Basic/Attr.td           |   8 +-
 clang/include/clang/Basic/AttrDocs.td       |   4 +
 clang/lib/AST/Decl.cpp                      |   8 +
 clang/lib/AST/RecordLayoutBuilder.cpp       | 191 +++++++++--
 clang/lib/CodeGen/CGRecordLayoutBuilder.cpp |   1 +
 clang/lib/Sema/SemaDeclAttr.cpp             |  17 +
 clang/test/Layout/ms-no-unique-address.cpp  | 338 ++++++++++++++++++++
 7 files changed, 540 insertions(+), 27 deletions(-)
 create mode 100644 clang/test/Layout/ms-no-unique-address.cpp

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index c95db7e8049d47a..23e56cda0f67e9d 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1798,11 +1798,13 @@ def ArmMveStrictPolymorphism : TypeAttr, TargetSpecificAttr<TargetARM> {
   let Documentation = [ArmMveStrictPolymorphismDocs];
 }
 
-def NoUniqueAddress : InheritableAttr, TargetSpecificAttr<TargetItaniumCXXABI> {
-  let Spellings = [CXX11<"", "no_unique_address", 201803>];
+def NoUniqueAddress : InheritableAttr {
+  let Spellings = [CXX11<"", "no_unique_address", 201803>,
+                   CXX11<"msvc", "no_unique_address", 201803>];
+  let Accessors = [Accessor<"isDefault", [CXX11<"", "no_unique_address", 201803>]>,
+                   Accessor<"isMSVC", [CXX11<"msvc", "no_unique_address", 201803>]>];
   let Subjects = SubjectList<[NonBitField], ErrorDiag>;
   let Documentation = [NoUniqueAddressDocs];
-  let SimpleHandler = 1;
 }
 
 def ReturnsTwice : InheritableAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index f11ea89d14bad0d..21e6373611272b5 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -1405,6 +1405,10 @@ Example usage:
 
 ``[[no_unique_address]]`` is a standard C++20 attribute. Clang supports its use
 in C++11 onwards.
+
+On MSVC targets, ``[[no_unique_address]]`` is ignored; use
+``[[msvc::no_unique_address]]`` instead. Currently there is no guarantee of ABI
+compatibility or stability.
   }];
 }
 
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 60c80f2b075336b..d1770330070a519 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4505,6 +4505,14 @@ bool FieldDecl::isZeroSize(const ASTContext &Ctx) const {
   if (!CXXRD->isEmpty())
     return false;
 
+  // MS ABI: nonzero if class type with class type fields
+  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft() &&
+      llvm::any_of(CXXRD->fields(), [](const FieldDecl *Field) {
+        return Field->getType()->getAs<RecordType>();
+      })) {
+    return false;
+  }
+
   // 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
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 8afd88ae7be27b3..2152e69732d65c9 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2545,7 +2545,10 @@ struct MicrosoftRecordLayoutBuilder {
     CharUnits Alignment;
   };
   typedef llvm::DenseMap<const CXXRecordDecl *, CharUnits> BaseOffsetsMapTy;
-  MicrosoftRecordLayoutBuilder(const ASTContext &Context) : Context(Context) {}
+  MicrosoftRecordLayoutBuilder(const ASTContext &Context,
+                               EmptySubobjectMap *EmptySubobjects)
+      : Context(Context), EmptySubobjects(EmptySubobjects) {}
+
 private:
   MicrosoftRecordLayoutBuilder(const MicrosoftRecordLayoutBuilder &) = delete;
   void operator=(const MicrosoftRecordLayoutBuilder &) = delete;
@@ -2562,7 +2565,11 @@ struct MicrosoftRecordLayoutBuilder {
   void layoutNonVirtualBase(const CXXRecordDecl *RD,
                             const CXXRecordDecl *BaseDecl,
                             const ASTRecordLayout &BaseLayout,
-                            const ASTRecordLayout *&PreviousBaseLayout);
+                            const ASTRecordLayout *&PreviousBaseLayout,
+                            BaseSubobjectInfo *Base);
+  BaseSubobjectInfo *computeBaseSubobjectInfo(const CXXRecordDecl *RD,
+                                              bool IsVirtual,
+                                              BaseSubobjectInfo *Derived);
   void injectVFPtr(const CXXRecordDecl *RD);
   void injectVBPtr(const CXXRecordDecl *RD);
   /// Lays out the fields of the record.  Also rounds size up to
@@ -2595,6 +2602,12 @@ struct MicrosoftRecordLayoutBuilder {
       llvm::SmallPtrSetImpl<const CXXRecordDecl *> &HasVtorDispSet,
       const CXXRecordDecl *RD) const;
   const ASTContext &Context;
+  EmptySubobjectMap *EmptySubobjects;
+  llvm::SpecificBumpPtrAllocator<BaseSubobjectInfo> BaseSubobjectInfoAllocator;
+  typedef llvm::DenseMap<const CXXRecordDecl *, BaseSubobjectInfo *>
+      BaseSubobjectInfoMapTy;
+  BaseSubobjectInfoMapTy VirtualBaseInfo;
+
   /// The size of the record being laid out.
   CharUnits Size;
   /// The non-virtual size of the record layout.
@@ -2818,6 +2831,8 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
   // Iterate through the bases and lay out the non-virtual ones.
   for (const CXXBaseSpecifier &Base : RD->bases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+    BaseSubobjectInfo *BaseInfo =
+        computeBaseSubobjectInfo(BaseDecl, Base.isVirtual(), nullptr);
     HasPolymorphicBaseClass |= BaseDecl->isPolymorphic();
     const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
     // Mark and skip virtual bases.
@@ -2839,7 +2854,8 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
       LeadsWithZeroSizedBase = BaseLayout.leadsWithZeroSizedBase();
     }
     // Lay out the base.
-    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout);
+    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout,
+                         BaseInfo);
   }
   // Figure out if we need a fresh VFPtr for this class.
   if (RD->isPolymorphic()) {
@@ -2864,10 +2880,21 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
   bool CheckLeadingLayout = !PrimaryBase;
   // Iterate through the bases and lay out the non-virtual ones.
   for (const CXXBaseSpecifier &Base : RD->bases()) {
-    if (Base.isVirtual())
-      continue;
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
     const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
+    BaseSubobjectInfo *BaseInfo =
+        computeBaseSubobjectInfo(BaseDecl, Base.isVirtual(), nullptr);
+
+    if (Base.isVirtual()) {
+      // Mark offset for virtual base.
+      CharUnits Offset = CharUnits::Zero();
+      while (!EmptySubobjects->CanPlaceBaseAtOffset(BaseInfo, Offset)) {
+        ElementInfo Info = getAdjustedElementInfo(BaseLayout);
+        Offset += Info.Alignment;
+      }
+      continue;
+    }
+
     // Only lay out bases without extendable VFPtrs on the second pass.
     if (BaseLayout.hasExtendableVFPtr()) {
       VBPtrOffset = Bases[BaseDecl] + BaseLayout.getNonVirtualSize();
@@ -2880,7 +2907,8 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
       LeadsWithZeroSizedBase = BaseLayout.leadsWithZeroSizedBase();
     }
     // Lay out the base.
-    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout);
+    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout,
+                         BaseInfo);
     VBPtrOffset = Bases[BaseDecl] + BaseLayout.getNonVirtualSize();
   }
   // Set our VBPtroffset if we know it at this point.
@@ -2908,10 +2936,9 @@ static bool recordUsesEBO(const RecordDecl *RD) {
 }
 
 void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
-    const CXXRecordDecl *RD,
-    const CXXRecordDecl *BaseDecl,
+    const CXXRecordDecl *RD, const CXXRecordDecl *BaseDecl,
     const ASTRecordLayout &BaseLayout,
-    const ASTRecordLayout *&PreviousBaseLayout) {
+    const ASTRecordLayout *&PreviousBaseLayout, BaseSubobjectInfo *Base) {
   // Insert padding between two bases if the left first one is zero sized or
   // contains a zero sized subobject and the right is zero sized or one leads
   // with a zero sized base.
@@ -2927,7 +2954,7 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
   if (UseExternalLayout) {
     FoundBase = External.getExternalNVBaseOffset(BaseDecl, BaseOffset);
     if (BaseOffset > Size) {
-      Size = BaseOffset;
+      DataSize = Size = BaseOffset;
     }
   }
 
@@ -2937,14 +2964,97 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
       BaseOffset = CharUnits::Zero();
     } else {
       // Otherwise, lay the base out at the end of the MDC.
-      BaseOffset = Size = Size.alignTo(Info.Alignment);
+      BaseOffset = DataSize = Size = Size.alignTo(Info.Alignment);
     }
+
+    // Place in EmptySubobjects map but don't check the position? MSVC seems to
+    // not allow fields to overlap at the end of a virtual base, but they can
+    // overlap with other bass.
+    EmptySubobjects->CanPlaceBaseAtOffset(Base, BaseOffset);
   }
+
   Bases.insert(std::make_pair(BaseDecl, BaseOffset));
   Size += BaseLayout.getNonVirtualSize();
+  DataSize = Size;
   PreviousBaseLayout = &BaseLayout;
 }
 
+BaseSubobjectInfo *MicrosoftRecordLayoutBuilder::computeBaseSubobjectInfo(
+    const CXXRecordDecl *RD, bool IsVirtual, BaseSubobjectInfo *Derived) {
+  // This is copied directly from ItaniumRecordLayoutBuilder::ComputeBaseSubobjectInfo.
+  BaseSubobjectInfo *Info;
+
+  if (IsVirtual) {
+    // Check if we already have info about this virtual base.
+    BaseSubobjectInfo *&InfoSlot = VirtualBaseInfo[RD];
+    if (InfoSlot) {
+      assert(InfoSlot->Class == RD && "Wrong class for virtual base info!");
+      return InfoSlot;
+    }
+
+    // We don't, create it.
+    InfoSlot = new (BaseSubobjectInfoAllocator.Allocate()) BaseSubobjectInfo;
+    Info = InfoSlot;
+  } else {
+    Info = new (BaseSubobjectInfoAllocator.Allocate()) BaseSubobjectInfo;
+  }
+
+  Info->Class = RD;
+  Info->IsVirtual = IsVirtual;
+  Info->Derived = nullptr;
+  Info->PrimaryVirtualBaseInfo = nullptr;
+
+  const CXXRecordDecl *PrimaryVirtualBase = nullptr;
+  BaseSubobjectInfo *PrimaryVirtualBaseInfo = nullptr;
+
+  // Check if this base has a primary virtual base.
+  if (RD->getNumVBases()) {
+    const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+    if (Layout.isPrimaryBaseVirtual()) {
+      // This base does have a primary virtual base.
+      PrimaryVirtualBase = Layout.getPrimaryBase();
+      assert(PrimaryVirtualBase && "Didn't have a primary virtual base!");
+
+      // Now check if we have base subobject info about this primary base.
+      PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
+
+      if (PrimaryVirtualBaseInfo) {
+        if (PrimaryVirtualBaseInfo->Derived) {
+          // We did have info about this primary base, and it turns out that it
+          // has already been claimed as a primary virtual base for another
+          // base.
+          PrimaryVirtualBase = nullptr;
+        } else {
+          // We can claim this base as our primary base.
+          Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;
+          PrimaryVirtualBaseInfo->Derived = Info;
+        }
+      }
+    }
+  }
+
+  // Now go through all direct bases.
+  for (const auto &I : RD->bases()) {
+    bool IsVirtual = I.isVirtual();
+
+    const CXXRecordDecl *BaseDecl = I.getType()->getAsCXXRecordDecl();
+
+    Info->Bases.push_back(computeBaseSubobjectInfo(BaseDecl, IsVirtual, Info));
+  }
+
+  if (PrimaryVirtualBase && !PrimaryVirtualBaseInfo) {
+    // Traversing the bases must have created the base info for our primary
+    // virtual base.
+    PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
+    assert(PrimaryVirtualBaseInfo && "Did not create a primary virtual base!");
+
+    // Claim the primary virtual base as our primary virtual base.
+    Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;
+    PrimaryVirtualBaseInfo->Derived = Info;
+  }
+  return Info;
+}
+
 void MicrosoftRecordLayoutBuilder::layoutFields(const RecordDecl *RD) {
   LastFieldIsNonZeroWidthBitfield = false;
   for (const FieldDecl *Field : RD->fields())
@@ -2956,18 +3066,48 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
     layoutBitField(FD);
     return;
   }
+
   LastFieldIsNonZeroWidthBitfield = false;
   ElementInfo Info = getAdjustedElementInfo(FD);
   Alignment = std::max(Alignment, Info.Alignment);
-  CharUnits FieldOffset;
-  if (UseExternalLayout)
+
+  const CXXRecordDecl *FieldClass = FD->getType()->getAsCXXRecordDecl();
+  bool IsOverlappingEmptyField = FD->isPotentiallyOverlapping() &&
+                                 FieldClass->isEmpty() &&
+                                 FieldClass->fields().empty();
+  const CXXRecordDecl *ParentClass = cast<CXXRecordDecl>(FD->getParent());
+  bool HasBases =
+      !ParentClass->bases().empty() || !ParentClass->vbases().empty();
+
+  CharUnits FieldOffset = CharUnits::Zero();
+
+  if (UseExternalLayout) {
     FieldOffset =
         Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD));
-  else if (IsUnion)
+  } else if (IsUnion) {
     FieldOffset = CharUnits::Zero();
-  else
+  } else if (EmptySubobjects) {
+    if (!IsOverlappingEmptyField)
+      FieldOffset = DataSize.alignTo(Info.Alignment);
+
+    while (!EmptySubobjects->CanPlaceFieldAtOffset(FD, FieldOffset)) {
+      if (FieldOffset == CharUnits::Zero() && DataSize != CharUnits::Zero() &&
+          HasBases) {
+        // MSVC appears to only do this when there are base classes;
+        // otherwise it overlaps no_unique_address fields in non-zero offsets.
+        FieldOffset = DataSize.alignTo(Info.Alignment);
+      } else {
+        FieldOffset += Info.Alignment;
+      }
+    }
+  } else {
     FieldOffset = Size.alignTo(Info.Alignment);
+  }
   placeFieldAtOffset(FieldOffset);
+
+  if (!IsOverlappingEmptyField)
+    DataSize = std::max(DataSize, FieldOffset + Info.Size);
+
   Size = std::max(Size, FieldOffset + Info.Size);
 }
 
@@ -2999,17 +3139,17 @@ void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
     auto NewSize = Context.toCharUnitsFromBits(
         llvm::alignDown(FieldBitOffset, Context.toBits(Info.Alignment)) +
         Context.toBits(Info.Size));
-    Size = std::max(Size, NewSize);
+    DataSize = Size = std::max(Size, NewSize);
     Alignment = std::max(Alignment, Info.Alignment);
   } else if (IsUnion) {
     placeFieldAtOffset(CharUnits::Zero());
-    Size = std::max(Size, Info.Size);
+    DataSize = Size = std::max(Size, Info.Size);
     // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
     // Allocate a new block of memory and place the bitfield in it.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
     placeFieldAtOffset(FieldOffset);
-    Size = FieldOffset + Info.Size;
+    DataSize = Size = FieldOffset + Info.Size;
     Alignment = std::max(Alignment, Info.Alignment);
     RemainingBitsInField = Context.toBits(Info.Size) - Width;
   }
@@ -3029,13 +3169,13 @@ MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
   ElementInfo Info = getAdjustedElementInfo(FD);
   if (IsUnion) {
     placeFieldAtOffset(CharUnits::Zero());
-    Size = std::max(Size, Info.Size);
+    DataSize = Size = std::max(Size, Info.Size);
     // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
     // Round up the current record size to the field's alignment boundary.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
     placeFieldAtOffset(FieldOffset);
-    Size = FieldOffset;
+    DataSize = Size = FieldOffset;
     Alignment = std::max(Alignment, Info.Alignment);
   }
 }
@@ -3055,7 +3195,7 @@ void MicrosoftRecordLayoutBuilder::injectVBPtr(const CXXRecordDecl *RD) {
     // It is possible that there were no fields or bases located after vbptr,
     // so the size was not adjusted before.
     if (Size < FieldStart)
-      Size = FieldStart;
+      DataSize = Size = FieldStart;
     return;
   }
   // Make sure that the amount we push the fields back by is a multiple of the
@@ -3103,6 +3243,7 @@ void MicrosoftRecordLayoutBuilder::injectVFPtr(const CXXRecordDecl *RD) {
 void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
   if (!HasVBPtr)
     return;
+
   // Vtordisps are always 4 bytes (even in 64-bit mode)
   CharUnits VtorDispSize = CharUnits::fromQuantity(4);
   CharUnits VtorDispAlignment = VtorDispSize;
@@ -3136,7 +3277,7 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
     if ((PreviousBaseLayout && PreviousBaseLayout->endsWithZeroSizedObject() &&
          BaseLayout.leadsWithZeroSizedBase() && !recordUsesEBO(RD)) ||
         HasVtordisp) {
-      Size = Size.alignTo(VtorDispAlignment) + VtorDispSize;
+      DataSize = Size = Size.alignTo(VtorDispAlignment) + VtorDispSize;
       Alignment = std::max(VtorDispAlignment, Alignment);
     }
     // Insert the virtual base.
@@ -3154,7 +3295,7 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
 
     VBases.insert(std::make_pair(BaseDecl,
         ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
-    Size = BaseOffset + BaseLayout.getNonVirtualSize();
+    DataSize = Size = BaseOffset + BaseLayout.getNonVirtualSize();
     PreviousBaseLayout = &BaseLayout;
   }
 }
@@ -3304,8 +3445,9 @@ ASTContext::getASTRecordLayout(const RecordDecl *D) const {
   const ASTRecordLayout *NewEntry = nullptr;
 
   if (isMsLayout(*this)) {
-    MicrosoftRecordLayoutBuilder Builder(*this);
     if (const auto *RD = dyn_cast<CXXRecordDecl>(D)) {
+      EmptySubobjectMap EmptySubobjects(*this, RD);
+      MicrosoftRecordLayoutBuilder Builder(*this, &EmptySubobjects);
       Builder.cxxLayout(RD);
       NewEntry = new (*this) ASTRecordLayout(
           *this, Builder.Size, Builder.Alignment, Builder.Alignment,
@@ -3317,6 +3459,7 @@ ASTContext::getASTRecordLayout(const RecordDecl *D) const {
           Builder.EndsWithZeroSizedObject, Builder.LeadsWithZeroSizedBase,
           Builder.Bases, Builder.VBases);
     } else {
+      MicrosoftRecordLayoutBuilder Builder(*this, /*EmptySubobjects*/ nullptr);
       Builder.layout(D);
       NewEntry = new (*this) ASTRecordLayout(
           *this, Builder.Size, Builder.Alignment, Builder.Alignment,
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 270ff11559417d9..792ff3ce45cf3a3 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -743,6 +743,7 @@ void CGRecordLowering::calculateZeroInit() {
 void CGRecordLowering::clipTailPadding() {
   std::vector<MemberInfo>::iterator Prior = Members.begin();
   CharUnits Tail = getSize(Prior->Data);
+
   for (std::vector<MemberInfo>::iterator Member = Prior + 1,
                                          MemberEnd = Members.end();
        Member != MemberEnd; ++Member) {
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 59e0f3e83cfdd80..370dfdeb800b38f 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8368,6 +8368,20 @@ static void handleNoMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(NoMergeAttr::Create(S.Context, AL));
 }
 
+static void handleNoUniqueAddressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  NoUniqueAddressAttr TmpAttr(S.Context, AL);
+  if (S.getLangOpts().MSVCCompat) {
+    if (TmpAttr.isDefault()) {
+      S.Diag(AL.getLoc(), diag::warn_attribute_ignored) << AL;
+      return;
+    }
+  } else if (TmpAttr.isMSVC()) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_ignored) << AL;
+    return;
+  }
+  D->addAttr(NoUniqueAddressAttr::Create(S.Context, AL));
+}
+
 static void handleSYCLKernelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   // The 'sycl_kernel' attribute applies only to function templates.
   const auto *FD = cast<FunctionDecl>(D);
@@ -9273,6 +9287,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_NoMerge:
     handleNoMergeAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_NoUniqueAddress:
+    handleNoUniqueAddressAttr(S, D, AL);
+    break;
 
   case ParsedAttr::AT_AvailableOnlyInDefaultEvalMethod:
     handleAvailableOnlyInDefaultEvalMethod(S, D, AL);
diff --git a/clang/test/Layout/ms-no-unique-address.cpp b/clang/test/Layout/ms-no-unique-address.cpp
new file mode 100644
index 000000000000000..b03c63cf9bc67f0
--- /dev/null
+++ b/clang/test/Layout/ms-no-unique-address.cpp
@@ -0,0 +1,338 @@
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -triple x86_64-windows-msvc -fms-compatibility -fdump-record-layouts %s | FileCheck %s
+
+namespace Empty {
+  struct A {};
+  struct A2 {};
+  struct A3 { [[msvc::no_unique_address]] A a; };
+  struct alignas(8) A4 {};
+
+  struct B {
+    [[msvc::no_unique_address]] A a;
+    char b;
+  };
+  static_assert(sizeof(B) == 1);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::B
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:     0 |   char b
+  // CHECK-NEXT:       | [sizeof=1, align=1,
+  // CHECK-NEXT:       |  nvsize=1, nvalign=1]
+
+  struct C {
+    [[msvc::no_unique_address]] A a;
+    [[msvc::no_unique_address]] A2 a2;
+    char c;
+  };
+  static_assert(sizeof(C) == 1);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::C
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:     0 |   struct Empty::A2 a2 (empty)
+  // CHECK-NEXT:     0 |   char c
+  // CHECK-NEXT:       | [sizeof=1, align=1,
+  // CHECK-NEXT:       |  nvsize=1, nvalign=1]
+
+  struct D {
+    [[msvc::no_unique_address]] A3 a;
+    int i;
+  };
+  static_assert(sizeof(D) == 8);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::D
+  // CHECK-NEXT:     0 |   struct Empty::A3 a (empty)
+  // CHECK-NEXT:     0 |     struct Empty::A a (empty)
+  // CHECK-NEXT:     4 |   int i
+  // CHECK-NEXT:       | [sizeof=8, align=4,
+  // CHECK-NEXT:       |  nvsize=8, nvalign=4]
+
+  struct E {
+    [[msvc::no_unique_address]] A a1;
+    [[msvc::no_unique_address]] A a2;
+    char e;
+  };
+  static_assert(sizeof(E) == 2);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::E
+  // CHECK-NEXT:     0 |   struct Empty::A a1 (empty)
+  // CHECK-NEXT:     1 |   struct Empty::A a2 (empty)
+  // CHECK-NEXT:     0 |   char e
+  // CHECK-NEXT:       | [sizeof=2, align=1,
+  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+
+  struct F {
+    ~F();
+    [[msvc::no_unique_address]] A a1;
+    [[msvc::no_unique_address]] A a2;
+    char f;
+  };
+  static_assert(sizeof(F) == 2);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::F
+  // CHECK-NEXT:     0 |   struct Empty::A a1 (empty)
+  // CHECK-NEXT:     1 |   struct Empty::A a2 (empty)
+  // CHECK-NEXT:     0 |   char f
+  // CHECK-NEXT:       | [sizeof=2, align=1,
+  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+
+  struct G { [[msvc::no_unique_address]] A a; ~G(); };
+  static_assert(sizeof(G) == 1);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::G
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:       | [sizeof=1, align=1,
+  // CHECK-NEXT:       |  nvsize=1, nvalign=1]
+
+  struct H {
+    [[msvc::no_unique_address]] A a;
+    [[msvc::no_unique_address]] A b;
+    ~H();
+  };
+  static_assert(sizeof(H) == 2);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::H
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:     1 |   struct Empty::A b (empty)
+  // CHECK-NEXT:       | [sizeof=2, align=1,
+  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+
+  struct I {
+    [[msvc::no_unique_address]] A4 a;
+    [[msvc::no_unique_address]] A4 b;
+  };
+  static_assert(sizeof(I) == 16);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::I
+  // CHECK-NEXT:     0 |   struct Empty::A4 a (empty)
+  // CHECK-NEXT:     8 |   struct Empty::A4 b (empty)
+  // CHECK-NEXT:       | [sizeof=16, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct J {
+    [[msvc::no_unique_address]] A4 a;
+    A4 b;
+  };
+  static_assert(sizeof(J) == 16);
+
+  // MSVC puts a and b at the same offset.
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::J
+  // CHECK-NEXT:     0 |   struct Empty::A4 a (empty)
+  // CHECK-NEXT:     8 |   struct Empty::A4 b (empty)
+  // CHECK-NEXT:       | [sizeof=16, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct K {
+    [[msvc::no_unique_address]] A4 a;
+    [[msvc::no_unique_address]] char c;
+    [[msvc::no_unique_address]] A4 b;
+  };
+  static_assert(sizeof(K) == 16);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::K
+  // CHECK-NEXT:     0 |   struct Empty::A4 a (empty)
+  // CHECK-NEXT:     0 |   char c
+  // CHECK-NEXT:     8 |   struct Empty::A4 b (empty)
+  // CHECK-NEXT:       | [sizeof=16, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct OversizedEmpty : A {
+    ~OversizedEmpty();
+    [[msvc::no_unique_address]] A a;
+  };
+  static_assert(sizeof(OversizedEmpty) == 2);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::OversizedEmpty
+  // CHECK-NEXT:     0 |   struct Empty::A (base) (empty)
+  // CHECK-NEXT:     1 |   struct Empty::A a (empty)
+  // CHECK-NEXT:       | [sizeof=2, align=1,
+  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+
+  struct HasOversizedEmpty {
+    [[msvc::no_unique_address]] OversizedEmpty m;
+  };
+  static_assert(sizeof(HasOversizedEmpty) == 2);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::HasOversizedEmpty
+  // CHECK-NEXT:     0 |   struct Empty::OversizedEmpty m (empty)
+  // CHECK-NEXT:     0 |     struct Empty::A (base) (empty)
+  // CHECK-NEXT:     1 |     struct Empty::A a (empty)
+  // CHECK-NEXT:       | [sizeof=2, align=1,
+  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+
+  struct EmptyWithNonzeroDSize {
+    [[msvc::no_unique_address]] A a;
+    int x;
+    [[msvc::no_unique_address]] A b;
+    int y;
+    [[msvc::no_unique_address]] A c;
+  };
+  static_assert(sizeof(EmptyWithNonzeroDSize) == 8);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::EmptyWithNonzeroDSize
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:     0 |   int x
+  // CHECK-NEXT:     1 |   struct Empty::A b (empty)
+  // CHECK-NEXT:     4 |   int y
+  // CHECK-NEXT:     2 |   struct Empty::A c (empty)
+  // CHECK-NEXT:       | [sizeof=8,  align=4,
+  // CHECK-NEXT:       |  nvsize=8, nvalign=4]
+
+  struct EmptyWithNonzeroDSizeNonPOD {
+    ~EmptyWithNonzeroDSizeNonPOD();
+    [[msvc::no_unique_address]] A a;
+    int x;
+    [[msvc::no_unique_address]] A b;
+    int y;
+    [[msvc::no_unique_address]] A c;
+  };
+  static_assert(sizeof(EmptyWithNonzeroDSizeNonPOD) == 8);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct Empty::EmptyWithNonzeroDSizeNonPOD
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:     0 |   int x
+  // CHECK-NEXT:     1 |   struct Empty::A b (empty)
+  // CHECK-NEXT:     4 |   int y
+  // CHECK-NEXT:     2 |   struct Empty::A c (empty)
+  // CHECK-NEXT:       | [sizeof=8, align=4,
+  // CHECK-NEXT:       |  nvsize=8, nvalign=4]
+}
+
+namespace POD {
+  struct A { int n; char c[3]; };
+  struct B { [[msvc::no_unique_address]] A a; char d; };
+  static_assert(sizeof(B) == 12);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct POD::B
+  // CHECK-NEXT:     0 |   struct POD::A a
+  // CHECK-NEXT:     0 |     int n
+  // CHECK-NEXT:     4 |     char[3] c
+  // CHECK-NEXT:     8 |   char d
+  // CHECK-NEXT:       | [sizeof=12,  align=4,
+  // CHECK-NEXT:       |  nvsize=12, nvalign=4]
+}
+
+namespace NonPOD {
+  struct A { int n; char c[3]; ~A(); };
+  struct B { [[msvc::no_unique_address]] A a; char d; };
+  static_assert(sizeof(B) == 12);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct NonPOD::B
+  // CHECK-NEXT:     0 |   struct NonPOD::A a
+  // CHECK-NEXT:     0 |     int n
+  // CHECK-NEXT:     4 |     char[3] c
+  // CHECK-NEXT:     8 |   char d
+  // CHECK-NEXT:       | [sizeof=12, align=4,
+  // CHECK-NEXT:       |  nvsize=12, nvalign=4]
+}
+
+namespace VBases {
+  // The nvsize of an object includes the complete size of its empty subobjects
+  // (although it's unclear why). Ensure this corner case is handled properly.
+  struct Empty {};
+  struct alignas(8) A {}; // dsize 0, nvsize 0, size 8
+  struct B : A { char c; }; // dsize 1, nvsize 8, size 8
+  static_assert(sizeof(B) == 8);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct VBases::B
+  // CHECK-NEXT:     0 |   struct VBases::A (base) (empty)
+  // CHECK-NEXT:     0 |   char c
+  // CHECK-NEXT:       | [sizeof=8, align=8,
+  // CHECK-NEXT:       |  nvsize=8, nvalign=8]
+
+  struct V { int n; };
+
+  struct C : B, virtual V {};
+  static_assert(sizeof(C) == 24);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct VBases::C
+  // CHECK-NEXT:     0 |   struct VBases::B (base)
+  // CHECK-NEXT:     0 |     struct VBases::A (base) (empty)
+  // CHECK-NEXT:     0 |     char c
+  // CHECK-NEXT:     8 |   (C vbtable pointer)
+  // CHECK-NEXT:    16 |   struct VBases::V (virtual base)
+  // CHECK-NEXT:    16 |     int n
+  // CHECK-NEXT:       | [sizeof=24, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct D : virtual Empty {
+    [[msvc::no_unique_address]] Empty a;
+  };
+  static_assert(sizeof(D) == 16);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct VBases::D
+  // CHECK-NEXT:     0 |   (D vbtable pointer)
+  // CHECK-NEXT:     9 |   struct VBases::Empty a
+  // CHECK-NEXT:    16 |   struct VBases::Empty (virtual base) (empty)
+  // CHECK-NEXT:       | [sizeof=16, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct E : virtual V {
+    [[msvc::no_unique_address]] B b;
+  };
+  static_assert(sizeof(E) == 24);
+
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct VBases::E
+  // CHECK-NEXT:     0 |   (E vbtable pointer)
+  // CHECK-NEXT:     8 |   struct VBases::B b
+  // CHECK-NEXT:     8 |     struct VBases::A (base) (empty)
+  // CHECK-NEXT:     8 |     char c
+  // CHECK-NEXT:    16 |   struct VBases::V (virtual base)
+  // CHECK-NEXT:    16 |     int n
+  // CHECK-NEXT:       | [sizeof=24, align=8,
+  // CHECK-NEXT:       |  nvsize=16, nvalign=8]
+
+  struct X : virtual A { [[msvc::no_unique_address]] A a; };
+  struct F : virtual A {
+    [[msvc::no_unique_address]] A a;
+    [[msvc::no_unique_address]] X x;
+  };
+  static_assert(sizeof(F) == 32);
+
+  // MSVC places x after a and the total size is 48.
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:          0 | struct VBases::F
+  // CHECK-NEXT:     0 |   (F vbtable pointer)
+  // CHECK-NEXT:    16 |   struct VBases::A a (empty)
+  // CHECK-NEXT:     8 |   struct VBases::X x
+  // CHECK-NEXT:     8 |     (X vbtable pointer)
+  // CHECK-NEXT:    24 |     struct VBases::A a (empty)
+  // CHECK-NEXT:    32 |     struct VBases::A (virtual base) (empty)
+  // CHECK-NEXT:    32 |   struct VBases::A (virtual base) (empty)
+  // CHECK-NEXT:       | [sizeof=32, align=8,
+  // CHECK-NEXT:       |  nvsize=32, nvalign=8]
+
+  struct G : virtual Empty {
+    int i;
+    [[msvc::no_unique_address]] A a;
+  };
+  static_assert(sizeof(G) == 16);
+
+  // MSVC places a at offset 12.
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:         0 | struct VBases::G
+  // CHECK-NEXT:    0 |   (G vbtable pointer)
+  // CHECK-NEXT:    8 |   int i
+  // CHECK-NEXT:    8 |   struct VBases::A a (empty)
+  // CHECK-NEXT:   16 | struct VBases::Empty (virtual base) (empty)
+  // CHECK-NEXT:      | [sizeof=16, align=8,
+  // CHECK-NEXT:      |  nvsize=16, nvalign=8]
+}

>From b336a0b149bc3b1a7742b301a702b71c7afd16da Mon Sep 17 00:00:00 2001
From: Amy Huang <akhuang at google.com>
Date: Tue, 12 Sep 2023 12:50:35 -0700
Subject: [PATCH 2/3] Address comments and update some test cases

---
 clang/lib/AST/Decl.cpp                        |  18 +--
 clang/lib/AST/RecordLayoutBuilder.cpp         | 137 +++---------------
 clang/test/Layout/ms-no-unique-address.cpp    |  32 ++--
 clang/test/Preprocessor/has_attribute.cpp     |   2 +-
 .../test/SemaCXX/cxx2a-no-unique-address.cpp  |   2 +-
 5 files changed, 45 insertions(+), 146 deletions(-)

diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index d1770330070a519..c99d5f6c19a15d6 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4505,19 +4505,15 @@ bool FieldDecl::isZeroSize(const ASTContext &Ctx) const {
   if (!CXXRD->isEmpty())
     return false;
 
-  // MS ABI: nonzero if class type with class type fields
-  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft() &&
-      llvm::any_of(CXXRD->fields(), [](const FieldDecl *Field) {
-        return Field->getType()->getAs<RecordType>();
-      })) {
-    return false;
-  }
-
   // 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;
+  if (!Ctx.getTargetInfo().getCXXABI().isMicrosoft())
+    return true;
+
+  // MS ABI: nonzero if class type with class type fields
+  return !llvm::any_of(CXXRD->fields(), [](const FieldDecl *Field) {
+    return Field->getType()->getAs<RecordType>();
+  });
 }
 
 bool FieldDecl::isPotentiallyOverlapping() const {
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 2152e69732d65c9..2f5b3be413a7b9e 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2565,11 +2565,7 @@ struct MicrosoftRecordLayoutBuilder {
   void layoutNonVirtualBase(const CXXRecordDecl *RD,
                             const CXXRecordDecl *BaseDecl,
                             const ASTRecordLayout &BaseLayout,
-                            const ASTRecordLayout *&PreviousBaseLayout,
-                            BaseSubobjectInfo *Base);
-  BaseSubobjectInfo *computeBaseSubobjectInfo(const CXXRecordDecl *RD,
-                                              bool IsVirtual,
-                                              BaseSubobjectInfo *Derived);
+                            const ASTRecordLayout *&PreviousBaseLayout);
   void injectVFPtr(const CXXRecordDecl *RD);
   void injectVBPtr(const CXXRecordDecl *RD);
   /// Lays out the fields of the record.  Also rounds size up to
@@ -2831,8 +2827,6 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
   // Iterate through the bases and lay out the non-virtual ones.
   for (const CXXBaseSpecifier &Base : RD->bases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
-    BaseSubobjectInfo *BaseInfo =
-        computeBaseSubobjectInfo(BaseDecl, Base.isVirtual(), nullptr);
     HasPolymorphicBaseClass |= BaseDecl->isPolymorphic();
     const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
     // Mark and skip virtual bases.
@@ -2854,8 +2848,7 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
       LeadsWithZeroSizedBase = BaseLayout.leadsWithZeroSizedBase();
     }
     // Lay out the base.
-    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout,
-                         BaseInfo);
+    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout);
   }
   // Figure out if we need a fresh VFPtr for this class.
   if (RD->isPolymorphic()) {
@@ -2882,18 +2875,9 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
   for (const CXXBaseSpecifier &Base : RD->bases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
     const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
-    BaseSubobjectInfo *BaseInfo =
-        computeBaseSubobjectInfo(BaseDecl, Base.isVirtual(), nullptr);
 
-    if (Base.isVirtual()) {
-      // Mark offset for virtual base.
-      CharUnits Offset = CharUnits::Zero();
-      while (!EmptySubobjects->CanPlaceBaseAtOffset(BaseInfo, Offset)) {
-        ElementInfo Info = getAdjustedElementInfo(BaseLayout);
-        Offset += Info.Alignment;
-      }
+    if (Base.isVirtual())
       continue;
-    }
 
     // Only lay out bases without extendable VFPtrs on the second pass.
     if (BaseLayout.hasExtendableVFPtr()) {
@@ -2907,8 +2891,7 @@ MicrosoftRecordLayoutBuilder::layoutNonVirtualBases(const CXXRecordDecl *RD) {
       LeadsWithZeroSizedBase = BaseLayout.leadsWithZeroSizedBase();
     }
     // Lay out the base.
-    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout,
-                         BaseInfo);
+    layoutNonVirtualBase(RD, BaseDecl, BaseLayout, PreviousBaseLayout);
     VBPtrOffset = Bases[BaseDecl] + BaseLayout.getNonVirtualSize();
   }
   // Set our VBPtroffset if we know it at this point.
@@ -2938,7 +2921,7 @@ static bool recordUsesEBO(const RecordDecl *RD) {
 void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
     const CXXRecordDecl *RD, const CXXRecordDecl *BaseDecl,
     const ASTRecordLayout &BaseLayout,
-    const ASTRecordLayout *&PreviousBaseLayout, BaseSubobjectInfo *Base) {
+    const ASTRecordLayout *&PreviousBaseLayout) {
   // Insert padding between two bases if the left first one is zero sized or
   // contains a zero sized subobject and the right is zero sized or one leads
   // with a zero sized base.
@@ -2954,7 +2937,7 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
   if (UseExternalLayout) {
     FoundBase = External.getExternalNVBaseOffset(BaseDecl, BaseOffset);
     if (BaseOffset > Size) {
-      DataSize = Size = BaseOffset;
+      Size = BaseOffset;
     }
   }
 
@@ -2964,13 +2947,8 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
       BaseOffset = CharUnits::Zero();
     } else {
       // Otherwise, lay the base out at the end of the MDC.
-      BaseOffset = DataSize = Size = Size.alignTo(Info.Alignment);
+      BaseOffset = Size = Size.alignTo(Info.Alignment);
     }
-
-    // Place in EmptySubobjects map but don't check the position? MSVC seems to
-    // not allow fields to overlap at the end of a virtual base, but they can
-    // overlap with other bass.
-    EmptySubobjects->CanPlaceBaseAtOffset(Base, BaseOffset);
   }
 
   Bases.insert(std::make_pair(BaseDecl, BaseOffset));
@@ -2979,82 +2957,6 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
   PreviousBaseLayout = &BaseLayout;
 }
 
-BaseSubobjectInfo *MicrosoftRecordLayoutBuilder::computeBaseSubobjectInfo(
-    const CXXRecordDecl *RD, bool IsVirtual, BaseSubobjectInfo *Derived) {
-  // This is copied directly from ItaniumRecordLayoutBuilder::ComputeBaseSubobjectInfo.
-  BaseSubobjectInfo *Info;
-
-  if (IsVirtual) {
-    // Check if we already have info about this virtual base.
-    BaseSubobjectInfo *&InfoSlot = VirtualBaseInfo[RD];
-    if (InfoSlot) {
-      assert(InfoSlot->Class == RD && "Wrong class for virtual base info!");
-      return InfoSlot;
-    }
-
-    // We don't, create it.
-    InfoSlot = new (BaseSubobjectInfoAllocator.Allocate()) BaseSubobjectInfo;
-    Info = InfoSlot;
-  } else {
-    Info = new (BaseSubobjectInfoAllocator.Allocate()) BaseSubobjectInfo;
-  }
-
-  Info->Class = RD;
-  Info->IsVirtual = IsVirtual;
-  Info->Derived = nullptr;
-  Info->PrimaryVirtualBaseInfo = nullptr;
-
-  const CXXRecordDecl *PrimaryVirtualBase = nullptr;
-  BaseSubobjectInfo *PrimaryVirtualBaseInfo = nullptr;
-
-  // Check if this base has a primary virtual base.
-  if (RD->getNumVBases()) {
-    const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
-    if (Layout.isPrimaryBaseVirtual()) {
-      // This base does have a primary virtual base.
-      PrimaryVirtualBase = Layout.getPrimaryBase();
-      assert(PrimaryVirtualBase && "Didn't have a primary virtual base!");
-
-      // Now check if we have base subobject info about this primary base.
-      PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
-
-      if (PrimaryVirtualBaseInfo) {
-        if (PrimaryVirtualBaseInfo->Derived) {
-          // We did have info about this primary base, and it turns out that it
-          // has already been claimed as a primary virtual base for another
-          // base.
-          PrimaryVirtualBase = nullptr;
-        } else {
-          // We can claim this base as our primary base.
-          Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;
-          PrimaryVirtualBaseInfo->Derived = Info;
-        }
-      }
-    }
-  }
-
-  // Now go through all direct bases.
-  for (const auto &I : RD->bases()) {
-    bool IsVirtual = I.isVirtual();
-
-    const CXXRecordDecl *BaseDecl = I.getType()->getAsCXXRecordDecl();
-
-    Info->Bases.push_back(computeBaseSubobjectInfo(BaseDecl, IsVirtual, Info));
-  }
-
-  if (PrimaryVirtualBase && !PrimaryVirtualBaseInfo) {
-    // Traversing the bases must have created the base info for our primary
-    // virtual base.
-    PrimaryVirtualBaseInfo = VirtualBaseInfo.lookup(PrimaryVirtualBase);
-    assert(PrimaryVirtualBaseInfo && "Did not create a primary virtual base!");
-
-    // Claim the primary virtual base as our primary virtual base.
-    Info->PrimaryVirtualBaseInfo = PrimaryVirtualBaseInfo;
-    PrimaryVirtualBaseInfo->Derived = Info;
-  }
-  return Info;
-}
-
 void MicrosoftRecordLayoutBuilder::layoutFields(const RecordDecl *RD) {
   LastFieldIsNonZeroWidthBitfield = false;
   for (const FieldDecl *Field : RD->fields())
@@ -3075,10 +2977,6 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
   bool IsOverlappingEmptyField = FD->isPotentiallyOverlapping() &&
                                  FieldClass->isEmpty() &&
                                  FieldClass->fields().empty();
-  const CXXRecordDecl *ParentClass = cast<CXXRecordDecl>(FD->getParent());
-  bool HasBases =
-      !ParentClass->bases().empty() || !ParentClass->vbases().empty();
-
   CharUnits FieldOffset = CharUnits::Zero();
 
   if (UseExternalLayout) {
@@ -3091,6 +2989,9 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
       FieldOffset = DataSize.alignTo(Info.Alignment);
 
     while (!EmptySubobjects->CanPlaceFieldAtOffset(FD, FieldOffset)) {
+      const CXXRecordDecl *ParentClass = cast<CXXRecordDecl>(FD->getParent());
+      bool HasBases = ParentClass && (!ParentClass->bases().empty() ||
+                                      !ParentClass->vbases().empty());
       if (FieldOffset == CharUnits::Zero() && DataSize != CharUnits::Zero() &&
           HasBases) {
         // MSVC appears to only do this when there are base classes;
@@ -3139,20 +3040,21 @@ void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
     auto NewSize = Context.toCharUnitsFromBits(
         llvm::alignDown(FieldBitOffset, Context.toBits(Info.Alignment)) +
         Context.toBits(Info.Size));
-    DataSize = Size = std::max(Size, NewSize);
+    Size = std::max(Size, NewSize);
     Alignment = std::max(Alignment, Info.Alignment);
   } else if (IsUnion) {
     placeFieldAtOffset(CharUnits::Zero());
-    DataSize = Size = std::max(Size, Info.Size);
+    Size = std::max(Size, Info.Size);
     // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
     // Allocate a new block of memory and place the bitfield in it.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
     placeFieldAtOffset(FieldOffset);
-    DataSize = Size = FieldOffset + Info.Size;
+    Size = FieldOffset + Info.Size;
     Alignment = std::max(Alignment, Info.Alignment);
     RemainingBitsInField = Context.toBits(Info.Size) - Width;
   }
+  DataSize = Size;
 }
 
 void
@@ -3169,15 +3071,16 @@ MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
   ElementInfo Info = getAdjustedElementInfo(FD);
   if (IsUnion) {
     placeFieldAtOffset(CharUnits::Zero());
-    DataSize = Size = std::max(Size, Info.Size);
+    Size = std::max(Size, Info.Size);
     // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
     // Round up the current record size to the field's alignment boundary.
     CharUnits FieldOffset = Size.alignTo(Info.Alignment);
     placeFieldAtOffset(FieldOffset);
-    DataSize = Size = FieldOffset;
+    Size = FieldOffset;
     Alignment = std::max(Alignment, Info.Alignment);
   }
+  DataSize = Size;
 }
 
 void MicrosoftRecordLayoutBuilder::injectVBPtr(const CXXRecordDecl *RD) {
@@ -3195,7 +3098,7 @@ void MicrosoftRecordLayoutBuilder::injectVBPtr(const CXXRecordDecl *RD) {
     // It is possible that there were no fields or bases located after vbptr,
     // so the size was not adjusted before.
     if (Size < FieldStart)
-      DataSize = Size = FieldStart;
+      Size = FieldStart;
     return;
   }
   // Make sure that the amount we push the fields back by is a multiple of the
@@ -3277,7 +3180,7 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
     if ((PreviousBaseLayout && PreviousBaseLayout->endsWithZeroSizedObject() &&
          BaseLayout.leadsWithZeroSizedBase() && !recordUsesEBO(RD)) ||
         HasVtordisp) {
-      DataSize = Size = Size.alignTo(VtorDispAlignment) + VtorDispSize;
+      Size = Size.alignTo(VtorDispAlignment) + VtorDispSize;
       Alignment = std::max(VtorDispAlignment, Alignment);
     }
     // Insert the virtual base.
@@ -3295,7 +3198,7 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
 
     VBases.insert(std::make_pair(BaseDecl,
         ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
-    DataSize = Size = BaseOffset + BaseLayout.getNonVirtualSize();
+    Size = BaseOffset + BaseLayout.getNonVirtualSize();
     PreviousBaseLayout = &BaseLayout;
   }
 }
diff --git a/clang/test/Layout/ms-no-unique-address.cpp b/clang/test/Layout/ms-no-unique-address.cpp
index b03c63cf9bc67f0..c5dd80aa5fce4d6 100644
--- a/clang/test/Layout/ms-no-unique-address.cpp
+++ b/clang/test/Layout/ms-no-unique-address.cpp
@@ -148,27 +148,27 @@ namespace Empty {
     ~OversizedEmpty();
     [[msvc::no_unique_address]] A a;
   };
-  static_assert(sizeof(OversizedEmpty) == 2);
+  static_assert(sizeof(OversizedEmpty) == 1);
 
   // CHECK:*** Dumping AST Record Layout
   // CHECK:          0 | struct Empty::OversizedEmpty
   // CHECK-NEXT:     0 |   struct Empty::A (base) (empty)
-  // CHECK-NEXT:     1 |   struct Empty::A a (empty)
-  // CHECK-NEXT:       | [sizeof=2, align=1,
-  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+  // CHECK-NEXT:     0 |   struct Empty::A a (empty)
+  // CHECK-NEXT:       | [sizeof=1, align=1,
+  // CHECK-NEXT:       |  nvsize=1, nvalign=1]
 
   struct HasOversizedEmpty {
     [[msvc::no_unique_address]] OversizedEmpty m;
   };
-  static_assert(sizeof(HasOversizedEmpty) == 2);
+  static_assert(sizeof(HasOversizedEmpty) == 1);
 
   // CHECK:*** Dumping AST Record Layout
   // CHECK:          0 | struct Empty::HasOversizedEmpty
   // CHECK-NEXT:     0 |   struct Empty::OversizedEmpty m (empty)
   // CHECK-NEXT:     0 |     struct Empty::A (base) (empty)
-  // CHECK-NEXT:     1 |     struct Empty::A a (empty)
-  // CHECK-NEXT:       | [sizeof=2, align=1,
-  // CHECK-NEXT:       |  nvsize=2, nvalign=1]
+  // CHECK-NEXT:     0 |     struct Empty::A a (empty)
+  // CHECK-NEXT:       | [sizeof=1, align=1,
+  // CHECK-NEXT:       |  nvsize=1, nvalign=1]
 
   struct EmptyWithNonzeroDSize {
     [[msvc::no_unique_address]] A a;
@@ -279,7 +279,7 @@ namespace VBases {
   // CHECK:*** Dumping AST Record Layout
   // CHECK:          0 | struct VBases::D
   // CHECK-NEXT:     0 |   (D vbtable pointer)
-  // CHECK-NEXT:     9 |   struct VBases::Empty a
+  // CHECK-NEXT:     8 |   struct VBases::Empty a
   // CHECK-NEXT:    16 |   struct VBases::Empty (virtual base) (empty)
   // CHECK-NEXT:       | [sizeof=16, align=8,
   // CHECK-NEXT:       |  nvsize=16, nvalign=8]
@@ -305,20 +305,20 @@ namespace VBases {
     [[msvc::no_unique_address]] A a;
     [[msvc::no_unique_address]] X x;
   };
-  static_assert(sizeof(F) == 32);
+  static_assert(sizeof(F) == 24);
 
   // MSVC places x after a and the total size is 48.
   // CHECK:*** Dumping AST Record Layout
   // CHECK:          0 | struct VBases::F
   // CHECK-NEXT:     0 |   (F vbtable pointer)
-  // CHECK-NEXT:    16 |   struct VBases::A a (empty)
+  // CHECK-NEXT:     8 |   struct VBases::A a (empty)
   // CHECK-NEXT:     8 |   struct VBases::X x
   // CHECK-NEXT:     8 |     (X vbtable pointer)
-  // CHECK-NEXT:    24 |     struct VBases::A a (empty)
-  // CHECK-NEXT:    32 |     struct VBases::A (virtual base) (empty)
-  // CHECK-NEXT:    32 |   struct VBases::A (virtual base) (empty)
-  // CHECK-NEXT:       | [sizeof=32, align=8,
-  // CHECK-NEXT:       |  nvsize=32, nvalign=8]
+  // CHECK-NEXT:    16 |     struct VBases::A a (empty)
+  // CHECK-NEXT:    24 |     struct VBases::A (virtual base) (empty)
+  // CHECK-NEXT:    24 |   struct VBases::A (virtual base) (empty)
+  // CHECK-NEXT:       | [sizeof=24, align=8,
+  // CHECK-NEXT:       |  nvsize=24, nvalign=8]
 
   struct G : virtual Empty {
     int i;
diff --git a/clang/test/Preprocessor/has_attribute.cpp b/clang/test/Preprocessor/has_attribute.cpp
index bf0f9b3bc4a8f08..53b3792d5664797 100644
--- a/clang/test/Preprocessor/has_attribute.cpp
+++ b/clang/test/Preprocessor/has_attribute.cpp
@@ -55,7 +55,7 @@ CXX11(unlikely)
 // CHECK: likely: 201803L
 // CHECK: maybe_unused: 201603L
 // ITANIUM: no_unique_address: 201803L
-// WINDOWS: no_unique_address: 0
+// WINDOWS: no_unique_address: 201803L 
 // CHECK: nodiscard: 201907L
 // CHECK: noreturn: 200809L
 // CHECK: unlikely: 201803L
diff --git a/clang/test/SemaCXX/cxx2a-no-unique-address.cpp b/clang/test/SemaCXX/cxx2a-no-unique-address.cpp
index 44d6d3acdddef70..49f987dc3865fd1 100644
--- a/clang/test/SemaCXX/cxx2a-no-unique-address.cpp
+++ b/clang/test/SemaCXX/cxx2a-no-unique-address.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c++2a %s -verify -triple x86_64-linux-gnu
-// RUN: %clang_cc1 -std=c++2a %s -verify=unsupported -triple x86_64-windows
+// RUN: %clang_cc1 -std=c++2a %s -verify -triple x86_64-windows
 
 [[no_unique_address]] int a; // expected-error {{only applies to non-bit-field non-static data members}} unsupported-warning {{unknown}}
 [[no_unique_address]] void f(); // expected-error {{only applies to non-bit-field non-static data members}} unsupported-warning {{unknown}}

>From 87b21d0317e3006cfe656f6fbc6d75423f95999a Mon Sep 17 00:00:00 2001
From: Amy Huang <akhuang at google.com>
Date: Tue, 12 Sep 2023 16:28:11 -0700
Subject: [PATCH 3/3] Rename accessor, add more tests for MS zero size
 conventions

---
 clang/include/clang/Basic/Attr.td          |  2 +-
 clang/lib/AST/Decl.cpp                     |  3 +-
 clang/lib/Sema/SemaDeclAttr.cpp            |  2 +-
 clang/test/Layout/ms-no-unique-address.cpp | 43 ++++++++++++++++++++++
 4 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 23e56cda0f67e9d..b9f71307dedb047 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1801,7 +1801,7 @@ def ArmMveStrictPolymorphism : TypeAttr, TargetSpecificAttr<TargetARM> {
 def NoUniqueAddress : InheritableAttr {
   let Spellings = [CXX11<"", "no_unique_address", 201803>,
                    CXX11<"msvc", "no_unique_address", 201803>];
-  let Accessors = [Accessor<"isDefault", [CXX11<"", "no_unique_address", 201803>]>,
+  let Accessors = [Accessor<"isStandard", [CXX11<"", "no_unique_address", 201803>]>,
                    Accessor<"isMSVC", [CXX11<"msvc", "no_unique_address", 201803>]>];
   let Subjects = SubjectList<[NonBitField], ErrorDiag>;
   let Documentation = [NoUniqueAddressDocs];
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index c99d5f6c19a15d6..db6854388833145 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -4510,7 +4510,8 @@ bool FieldDecl::isZeroSize(const ASTContext &Ctx) const {
   if (!Ctx.getTargetInfo().getCXXABI().isMicrosoft())
     return true;
 
-  // MS ABI: nonzero if class type with class type fields
+  // MS ABI: has nonzero size if it is a class type with class type fields,
+  // whether or not they have nonzero size
   return !llvm::any_of(CXXRD->fields(), [](const FieldDecl *Field) {
     return Field->getType()->getAs<RecordType>();
   });
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 370dfdeb800b38f..f5aa9269b73bceb 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8371,7 +8371,7 @@ static void handleNoMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 static void handleNoUniqueAddressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   NoUniqueAddressAttr TmpAttr(S.Context, AL);
   if (S.getLangOpts().MSVCCompat) {
-    if (TmpAttr.isDefault()) {
+    if (TmpAttr.isStandard()) {
       S.Diag(AL.getLoc(), diag::warn_attribute_ignored) << AL;
       return;
     }
diff --git a/clang/test/Layout/ms-no-unique-address.cpp b/clang/test/Layout/ms-no-unique-address.cpp
index c5dd80aa5fce4d6..51cfd9a6ae3b77f 100644
--- a/clang/test/Layout/ms-no-unique-address.cpp
+++ b/clang/test/Layout/ms-no-unique-address.cpp
@@ -336,3 +336,46 @@ namespace VBases {
   // CHECK-NEXT:      | [sizeof=16, align=8,
   // CHECK-NEXT:      |  nvsize=16, nvalign=8]
 }
+
+namespace ZeroSize {
+  struct empty {};
+
+  union empty_union {};
+
+  struct empty_union_container {
+    [[msvc::no_unique_address]] empty_union x;
+  };
+
+  union union_of_empty {
+    [[msvc::no_unique_address]] empty x;
+  };
+
+  struct struct_of_empty {
+    [[msvc::no_unique_address]] empty x;
+  };
+
+  struct union_of_empty_container {
+    [[msvc::no_unique_address]] union_of_empty x;
+  };
+  static_assert(sizeof(union_of_empty_container) == 1);
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:         0 | struct ZeroSize::union_of_empty_container
+  // CHECK-NOT: (empty)
+  // CHECK:         0 |   union ZeroSize::union_of_empty x (empty)
+  // CHECK:         0 |     struct ZeroSize::empty x (empty)
+  // CHECK:           | [sizeof=1, align=1,
+  // CHECK:           |  nvsize=1, nvalign=1] 
+
+  struct struct_of_empty_container {
+    [[msvc::no_unique_address]] struct_of_empty x;
+  }; 
+  static_assert(sizeof(struct_of_empty_container) == 1);
+  // CHECK:*** Dumping AST Record Layout
+  // CHECK:         0 | struct ZeroSize::struct_of_empty_container
+  // CHECK-NOT: (empty)
+  // CHECK:         0 |   struct ZeroSize::struct_of_empty x (empty)
+  // CHECK:         0 |     struct ZeroSize::empty x (empty)
+  // CHECK:           | [sizeof=1, align=1,
+  // CHECK:           |  nvsize=1, nvalign=1] 
+
+}



More information about the cfe-commits mailing list