[clang] [CIR] Always set base subobject type for C++ records (PR #194504)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Bruno Cardoso Lopes (bcardosolopes)

<details>
<summary>Changes</summary>

For unions and final C++ classes, computeRecordLayout previously left the base-subobject type as a default-constructed (null) cir::RecordType. The non-virtual size always equals the size for those kinds of records, so no separate ".base" variant is needed, but `BaseSubobjectType` itself was never set to the complete type either.

That null leaked through getCIRGenRecordLayout(RD).getBaseSubobjectCIRType(), which is called by getStorageType(const CXXRecordDecl *) when laying out a [[no_unique_address]] / potentially-overlapping field. The null mlir::Type was then stored as MemberInfo::data and tripped a SmallVector::back() !empty() assertion in CIRRecordLowering::fillOutputFields, because a member with null data is interpreted as a bitfield placeholder and reads fieldTypes.back() that was never pushed.

Match classic CodeGen (CGRecordLayoutBuilder.cpp) and unconditionally set baseTy = *ty for any CXXRecordDecl, only constructing a separate ".base" record when nv-size != size (which excludes unions and final classes by construction).

Reproducer:
```
  union U { int a; long b; };
  struct S {
    [[no_unique_address]] U u;
    int x;
  } s;
```

This comes from a crash on the libc++ test-suite (~1,500 tests affected, all crashing inside CIRRecordLowering::lower while laying out __compressed_pair_padding-style structs in <string>).

---
Full diff: https://github.com/llvm/llvm-project/pull/194504.diff


2 Files Affected:

- (modified) clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp (+13-5) 
- (modified) clang/test/CIR/CodeGen/no-unique-address.cpp (+56) 


``````````diff
diff --git a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
index 6947fd257f47f..8399d6a333006 100644
--- a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
@@ -685,13 +685,21 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd, cir::RecordType *ty) {
   assert(ty->isIncomplete() && "recomputing record layout?");
   lowering.lower(/*nonVirtualBaseType=*/false);
 
-  // If we're in C++, compute the base subobject type.
+  // If we're in C++, compute the base subobject type. For C++ records the base
+  // subobject type is always set (matching classic CodeGen). For unions and
+  // final classes the base subobject and complete object types are identical
+  // (no tail padding can be reused), so baseTy points at the same record as
+  // ty. We must still populate baseTy in those cases because callers such as
+  // getStorageType(const CXXRecordDecl *) used to lay out potentially-
+  // overlapping ([[no_unique_address]]) fields read it unconditionally; a
+  // null baseTy would otherwise propagate as a null mlir::Type into the
+  // members vector and trip the !empty() assertion in fillOutputFields.
   cir::RecordType baseTy;
-  if (llvm::isa<CXXRecordDecl>(rd) && !rd->isUnion() &&
-      !rd->hasAttr<FinalAttr>()) {
+  if (llvm::isa<CXXRecordDecl>(rd)) {
     baseTy = *ty;
-    if (lowering.astRecordLayout.getNonVirtualSize() !=
-        lowering.astRecordLayout.getSize()) {
+    if (!rd->isUnion() && !rd->hasAttr<FinalAttr>() &&
+        lowering.astRecordLayout.getNonVirtualSize() !=
+            lowering.astRecordLayout.getSize()) {
       CIRRecordLowering baseLowering(*this, rd, /*Packed=*/lowering.packed);
       baseLowering.lower(/*NonVirtualBaseType=*/true);
       std::string baseIdentifier = getRecordTypeName(rd, ".base");
diff --git a/clang/test/CIR/CodeGen/no-unique-address.cpp b/clang/test/CIR/CodeGen/no-unique-address.cpp
index da64b5078ba53..82eef4d3a0838 100644
--- a/clang/test/CIR/CodeGen/no-unique-address.cpp
+++ b/clang/test/CIR/CodeGen/no-unique-address.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu \
 // RUN:   -fclangir -emit-cir %s -o %t.cir
 // RUN: FileCheck --check-prefix=CIR --input-file=%t.cir %s
+// RUN: FileCheck --check-prefix=CIR-NUA --input-file=%t.cir %s
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu \
 // RUN:   -fclangir -emit-llvm %s -o %t.ll
 // RUN: FileCheck --check-prefix=LLVM --input-file=%t.ll %s
@@ -38,6 +39,22 @@ struct Outer {
 // CIR:         cir.copy %{{.+}} to %[[M_COMPLETE]] skip_tail_padding : !cir.ptr<!rec_Middle>
 // CIR:         %[[EXTRA:.*]] = cir.get_member %[[THIS]][1] {name = "extra"} : !cir.ptr<!rec_Outer> -> !cir.ptr<!s8i>
 
+// Globals for the union/final NUA cases below (placed before LLVM-LABEL so
+// these DAG checks anchor to the top of the .ll file rather than to the
+// function body).
+// LLVM-DAG: %struct.OuterUnion = type { %union.UnionForNUA, i32 }
+// LLVM-DAG: %union.UnionForNUA = type { i64 }
+// LLVM-DAG: %struct.OuterFinal = type { %struct.FinalForNUA, i8 }
+// LLVM-DAG: %struct.FinalForNUA = type { i32, i8 }
+// LLVM-DAG: @ou = {{(dso_local )?}}global %struct.OuterUnion zeroinitializer, align 8
+// LLVM-DAG: @of = {{(dso_local )?}}global %struct.OuterFinal zeroinitializer, align 4
+// OGCG-DAG: %struct.OuterUnion = type { %union.UnionForNUA, i32 }
+// OGCG-DAG: %union.UnionForNUA = type { i64 }
+// OGCG-DAG: %struct.OuterFinal = type { %struct.FinalForNUA, i8 }
+// OGCG-DAG: %struct.FinalForNUA = type { i32, i8 }
+// OGCG-DAG: @ou = {{(dso_local )?}}global %struct.OuterUnion zeroinitializer, align 8
+// OGCG-DAG: @of = {{(dso_local )?}}global %struct.OuterFinal zeroinitializer, align 4
+
 // LLVM-LABEL: define {{.*}} void @_ZN5OuterC2ERK6Middlec(
 // LLVM:         %[[GEP:.*]] = getelementptr inbounds nuw %struct.Outer, ptr %{{.+}}, i32 0, i32 0
 // LLVM:         call void @llvm.memcpy.p0.p0.i64(ptr %[[GEP]], ptr %{{.+}}, i64 5, i1 false)
@@ -49,3 +66,42 @@ struct Outer {
 void test(const Middle &m) {
   Outer o(m, 'x');
 }
+
+// Regression test: a [[no_unique_address]] field whose type is a union (or a
+// final class) used to crash CIRRecordLowering with an empty SmallVector::back()
+// because computeRecordLayout left the base-subobject type unset for those
+// kinds of records, and getStorageType(const CXXRecordDecl *) propagated the
+// resulting null mlir::Type into the members vector. We now set baseTy = ty
+// for all C++ records, so these layouts succeed.
+
+union UnionForNUA {
+  int i;
+  long l;
+};
+
+struct OuterUnion {
+  [[no_unique_address]] UnionForNUA u;
+  int x;
+};
+
+OuterUnion ou;
+
+struct FinalForNUA final {
+  int a;
+  char b;
+};
+
+struct OuterFinal {
+  [[no_unique_address]] FinalForNUA f;
+  char tail;
+};
+
+OuterFinal of;
+
+// CIR-NUA-DAG: !rec_FinalForNUA = !cir.record<struct "FinalForNUA" {!s32i, !s8i}>
+// CIR-NUA-DAG: !rec_UnionForNUA = !cir.record<union "UnionForNUA" {!s32i, !s64i}>
+// CIR-NUA-DAG: !rec_OuterFinal = !cir.record<struct "OuterFinal" {!rec_FinalForNUA, !s8i}>
+// CIR-NUA-DAG: !rec_OuterUnion = !cir.record<struct "OuterUnion" {!rec_UnionForNUA, !s32i}>
+// CIR-NUA-DAG: cir.global external @ou = #cir.zero : !rec_OuterUnion
+// CIR-NUA-DAG: cir.global external @of = #cir.zero : !rec_OuterFinal
+

``````````

</details>


https://github.com/llvm/llvm-project/pull/194504