[clang][CGRecordLayout] Remove dependency on isZeroSize #96422

Michael137 · 2024-06-23T08:57:37Z

This is a follow-up from the conversation starting at #93809 (comment)

The root problem that motivated the change are external AST sources that compute ASTRecordLayouts 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).

Michael137 · 2024-06-23T08:58:26Z

In draft for now because I'm still wrapping my head around some of the IR implications of this. Some of them seem suspect.

efriedma-quic · 2024-06-24T16:52:06Z

This is roughly what I expected the patch to look like. Maybe consider adding a couple small wrapper functions around isEmptyRecord/isEmptyField to simplify the uses (and so we can use a name that indicates why the checks exist).

efriedma-quic · 2024-06-24T18:25:11Z

(Please move out of "draft" when you think this is ready.)

Michael137 · 2024-06-25T08:32:34Z

(Please move out of "draft" when you think this is ready.)

Updated the PR with some more context. There's still a few instances of FieldDecl::isZeroSize/CXXRecordDecl::isEmpty around CodeGen (see CGClass, CGExprConstant), but it wasn't obvious to me whether those needed replacing as well. Particularly ConstStructBuilder::Build has [[no_unique_address]] specific logic.

llvmbot · 2024-06-25T08:33:05Z

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Michael Buch (Michael137)

Changes

This is a follow-up from the conversation starting at #93809 (comment)

The root problem that motivated the change are external AST sources that compute ASTRecordLayouts 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), which mostly just dispatch to the isEmptyRecord/isEmptyField respectively. The main difference is that isEmptyFieldForLayout doesn't treat unnamed bitfields as unconditionally empty (unlike isEmptyField).

Patch is 46.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96422.diff

26 Files Affected:

(modified) clang/lib/CodeGen/ABIInfoImpl.cpp (+20-3)
(modified) clang/lib/CodeGen/ABIInfoImpl.h (+12-3)
(modified) clang/lib/CodeGen/CGExpr.cpp (+2-1)
(modified) clang/lib/CodeGen/CGExprConstant.cpp (+6-3)
(modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+8-7)
(modified) clang/test/CodeGen/X86/x86_64-vaarg.c (+2-1)
(modified) clang/test/CodeGen/debug-info-packed-struct.c (+1-1)
(modified) clang/test/CodeGen/paren-list-agg-init.cpp (+6-9)
(modified) clang/test/CodeGen/voidptr-vaarg.c (+2-1)
(modified) clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp (+3-3)
(modified) clang/test/CodeGenCXX/auto-var-init.cpp (+9-6)
(modified) clang/test/CodeGenCXX/bitfield-access-empty.cpp (+8-8)
(modified) clang/test/CodeGenCXX/class-layout.cpp (+1-1)
(modified) clang/test/CodeGenCXX/compound-literals.cpp (+1-1)
(modified) clang/test/CodeGenCXX/exceptions.cpp (+3-4)
(modified) clang/test/CodeGenCXX/lambda-deterministic-captures.cpp (+1-3)
(modified) clang/test/CodeGenCXX/ms_struct.cpp (+1-3)
(modified) clang/test/CodeGenCXX/partial-destruction.cpp (+4-7)
(modified) clang/test/CodeGenCXX/pr18962.cpp (+2-3)
(modified) clang/test/CodeGenCXX/references.cpp (+1-3)
(modified) clang/test/CodeGenCXX/temporaries.cpp (+6-6)
(modified) clang/test/CodeGenObjCXX/lambda-to-block.mm (+4-5)
(modified) clang/test/OpenMP/irbuilder_for_iterator.cpp (+4-6)
(modified) clang/test/OpenMP/irbuilder_for_rangefor.cpp (+6-8)
(modified) clang/test/OpenMP/task_member_call_codegen.cpp (+9-13)
(modified) clang/test/Sema/ms_class_layout.cpp (+2-12)

diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..703e69e889967 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address Addr1,
   return Address(PHI, Addr1.getElementType(), Align);
 }
 
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
                            bool AllowArrays, bool AsIfNoUniqueAddr) {
   if (FD->isUnnamedBitField())
     return true;
@@ -289,8 +289,20 @@ bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
   return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
 }
 
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-                            bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyFieldForLayout(const ASTContext &Context,
+                                    const FieldDecl *FD) {
+  if (FD->isZeroLengthBitField(Context))
+    return true;
+
+  if (FD->isUnnamedBitField())
+    return false;
+
+  return isEmptyField(Context, FD, /*AllowArrays=*/false,
+                      /*AsIfNoUniqueAddr=*/true);
+}
+
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+                            bool AllowArrays, bool AsIfNoUniqueAddr) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)
     return false;
@@ -310,6 +322,11 @@ bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
   return true;
 }
 
+bool CodeGen::isEmptyRecordForLayout(const ASTContext &Context, QualType T) {
+  return isEmptyRecord(Context, T, /*AllowArrays=*/false,
+                       /*AsIfNoUniqueAddr=*/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
index 92986fb431646..ec5a5872462c7 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,17 +126,26 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
 /// is an unnamed bit-field or an (array of) empty record(s). If
 /// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
 /// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
-                  bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+                  bool AllowArrays, bool AsIfNoUniqueAddr = false);
+
+/// isEmptyFieldForLayout - Return true iff the field is "empty", that is,
+/// either a zero-width bit-field or an empty record.
+bool isEmptyFieldForLayout(const ASTContext &Context, const FieldDecl *FD);
 
 /// isEmptyRecord - Return true iff a structure contains only empty
 /// fields. Note that a structure with a flexible array member is not
 /// considered empty. If AsIfNoUniqueAddr is true, then C++ record fields are
 /// considered empty if the [[no_unique_address]] attribute would have made
 /// them empty.
-bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
+bool isEmptyRecord(const ASTContext &Context, QualType T, bool AllowArrays,
                    bool AsIfNoUniqueAddr = false);
 
+/// isEmptyRecordForLayout - Return true iff a structure contains only empty
+/// fields. 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/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 534f46da74862..60a78e77f78c6 100644
--- 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"
@@ -4756,7 +4757,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
index dffb8ce83b643..1c6f842081ada 100644
--- 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"
@@ -2437,8 +2438,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;
 
@@ -2474,7 +2477,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/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 5169be204c14d..c0f2e4e52f117 100644
--- 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 {
@@ -634,7 +635,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");
@@ -732,7 +733,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));
@@ -880,7 +881,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.
@@ -896,7 +897,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
@@ -1162,7 +1163,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/test/CodeGen/X86/x86_64-vaarg.c b/clang/test/CodeGen/X86/x86_64-vaarg.c
index d6b885d9fb18c..7d5102f93ca6f 100644
--- 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/debug-info-packed-struct.c b/clang/test/CodeGen/debug-info-packed-struct.c
index 676cdb38b396f..33b8b4c7e13fb 100644
--- a/clang/test/CodeGen/debug-info-packed-struct.c
+++ b/clang/test/CodeGen/debug-info-packed-struct.c
@@ -2,7 +2,7 @@
 
 // CHECK: %struct.layout3 = type <{ i8, [3 x i8], %struct.size8_pack4, i8, [3 x i8] }>
 // CHECK: %struct.layout0 = type { i8, %struct.size8, i8 }
-// CHECK: %struct.layout1 = type <{ i8, %struct.size8_anon, i8, [2 x i8] }>
+// CHECK: %struct.layout1 = type { i8, [8 x i8], i8, [2 x i8] }
 // CHECK: %struct.layout2 = type <{ i8, %struct.size8_pack1, i8 }>
 
 // ---------------------------------------------------------------------
diff --git a/clang/test/CodeGen/paren-list-agg-init.cpp b/clang/test/CodeGen/paren-list-agg-init.cpp
index 88b1834d42d87..16cfe772a4ae5 100644
--- 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()
diff --git a/clang/test/CodeGen/voidptr-vaarg.c b/clang/test/CodeGen/voidptr-vaarg.c
index d023ddf0fb5d2..4f008fd85115a 100644
--- a/clang/test/CodeGen/voidptr-vaarg.c
+++ b/clang/test/CodeGen/voidptr-vaarg.c
@@ -245,7 +245,8 @@ typedef struct {
 // CHECK-NEXT:    [[ARGP_NEXT:%.*]] = getelementptr inbounds i8, ptr [[ARGP_CUR]], i32 4
 // CHECK-NEXT:    store ptr [[ARGP_NEXT]], ptr [[LIST_ADDR]], align 4
 // CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[RETVAL]], ptr align 4 [[ARGP_CUR]], i32 4, i1 false)
-// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[RETVAL]], align 4
+// CHECK-NEXT:    [[COERCE_DIVE:%.*]] = getelementptr inbounds [[STRUCT_EMPTY_INT_T]], ptr [[RETVAL]], i32 0, i32 0
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[COERCE_DIVE]], align 4
 // CHECK-NEXT:    ret i32 [[TMP0]]
 //
 empty_int_t empty_int(__builtin_va_list list) {
diff --git a/clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp b/clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp
index 14557829268ef..3efb8c449c8fa 100644
--- a/clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp
+++ b/clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp
@@ -19,8 +19,8 @@ struct S {
 };
 
 // CHECK: store i32 0, ptr @arr
-// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr @arr, i32 0, i32 1), ptr noundef @.str)
+// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (i8, ptr @arr, i64 4), ptr noundef @.str)
 // CHECK: store i32 1, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1)
-// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1), i32 0, i32 1), ptr noundef @.str.1)
+// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (i8, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1), i64 4), ptr noundef @.str.1)
 // CHECK: store i32 2, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 2)
-// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 2), i32 0, i32 1), ptr noundef @.str.2)
+// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (i8, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 2), i64 4), ptr noundef @.str.2)
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..b0fc4946040b7 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -1,9 +1,9 @@
 // RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK,CHECK-O0
-// RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK-O0,PATTERN,PATTERN-O0
+// RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK-O0,PATTERN,PATTERN-O0,PATTERN-64-O0
 // RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -O1 -emit-llvm -o - | FileCheck %s -check-prefixes=PATTERN,PATTERN-O1
 // RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK-O0,ZERO,ZERO-O0
 // RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown -fblocks -ftrivial-auto-var-init=zero %s -O1 -emit-llvm -o - | FileCheck %s -check-prefixes=ZERO,ZERO-O1
-// RUN: %clang_cc1 -std=c++14 -triple i386-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK-O0,PATTERN,PATTERN-O0
+// RUN: %clang_cc1 -std=c++14 -triple i386-unknown-unknown -fblocks -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s -check-prefixes=CHECK-O0,PATTERN,PATTERN-O0,PATTERN-32-O0
 
 #pragma clang diagnostic ignored "-Winaccessible-base"
 
@@ -174,14 +174,17 @@ struct semivolatileinit { int i = 0x11111111; volatile int vi = 0x11111111; };
 // PATTERN-O0: @__const.test_base_braces.braces = private unnamed_addr constant %struct.base { ptr inttoptr ([[IPTRT]] [[IPTR]] to ptr) }, align
 // PATTERN-O1-NOT: @__const.test_base_braces.braces
 struct base { virtual ~base(); };
-// PATTERN-O0: @__const.test_derived_uninit.uninit = private unnamed_addr constant %struct.derived { %struct.base { ptr inttoptr ([[IPTRT]] [[IPTR]] to ptr) } }, align
+// PATTERN-32-O0: @__const.test_derived_uninit.uninit = private unnamed_addr constant %struct.derived { [4 x i8] c"\FF\FF\FF\FF" }, align 4
+// PATTERN-64-O0: @__const.test_derived_uninit.uninit = private unnamed_addr constant %struct.derived { [8 x i8] c"\AA\AA\AA\AA\AA\AA\AA\AA" }, align 8
 // PATTERN-O1-NOT: @__const.test_derived_uninit.uninit
-// PATTERN-O0: @__const.test_derived_braces.braces = private unnamed_addr constant %struct.derived { %struct.base { ptr inttoptr ([[IPTRT]] [[IPTR]] to ptr) } }, align
+// PATTERN-32-O0: @__const.test_derived_braces.braces = private unnamed_addr constant %struct.derived { [4 x i8] c"\FF\FF\FF\FF" }, align 4
+// PATTERN-64-O0: @__const.test_derived_braces.braces = private unnamed_addr constant %struct.derived { [8 x i8] c"\AA\AA\AA\AA\AA\AA\AA\AA" }, align 8
 // PATTERN-O1-NOT: @__const.test_derived_braces.braces
 struct derived : public base {};
-// PATTERN-O0: @__const.test_virtualderived_uninit.uninit = private unnamed_addr constant %struct.virtualderived { %struct.base { ptr inttoptr ([[IPTRT]] [[IPTR]] to ptr) }, %struct.derived { %struct.base { ptr inttoptr ([[IPTRT]] [[IPTR]] to ptr) } } }, align
+// PATERN-O0: @__const.test_virtualderived_uninit.uninit = private unnamed_addr constant %struct.virtualderived { %struct.base { ptr inttoptr (i64 -6148914691236517206 to ptr) }, [8 x i8] c"\AA\AA\AA\AA\AA\AA\AA\AA" }, align 8
 // PATTERN-O1-NOT: @__const.test_virtualderived_uninit.uninit
-// PATTERN-O0: @__const.test_virtualderived_braces.braces = private unnam...
[truncated]

rnk · 2024-06-25T20:21:13Z

IIUC, this effectively removes all empty records from LLVM struct types. This makes the IR less "pretty", but these subobjects are notionally empty and consist of only padding bytes (i8 and i8 arrays) at the end of the day. I think that's acceptable. + @rjmccall , does that sound good to you?

rjmccall · 2024-06-25T20:28:11Z

That sounds fine to me as long as we're still emitting projections of them properly (i.e. not just assuming "oh, it's an empty record, we can use whatever pointer we want because it'll never be dereferenced").

efriedma-quic · 2024-06-25T20:49:31Z

For CGExprConstant, the code is checking "empty" in the sense of whether there's a corresponding LLVM field. So almost certainly needs changes. Not sure how that isn't causing test failures; maybe there's missing test coverage.

For CGClass, it's not directly tied to the LLVM structure layout, but I'm not sure the generated code would be semantically correct if an "empty" field that isn't isEmpty() overlaps with actual data.

Michael137 · 2024-06-29T09:20:03Z

For CGExprConstant, the code is checking "empty" in the sense of whether there's a corresponding LLVM field. So almost certainly needs changes. Not sure how that isn't causing test failures; maybe there's missing test coverage.

Yea I was pretty sure we'd have to adjust that too. Will try to get some coverage here

rnk

I think this looks good, then. Any other concerns?

Michael137 · 2024-07-05T12:52:20Z

Had to adjust isEmptyFieldForLayout/isEmptyRecordForLayout because isEmptyField/isEmptyRecord would call each other recursively, so dispatching from isEmptyFieldForLayout wouldn't have been correct (since the isEmptyField/isEmptyRecord have extra checks for unnamed bitfields and arrays which didn't make sense in the context of our emptiness check, correct me if I'm wrong here).

For CGClass, it's not directly tied to the LLVM structure layout, but I'm not sure the generated code would be semantically correct if an "empty" field that isn't isEmpty() overlaps with actual data.

I haven't addressed this yet. To clarify, are you referring to addMemcpyableField and PushCleanupForField (both of which check isZeroSize? So if we generated an overlap between an "empty" (but not isEmpty/isZeroSize) field and a non-empty field, previously addMemcpyableField wouldn't have considered the field for inclusion in the memcpy? So we should adjust the isZeroSize here too? Doing so didn't reflect in the test-suite, so it's possible there's some missing coverage here too

… layout

llvm-ci · 2024-07-16T04:14:16Z

LLVM Buildbot has detected a new failure on builder clang-armv8-quick running on linaro-clang-armv8-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/1436

Here is the relevant piece of the build log for the reference:

Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'Clang :: CodeGenCXX/zero-init-empty-virtual.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp -emit-llvm -o - | /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp --check-prefixes=CHECK
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp -emit-llvm -o -
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp --check-prefixes=CHECK
/home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp:29:11: error: CHECK: expected string not found in input
// CHECK: @{{.*}} = {{.*}}global %struct.Holder2 zeroinitializer, align 8
          ^
<stdin>:66:178: note: scanning from here
@g_holder1 = global %struct.Holder1 { %struct.polymorphic_base { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV16polymorphic_base, i32 0, i32 0, i32 2) } }, align 4
                                                                                                                                                                                 ^
<stdin>:68:8: note: possible intended match here
@g_holder2 = global %struct.Holder2 zeroinitializer, align 4
       ^

Input file: <stdin>
Check file: /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           61: $_ZTI5Empty = comdat any 
           62:  
           63: $_ZTI15derived_virtual = comdat any 
           64:  
           65: @_ZTV16polymorphic_base = linkonce_odr unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI16polymorphic_base, ptr @_ZN16polymorphic_base4funcEv, ptr @_ZN16polymorphic_baseD1Ev, ptr @_ZN16polymorphic_baseD0Ev] }, comdat, align 4 
           66: @g_holder1 = global %struct.Holder1 { %struct.polymorphic_base { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV16polymorphic_base, i32 0, i32 0, i32 2) } }, align 4 
check:29'0                                                                                                                                                                                      X~~~~~~~~~ error: no match found
           67: @__dso_handle = external hidden global i8 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           68: @g_holder2 = global %struct.Holder2 zeroinitializer, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:29'1            ?                                                      possible intended match
           69: @_ZTV7derived = linkonce_odr unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI7derived, ptr @_ZN16polymorphic_base4funcEv, ptr @_ZN7derivedD1Ev, ptr @_ZN7derivedD0Ev] }, comdat, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           70: @g_holder3 = global { { ptr } } { { ptr } { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV7derived, i32 0, i32 0, i32 2) } }, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           71: @_ZTVN10__cxxabiv117__class_type_infoE = external global [0 x ptr] 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           72: @_ZTS16polymorphic_base = linkonce_odr constant [19 x i8] c"16polymorphic_base\00", comdat, align 1 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           73: @_ZTI16polymorphic_base = linkonce_odr constant { ptr, ptr } { ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i32 2), ptr @_ZTS16polymorphic_base }, comdat, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

Michael137 · 2024-07-16T04:20:19Z

LLVM Buildbot has detected a new failure on builder clang-armv8-quick running on linaro-clang-armv8-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/1436

Here is the relevant piece of the build log for the reference:

Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'Clang :: CodeGenCXX/zero-init-empty-virtual.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp -emit-llvm -o - | /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp --check-prefixes=CHECK
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/19/include -nostdsysteminc /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp -emit-llvm -o -
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp --check-prefixes=CHECK
/home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp:29:11: error: CHECK: expected string not found in input
// CHECK: @{{.*}} = {{.*}}global %struct.Holder2 zeroinitializer, align 8
          ^
<stdin>:66:178: note: scanning from here
@g_holder1 = global %struct.Holder1 { %struct.polymorphic_base { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV16polymorphic_base, i32 0, i32 0, i32 2) } }, align 4
                                                                                                                                                                                 ^
<stdin>:68:8: note: possible intended match here
@g_holder2 = global %struct.Holder2 zeroinitializer, align 4
       ^

Input file: <stdin>
Check file: /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/CodeGenCXX/zero-init-empty-virtual.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           61: $_ZTI5Empty = comdat any 
           62:  
           63: $_ZTI15derived_virtual = comdat any 
           64:  
           65: @_ZTV16polymorphic_base = linkonce_odr unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI16polymorphic_base, ptr @_ZN16polymorphic_base4funcEv, ptr @_ZN16polymorphic_baseD1Ev, ptr @_ZN16polymorphic_baseD0Ev] }, comdat, align 4 
           66: @g_holder1 = global %struct.Holder1 { %struct.polymorphic_base { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV16polymorphic_base, i32 0, i32 0, i32 2) } }, align 4 
check:29'0                                                                                                                                                                                      X~~~~~~~~~ error: no match found
           67: @__dso_handle = external hidden global i8 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           68: @g_holder2 = global %struct.Holder2 zeroinitializer, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:29'1            ?                                                      possible intended match
           69: @_ZTV7derived = linkonce_odr unnamed_addr constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr @_ZTI7derived, ptr @_ZN16polymorphic_base4funcEv, ptr @_ZN7derivedD1Ev, ptr @_ZN7derivedD0Ev] }, comdat, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           70: @g_holder3 = global { { ptr } } { { ptr } { ptr getelementptr inbounds inrange(-8, 12) ({ [5 x ptr] }, ptr @_ZTV7derived, i32 0, i32 0, i32 2) } }, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           71: @_ZTVN10__cxxabiv117__class_type_infoE = external global [0 x ptr] 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           72: @_ZTS16polymorphic_base = linkonce_odr constant [19 x i8] c"16polymorphic_base\00", comdat, align 1 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           73: @_ZTI16polymorphic_base = linkonce_odr constant { ptr, ptr } { ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i32 2), ptr @_ZTS16polymorphic_base }, comdat, align 4 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

The FileCheck entry was too strict in this test. Fixed by removing the align 8 check

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

Summary: This is a follow-up from the conversation starting at #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/D60251747

s-barannikov · 2024-07-26T10:05:25Z

clang/lib/CodeGen/ABIInfoImpl.h

+/// 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);


These functions don't belong here. This file contains helper functions for use in implementations of ABIInfo class.
I guess CGRecordLayout* could be a better place for them.

I put those here just to have them next to isEmptyRecord/isEmptyField, which are used outside the implementation of ABIInfo. But agree with your point. These should probably all live in CGRecordLayout instead

hnrklssn · 2024-09-18T20:57:34Z

This change leads to a crash in ConstStructBuilder::Build() for the following program:

struct S {
};

union U {
    struct S s;
    int x;
};

void foo() {
    union U bar = {};
}

isEmptyRecordForLayout returns false for union U because the recursive call for U::x returns false. This means we call Init->HasSideEffects() despite Init being null (because ILE->getNumInits() is 0).

Michael137 · 2024-09-18T23:11:22Z

This change leads to a crash in ConstStructBuilder::Build() for the following program:
struct S {
};

union U {
    struct S s;
    int x;
};

void foo() {
    union U bar = {};
}
isEmptyRecordForLayout returns false for union U because the recursive call for U::x returns false. This means we call Init->HasSideEffects() despite Init being null (because ILE->getNumInits() is 0).

Thanks for reporting. It looks like this is only happening when compiling as C. A slightly more condensed reproducer:

union U {
    struct S {} s;
};

void foo() {
    // union U bar = {{}}; // WORKS
    union U bar = {}; // CRASHES
}

Looks like when the brace initializer is empty, in C, ILE->getNumInits() returns 0 (not entirely sure off the top why this is the case). Previously, isZeroSize would never have been true for fields in C, so this wasn't an issue. Now we do hit this code-path and blindly dereference a null Init. I think we can probably just add a nullptr check here:

llvm-project/clang/lib/CodeGen/CGExprConstant.cpp

Line 741 in 38752ff

if (Init->HasSideEffects(CGM.getContext()))

Though would like to understand the C vs. C++ significance here first.

llvm#96422 changed treats empty records as zero-sized for the purpose of layout. In `C`, empty fields were never considered `isZeroSize`, so we would never have tried to call `Init->hasSideEffects` on them. But since llvm#96422 we can get here when compiling `C`, but the `Init` need to exist. This patch adds a null-check to account for this situtation.

…109271) In #96422 we started treating empty records as zero-sized for the purpose of layout. In `C`, empty fields were never considered `isZeroSize`, so we would never have tried to call `Init->hasSideEffects` on them. But since #96422 we can get here when compiling `C`, but `Init` need not exist. This patch adds a null-check to account for this situtation.

…lvm#109271) In llvm#96422 we started treating empty records as zero-sized for the purpose of layout. In `C`, empty fields were never considered `isZeroSize`, so we would never have tried to call `Init->hasSideEffects` on them. But since llvm#96422 we can get here when compiling `C`, but `Init` need not exist. This patch adds a null-check to account for this situtation. (cherry picked from commit 2162a18)

…c-crash [clang][CodeGen] Check initializer of zero-size fields for nullptr In llvm#96422 we started treating empty records as zero-sized for the purpose of layout. In `C`, empty fields were never considered `isZeroSize`, so we would never have tried to call `Init->hasSideEffects` on them. But since llvm#96422 we can get here when compiling `C`, but `Init` need not exist. This patch adds a null-check to account for this situtation. (cherry picked from commit 2162a18)

…lvm#109271) In llvm#96422 we started treating empty records as zero-sized for the purpose of layout. In `C`, empty fields were never considered `isZeroSize`, so we would never have tried to call `Init->hasSideEffects` on them. But since llvm#96422 we can get here when compiling `C`, but `Init` need not exist. This patch adds a null-check to account for this situtation.

Michael137 requested review from dwblaikie and efriedma-quic June 23, 2024 08:57

Michael137 mentioned this pull request Jun 23, 2024

[clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB #93809

Closed

Michael137 requested a review from adrian-prantl June 23, 2024 12:10

Michael137 force-pushed the clang/codegen/isZeroSize-dependency branch 3 times, most recently from db3db1e to 303fc41 Compare June 24, 2024 16:02

Michael137 mentioned this pull request Jun 24, 2024

[libc++] Specialize unique_ptr for the default deleter #96504

Closed

Michael137 marked this pull request as ready for review June 25, 2024 08:32

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:codegen clang:openmp OpenMP related changes to Clang labels Jun 25, 2024

Michael137 removed backend:X86 clang:openmp OpenMP related changes to Clang labels Jun 25, 2024

Michael137 force-pushed the clang/codegen/isZeroSize-dependency branch from df613b4 to 363be51 Compare July 1, 2024 15:46

llvmbot added backend:X86 clang:openmp OpenMP related changes to Clang labels Jul 1, 2024

rnk approved these changes Jul 2, 2024

View reviewed changes

Michael137 force-pushed the clang/codegen/isZeroSize-dependency branch from 363be51 to 67caf75 Compare July 5, 2024 14:59

Michael137 added 10 commits July 15, 2024 08:23

fixup! update isEmptyRecordForLayout comment

9041aa7

fixup! fix anonymous-union-init.c test on Windows

2097deb

fixup! different layout on msvc for anonymous-union-init.c

f6693e4

fixup! don't consider polymorphic classes to be empty for purposes of…

2d22773

… layout

fixup! adjust back tests after isEmptyRecordForLayout fix

f6c64fe

fixup! isDynamicClass instead of isPolymorphic; add test

4e5002d

fixup! add missing test

c7e6468

fixup! fix filecheck pattern in new test

5312be4

fixup! fix filecheck pattern in new test

02165f5

fixup! add back a test check that was removed in earlier revisions

a4b15f0

Michael137 force-pushed the clang/codegen/isZeroSize-dependency branch from f02185b to a4b15f0 Compare July 15, 2024 19:05

Michael137 merged commit 4497ec2 into llvm:main Jul 16, 2024
7 checks passed

Michael137 deleted the clang/codegen/isZeroSize-dependency branch July 16, 2024 03:59

efriedma-quic mentioned this pull request Jul 22, 2024

[Clang] Loop over FieldDecls instead of all Decls #99574

Merged

s-barannikov reviewed Jul 26, 2024

View reviewed changes

Michael137 mentioned this pull request Sep 19, 2024

[clang][CodeGen] Check initializer of zero-size fields for nullptr #109271

Merged

Michael137 mentioned this pull request Sep 20, 2024

[clang][CodeGen] Check initializer of zero-size fields for nullptr swiftlang/llvm-project#9313

Merged

Michael137 mentioned this pull request Oct 14, 2024

[lldb] Printing some pointers causes clang assertion while debugging LLVM tool #62972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][CGRecordLayout] Remove dependency on isZeroSize #96422

[clang][CGRecordLayout] Remove dependency on isZeroSize #96422

Michael137 commented Jun 23, 2024 •

edited

Loading

Michael137 commented Jun 23, 2024

efriedma-quic commented Jun 24, 2024

efriedma-quic commented Jun 24, 2024

Michael137 commented Jun 25, 2024

llvmbot commented Jun 25, 2024 •

edited

Loading

rnk commented Jun 25, 2024

rjmccall commented Jun 25, 2024

efriedma-quic commented Jun 25, 2024

Michael137 commented Jun 29, 2024

rnk left a comment

Michael137 commented Jul 5, 2024

llvm-ci commented Jul 16, 2024

Michael137 commented Jul 16, 2024

s-barannikov Jul 26, 2024

Michael137 Aug 1, 2024

hnrklssn commented Sep 18, 2024 •

edited

Loading

Michael137 commented Sep 18, 2024 •

edited

Loading

[clang][CGRecordLayout] Remove dependency on isZeroSize #96422

[clang][CGRecordLayout] Remove dependency on isZeroSize #96422

Conversation

Michael137 commented Jun 23, 2024 • edited Loading

Michael137 commented Jun 23, 2024

efriedma-quic commented Jun 24, 2024

efriedma-quic commented Jun 24, 2024

Michael137 commented Jun 25, 2024

llvmbot commented Jun 25, 2024 • edited Loading

rnk commented Jun 25, 2024

rjmccall commented Jun 25, 2024

efriedma-quic commented Jun 25, 2024

Michael137 commented Jun 29, 2024

rnk left a comment

Choose a reason for hiding this comment

Michael137 commented Jul 5, 2024

llvm-ci commented Jul 16, 2024

Michael137 commented Jul 16, 2024

s-barannikov Jul 26, 2024

Choose a reason for hiding this comment

Michael137 Aug 1, 2024

Choose a reason for hiding this comment

hnrklssn commented Sep 18, 2024 • edited Loading

Michael137 commented Sep 18, 2024 • edited Loading

Michael137 commented Jun 23, 2024 •

edited

Loading

llvmbot commented Jun 25, 2024 •

edited

Loading

hnrklssn commented Sep 18, 2024 •

edited

Loading

Michael137 commented Sep 18, 2024 •

edited

Loading