[clang][CGRecordLayout] Remove dependency on isZeroSize (llvm#96422)

Summary: This is a follow-up from the conversation starting at llvm#93809 (comment) The root problem that motivated the change are external AST sources that compute `ASTRecordLayout`s themselves instead of letting Clang compute them from the AST. One such example is LLDB using DWARF to get the definitive offsets and sizes of C++ structures. Such layouts should be considered correct (modulo buggy DWARF), but various assertions and lowering logic around the `CGRecordLayoutBuilder` relies on the AST having `[[no_unique_address]]` attached to them. This is a layout-altering attribute which is not encoded in DWARF. This causes us LLDB to trip over the various LLVM<->Clang layout consistency checks. There has been precedent for avoiding such layout-altering attributes from affecting lowering with externally-provided layouts (e.g., packed structs). This patch proposes to replace the `isZeroSize` checks in `CGRecordLayoutBuilder` (which roughly means "empty field with [[no_unique_address]]") with checks for `CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`. **Details** The main strategy here was to change the `isZeroSize` check in `CGRecordLowering::accumulateFields` and `CGRecordLowering::accumulateBases` to use the `isEmptyXXX` APIs instead, preventing empty fields from being added to the `Members` and `Bases` structures. The rest of the changes fall out from here, to prevent lookups into these structures (for field numbers or base indices) from failing. Added `isEmptyRecordForLayout` and `isEmptyFieldForLayout` (open to better naming suggestions). The main difference to the existing `isEmptyRecord`/`isEmptyField` APIs, is that the `isEmptyXXXForLayout` counterparts don't have special treatment for `unnamed bitfields`/arrays and also treat fields of empty types as if they had `[[no_unique_address]]` (i.e., just like the `AsIfNoUniqueAddr` in `isEmptyField` does). Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D59822467
sayhaan · Jul 16, 2024 · 38a7282 · 38a7282
1 parent 8508df6
commit 38a7282
Show file tree

Hide file tree

Showing 28 changed files with 242 additions and 109 deletions.
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -310,6 +310,41 @@ bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
   return true;
 }
 
+bool CodeGen::isEmptyFieldForLayout(const ASTContext &Context,
+                                    const FieldDecl *FD) {
+  if (FD->isZeroLengthBitField(Context))
+    return true;
+
+  if (FD->isUnnamedBitField())
+    return false;
+
+  return isEmptyRecordForLayout(Context, FD->getType());
+}
+
+bool CodeGen::isEmptyRecordForLayout(const ASTContext &Context, QualType T) {
+  const RecordType *RT = T->getAs<RecordType>();
+  if (!RT)
+    return false;
+
+  const RecordDecl *RD = RT->getDecl();
+
+  // If this is a C++ record, check the bases first.
+  if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+    if (CXXRD->isDynamicClass())
+      return false;
+
+    for (const auto &I : CXXRD->bases())
+      if (!isEmptyRecordForLayout(Context, I.getType()))
+        return false;
+  }
+
+  for (const auto *I : RD->fields())
+    if (!isEmptyFieldForLayout(Context, I))
+      return false;
+
+  return true;
+}
+
 const Type *CodeGen::isSingleElementStruct(QualType T, ASTContext &Context) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)

diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -137,6 +137,16 @@ bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
 bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
                    bool AsIfNoUniqueAddr = false);
 
+/// isEmptyFieldForLayout - Return true iff the field is "empty", that is,
+/// either a zero-width bit-field or an \ref isEmptyRecordForLayout.
+bool isEmptyFieldForLayout(const ASTContext &Context, const FieldDecl *FD);
+
+/// isEmptyRecordForLayout - Return true iff a structure contains only empty
+/// base classes (per \ref isEmptyRecordForLayout) and fields (per
+/// \ref isEmptyFieldForLayout). Note, C++ record fields are considered empty
+/// if the [[no_unique_address]] attribute would have made them empty.
+bool isEmptyRecordForLayout(const ASTContext &Context, QualType T);
+
 /// isSingleElementStruct - Determine if a structure is a "single
 /// element struct", i.e. it has exactly one non-empty field or
 /// exactly one field which is itself a single element

diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
@@ -933,7 +934,7 @@ namespace {
     }
 
     void addMemcpyableField(FieldDecl *F) {
-      if (F->isZeroSize(CGF.getContext()))
+      if (isEmptyFieldForLayout(CGF.getContext(), F))
         return;
       if (!FirstField)
         addInitialField(F);
@@ -1815,7 +1816,7 @@ namespace {
                               const CXXDestructorDecl *DD)
        : Context(Context), EHStack(EHStack), DD(DD), StartIndex(std::nullopt) {}
    void PushCleanupForField(const FieldDecl *Field) {
-     if (Field->isZeroSize(Context))
+     if (isEmptyFieldForLayout(Context, Field))
        return;
      unsigned FieldIndex = Field->getFieldIndex();
      if (FieldHasTrivialDestructorBody(Context, Field)) {

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ABIInfoImpl.h"
 #include "CGCUDARuntime.h"
 #include "CGCXXABI.h"
 #include "CGCall.h"
@@ -4757,7 +4758,7 @@ static Address emitAddrOfZeroSizeField(CodeGenFunction &CGF, Address Base,
 /// The resulting address doesn't necessarily have the right type.
 static Address emitAddrOfFieldStorage(CodeGenFunction &CGF, Address base,
                                       const FieldDecl *field) {
-  if (field->isZeroSize(CGF.getContext()))
+  if (isEmptyFieldForLayout(CGF.getContext(), field))
     return emitAddrOfZeroSizeField(CGF, base, field);
 
   const RecordDecl *rec = field->getParent();

diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ABIInfoImpl.h"
 #include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
 #include "CGRecordLayout.h"
@@ -736,7 +737,7 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
 
     // Zero-sized fields are not emitted, but their initializers may still
     // prevent emission of this struct as a constant.
-    if (Field->isZeroSize(CGM.getContext())) {
+    if (isEmptyFieldForLayout(CGM.getContext(), Field)) {
       if (Init->HasSideEffects(CGM.getContext()))
         return false;
       continue;
@@ -858,7 +859,8 @@ bool ConstStructBuilder::Build(const APValue &Val, const RecordDecl *RD,
       continue;
 
     // Don't emit anonymous bitfields or zero-sized fields.
-    if (Field->isUnnamedBitField() || Field->isZeroSize(CGM.getContext()))
+    if (Field->isUnnamedBitField() ||
+        isEmptyFieldForLayout(CGM.getContext(), *Field))
       continue;
 
     // Emit the value of the initializer.
@@ -2526,8 +2528,10 @@ static llvm::Constant *EmitNullConstant(CodeGenModule &CGM,
         cast<CXXRecordDecl>(I.getType()->castAs<RecordType>()->getDecl());
 
       // Ignore empty bases.
-      if (base->isEmpty() ||
-          CGM.getContext().getASTRecordLayout(base).getNonVirtualSize()
+      if (isEmptyRecordForLayout(CGM.getContext(), I.getType()) ||
+          CGM.getContext()
+              .getASTRecordLayout(base)
+              .getNonVirtualSize()
               .isZero())
         continue;
 
@@ -2541,7 +2545,8 @@ static llvm::Constant *EmitNullConstant(CodeGenModule &CGM,
   for (const auto *Field : record->fields()) {
     // Fill in non-bitfields. (Bitfields always use a zero pattern, which we
     // will fill in later.)
-    if (!Field->isBitField() && !Field->isZeroSize(CGM.getContext())) {
+    if (!Field->isBitField() &&
+        !isEmptyFieldForLayout(CGM.getContext(), Field)) {
       unsigned fieldIndex = layout.getLLVMFieldNo(Field);
       elements[fieldIndex] = CGM.EmitNullConstant(Field->getType());
     }
@@ -2563,7 +2568,7 @@ static llvm::Constant *EmitNullConstant(CodeGenModule &CGM,
         cast<CXXRecordDecl>(I.getType()->castAs<RecordType>()->getDecl());
 
       // Ignore empty bases.
-      if (base->isEmpty())
+      if (isEmptyRecordForLayout(CGM.getContext(), I.getType()))
         continue;
 
       unsigned fieldIndex = layout.getVirtualBaseIndex(base);

diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CGOpenMPRuntime.h"
+#include "ABIInfoImpl.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
 #include "CGRecordLayout.h"
@@ -7729,23 +7730,28 @@ class MappableExprsHandler {
     for (const auto &I : RD->bases()) {
       if (I.isVirtual())
         continue;
-      const auto *Base = I.getType()->getAsCXXRecordDecl();
+
+      QualType BaseTy = I.getType();
+      const auto *Base = BaseTy->getAsCXXRecordDecl();
       // Ignore empty bases.
-      if (Base->isEmpty() || CGF.getContext()
-                                 .getASTRecordLayout(Base)
-                                 .getNonVirtualSize()
-                                 .isZero())
+      if (isEmptyRecordForLayout(CGF.getContext(), BaseTy) ||
+          CGF.getContext()
+              .getASTRecordLayout(Base)
+              .getNonVirtualSize()
+              .isZero())
         continue;
 
       unsigned FieldIndex = RL.getNonVirtualBaseLLVMFieldNo(Base);
       RecordLayout[FieldIndex] = Base;
     }
     // Fill in virtual bases.
     for (const auto &I : RD->vbases()) {
-      const auto *Base = I.getType()->getAsCXXRecordDecl();
+      QualType BaseTy = I.getType();
       // Ignore empty bases.
-      if (Base->isEmpty())
+      if (isEmptyRecordForLayout(CGF.getContext(), BaseTy))
         continue;
+
+      const auto *Base = BaseTy->getAsCXXRecordDecl();
       unsigned FieldIndex = RL.getVirtualBaseIndex(Base);
       if (RecordLayout[FieldIndex])
         continue;
@@ -7756,7 +7762,8 @@ class MappableExprsHandler {
     for (const auto *Field : RD->fields()) {
       // Fill in non-bitfields. (Bitfields always use a zero pattern, which we
       // will fill in later.)
-      if (!Field->isBitField() && !Field->isZeroSize(CGF.getContext())) {
+      if (!Field->isBitField() &&
+          !isEmptyFieldForLayout(CGF.getContext(), Field)) {
         unsigned FieldIndex = RL.getLLVMFieldNo(Field);
         RecordLayout[FieldIndex] = Field;
       }

diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -10,8 +10,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "CGRecordLayout.h"
+#include "ABIInfoImpl.h"
 #include "CGCXXABI.h"
+#include "CGRecordLayout.h"
 #include "CodeGenTypes.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
@@ -384,7 +385,7 @@ void CGRecordLowering::accumulateFields(bool isNonVirtualBaseType) {
       Field = accumulateBitFields(isNonVirtualBaseType, Field, FieldEnd);
       assert((Field == FieldEnd || !Field->isBitField()) &&
              "Failed to accumulate all the bitfields");
-    } else if (Field->isZeroSize(Context)) {
+    } else if (isEmptyFieldForLayout(Context, *Field)) {
       // Empty fields have no storage.
       ++Field;
     } else {
@@ -633,7 +634,7 @@ CGRecordLowering::accumulateBitFields(bool isNonVirtualBaseType,
           // non-reusable tail padding.
           CharUnits LimitOffset;
           for (auto Probe = Field; Probe != FieldEnd; ++Probe)
-            if (!Probe->isZeroSize(Context)) {
+            if (!isEmptyFieldForLayout(Context, *Probe)) {
               // A member with storage sets the limit.
               assert((getFieldBitOffset(*Probe) % CharBits) == 0 &&
                      "Next storage is not byte-aligned");
@@ -731,7 +732,7 @@ void CGRecordLowering::accumulateBases() {
     // Bases can be zero-sized even if not technically empty if they
     // contain only a trailing array member.
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
-    if (!BaseDecl->isEmpty() &&
+    if (!isEmptyRecordForLayout(Context, Base.getType()) &&
         !Context.getASTRecordLayout(BaseDecl).getNonVirtualSize().isZero())
       Members.push_back(MemberInfo(Layout.getBaseClassOffset(BaseDecl),
           MemberInfo::Base, getStorageType(BaseDecl), BaseDecl));
@@ -879,7 +880,7 @@ CGRecordLowering::calculateTailClippingOffset(bool isNonVirtualBaseType) const {
   if (!isNonVirtualBaseType && isOverlappingVBaseABI())
     for (const auto &Base : RD->vbases()) {
       const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
-      if (BaseDecl->isEmpty())
+      if (isEmptyRecordForLayout(Context, Base.getType()))
         continue;
       // If the vbase is a primary virtual base of some base, then it doesn't
       // get its own storage location but instead lives inside of that base.
@@ -895,7 +896,7 @@ CGRecordLowering::calculateTailClippingOffset(bool isNonVirtualBaseType) const {
 void CGRecordLowering::accumulateVBases() {
   for (const auto &Base : RD->vbases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
-    if (BaseDecl->isEmpty())
+    if (isEmptyRecordForLayout(Context, Base.getType()))
       continue;
     CharUnits Offset = Layout.getVBaseClassOffset(BaseDecl);
     // If the vbase is a primary virtual base of some base, then it doesn't
@@ -1161,7 +1162,7 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, llvm::StructType *Ty) {
     const FieldDecl *FD = *it;
 
     // Ignore zero-sized fields.
-    if (FD->isZeroSize(getContext()))
+    if (isEmptyFieldForLayout(getContext(), FD))
       continue;
 
     // For non-bit-fields, just check that the LLVM struct offset matches the

diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -15,6 +15,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CodeGenTBAA.h"
+#include "ABIInfoImpl.h"
 #include "CGRecordLayout.h"
 #include "CodeGenTypes.h"
 #include "clang/AST/ASTContext.h"
@@ -309,7 +310,7 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
     unsigned idx = 0;
     for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
          i != e; ++i, ++idx) {
-      if ((*i)->isZeroSize(Context))
+      if (isEmptyFieldForLayout(Context, *i))
         continue;
 
       uint64_t Offset =

diff --git a/clang/test/CodeGen/2009-06-14-anonymous-union-init.c b/clang/test/CodeGen/2009-06-14-anonymous-union-init.c
@@ -1,7 +1,19 @@
-// RUN: %clang_cc1 -emit-llvm < %s | grep "zeroinitializer, i16 16877"
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefixes=CHECK,EMPTY
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-windows-msvc -o - | FileCheck %s --check-prefixes=CHECK,EMPTY-MSVC
 // PR4390
 struct sysfs_dirent {
- union { struct sysfs_elem_dir {} s_dir; };
+ union { struct sysfs_elem_dir { int x; } s_dir; };
  unsigned short s_mode;
 };
 struct sysfs_dirent sysfs_root = { {}, 16877 };
+
+// CHECK: @sysfs_root = {{.*}}global %struct.sysfs_dirent { %union.anon zeroinitializer, i16 16877 }
+
+struct Foo {
+ union { struct empty {} x; };
+ unsigned short s_mode;
+};
+struct Foo foo = { {}, 16877 };
+
+// EMPTY:      @foo = {{.*}}global %struct.Foo { i16 16877 }
+// EMPTY-MSVC: @foo = {{.*}}global %struct.Foo { [4 x i8] undef, i16 16877 }
diff --git a/clang/test/CodeGen/X86/x86_64-vaarg.c b/clang/test/CodeGen/X86/x86_64-vaarg.c
@@ -56,7 +56,8 @@ typedef struct {
 // CHECK:       vaarg.end:
 // CHECK-NEXT:    [[VAARG_ADDR:%.*]] = phi ptr [ [[TMP1]], [[VAARG_IN_REG]] ], [ [[OVERFLOW_ARG_AREA]], [[VAARG_IN_MEM]] ]
 // CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[RETVAL]], ptr align 8 [[VAARG_ADDR]], i64 8, i1 false)
-// CHECK-NEXT:    [[TMP3:%.*]] = load double, ptr [[RETVAL]], align 8
+// CHECK-NEXT:    [[COERCE:%.*]] = getelementptr inbounds [[STRUCT_S1]], ptr [[RETVAL]], i32 0, i32 0
+// CHECK-NEXT:    [[TMP3:%.*]] = load double, ptr [[COERCE]], align 8
 // CHECK-NEXT:    ret double [[TMP3]]
 //
 s1 f(int z, ...) {

diff --git a/clang/test/CodeGen/paren-list-agg-init.cpp b/clang/test/CodeGen/paren-list-agg-init.cpp
@@ -48,14 +48,13 @@ struct E {
   ~E() {};
 };
 
-// CHECK-DAG: [[STRUCT_F:%.*]] = type { i8 }
 struct F {
   F (int i = 1);
   F (const F &f) = delete;
   F (F &&f) = default;
 };
 
-// CHECK-DAG: [[STRUCT_G:%.*]] = type <{ i32, [[STRUCT_F]], [3 x i8] }>
+// CHECK-DAG: [[STRUCT_G:%.*]] = type <{ i32, [4 x i8] }>
 struct G {
   int a;
   F f;
@@ -78,12 +77,12 @@ namespace gh61145 {
     ~Vec();
   };
 
-  // CHECK-DAG: [[STRUCT_S1:%.*]] = type { [[STRUCT_VEC]] }
+  // CHECK-DAG: [[STRUCT_S1:%.*]] = type { i8 }
   struct S1 {
     Vec v;
   };
 
-  // CHECK-DAG: [[STRUCT_S2:%.*]] = type { [[STRUCT_VEC]], i8 }
+  // CHECK-DAG: [[STRUCT_S2:%.*]] = type { i8, i8 }
   struct S2 {
     Vec v;
     char c;
@@ -377,7 +376,7 @@ void foo18() {
 // CHECK-NEXT: [[G:%.*g.*]] = alloca [[STRUCT_G]], align 4
 // CHECK-NEXT: [[A:%.*a.*]] = getelementptr inbounds [[STRUCT_G]], ptr [[G]], i32 0, i32 0
 // CHECK-NEXT: store i32 2, ptr [[A]], align 4
-// CHECK-NEXT: [[F:%.*f.*]] = getelementptr inbounds [[STRUCT_G]], ptr [[G]], i32 0, i32 1
+// CHECK-NEXT: [[F:%.*]] = getelementptr inbounds i8, ptr [[G]], i64 4
 // CHECk-NEXT: call void @{{.*F.*}}(ptr noundef nonnull align 1 dereferenceable(1)) [[F]], ie32 noundef 1)
 // CHECK: ret void
 void foo19() {
@@ -392,9 +391,8 @@ namespace gh61145 {
   // CHECK-NEXT: [[AGG_TMP_ENSURED:%.*agg.tmp.ensured.*]] = alloca [[STRUCT_S1]], align 1
   // a.k.a. Vec::Vec()
   // CHECK-NEXT: call void @_ZN7gh611453VecC1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
-  // CHECK-NEXT: [[V1:%.*v1.*]] = getelementptr inbounds [[STRUCT_S1]], ptr [[AGG_TMP_ENSURED]], i32 0, i32 0
   // a.k.a. Vec::Vec(Vec&&)
-  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[V1]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[AGG_TMP_ENSURED]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
   // a.k.a. S1::~S1()
   // CHECK-NEXT: call void @_ZN7gh611452S1D1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[AGG_TMP_ENSURED]])
   // a.k.a.Vec::~Vec()
@@ -413,9 +411,8 @@ namespace gh61145 {
   // CHECK-NEXT: [[AGG_TMP_ENSURED:%.*agg.tmp.ensured.*]] = alloca [[STRUCT_S2]], align 1
   // a.k.a. Vec::Vec()
   // CHECK-NEXT: call void @_ZN7gh611453VecC1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
-  // CHECK-NEXT: [[V1:%.*v1.*]] = getelementptr inbounds [[STRUCT_S2]], ptr [[AGG_TMP_ENSURED]], i32 0, i32 0
   // a.k.a. Vec::Vec(Vec&&)
-  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[V1]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[AGG_TMP_ENSURED]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
   // CHECK-NEXT: [[C:%.*c.*]] = getelementptr inbounds [[STRUCT_S2]], ptr [[AGG_TMP_ENSURED]], i32 0, i32
   // CHECK-NEXT: store i8 0, ptr [[C]], align 1
   // a.k.a. S2::~S2()