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

Michael137 · 2024-05-30T11:51:19Z

This is the outcome of the discussions we had in
https://discourse.llvm.org/t/rfc-lldb-handling-no-unique-address-in-lldb/77483

To summarize, LLDB creates AST nodes by parsing debug-info and hands those off to Clang for codegen. There are certain Clang attributes that affect structure layout which are not encoded in DWARF, one of which is [[no_unique_address]]. But the effects of such attributes are encoded in DWARF in terms of member offsets and sizes. So some of the correctness checks that the RecordLayoutBuilder performs aren't really necessary for layouts provided by LLDB since we know that the offsets we get from DWARF are correct (modulo corrupt DWARF). Hence this patch guards those asserts behind the DebuggerSupport flag.

This unblocks the libc++ compressed_pair refactor in #93069

…rovided by LLDB This is the outcome of the discussions we had in https://discourse.llvm.org/t/rfc-lldb-handling-no-unique-address-in-lldb/77483 To summarize, LLDB creates AST nodes by parsing debug-info and hands those off to Clang for codegen. There are certain Clang attributes that affect structure layout which are not encoded in DWARF, one of which is `[[no_unique_address]]`. But the affects of such attributes *is* encoded in DWARF in terms of member offsets and sizes. So some of the correctness checks that the `RecordLayoutBuilder` performs aren't really necessary for layouts provided by LLDB since we know that the offsets we get from DWARF are correct (modulo corrupt DWARF). Hence this patch guards those asserts behind the `DebuggerSupport` flag.

llvmbot · 2024-05-30T11:51:48Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Michael Buch (Michael137)

Changes

This is the outcome of the discussions we had in
https://discourse.llvm.org/t/rfc-lldb-handling-no-unique-address-in-lldb/77483

To summarize, LLDB creates AST nodes by parsing debug-info and hands those off to Clang for codegen. There are certain Clang attributes that affect structure layout which are not encoded in DWARF, one of which is [[no_unique_address]]. But the affects of such attributes is encoded in DWARF in terms of member offsets and sizes. So some of the correctness checks that the RecordLayoutBuilder performs aren't really necessary for layouts provided by LLDB since we know that the offsets we get from DWARF are correct (modulo corrupt DWARF). Hence this patch guards those asserts behind the DebuggerSupport flag.

This unblocks the libc++ compressed_pair refactor in #93069

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

4 Files Affected:

(modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+12-6)
(added) lldb/test/API/lang/cpp/no_unique_address/Makefile (+3)
(added) lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py (+50)
(added) lldb/test/API/lang/cpp/no_unique_address/main.cpp (+35)

diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 5169be204c14d..32fb032214e28 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -950,6 +950,9 @@ void CGRecordLowering::calculateZeroInit() {
 // Verify accumulateBitfields computed the correct storage representations.
 void CGRecordLowering::checkBitfieldClipping(bool IsNonVirtualBaseType) const {
 #ifndef NDEBUG
+  if (Context.getLangOpts().DebuggerSupport)
+    return;
+
   auto ScissorOffset = calculateTailClippingOffset(IsNonVirtualBaseType);
   auto Tail = CharUnits::Zero();
   for (const auto &M : Members) {
@@ -1008,7 +1011,8 @@ void CGRecordLowering::insertPadding() {
     if (!Member->Data)
       continue;
     CharUnits Offset = Member->Offset;
-    assert(Offset >= Size);
+    if (!Context.getLangOpts().DebuggerSupport)
+      assert(Offset >= Size);
     // Insert padding if we need to.
     if (Offset !=
         Size.alignTo(Packed ? CharUnits::One() : getAlignment(Member->Data)))
@@ -1138,8 +1142,9 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, llvm::StructType *Ty) {
   const ASTRecordLayout &Layout = getContext().getASTRecordLayout(D);
 
   uint64_t TypeSizeInBits = getContext().toBits(Layout.getSize());
-  assert(TypeSizeInBits == getDataLayout().getTypeAllocSizeInBits(Ty) &&
-         "Type size mismatch!");
+  if (!Context.getLangOpts().DebuggerSupport)
+    assert(TypeSizeInBits == getDataLayout().getTypeAllocSizeInBits(Ty) &&
+           "Type size mismatch!");
 
   if (BaseTy) {
     CharUnits NonVirtualSize  = Layout.getNonVirtualSize();
@@ -1147,9 +1152,10 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, llvm::StructType *Ty) {
     uint64_t AlignedNonVirtualTypeSizeInBits =
       getContext().toBits(NonVirtualSize);
 
-    assert(AlignedNonVirtualTypeSizeInBits ==
-           getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
-           "Type size mismatch!");
+    if (!Context.getLangOpts().DebuggerSupport)
+      assert(AlignedNonVirtualTypeSizeInBits ==
+             getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
+             "Type size mismatch!");
   }
 
   // Verify that the LLVM and AST field offsets agree.
diff --git a/lldb/test/API/lang/cpp/no_unique_address/Makefile b/lldb/test/API/lang/cpp/no_unique_address/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py b/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
new file mode 100644
index 0000000000000..373bcb9062cde
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
@@ -0,0 +1,50 @@
+"""
+Test that LLDB correctly handles fields
+marked with [[no_unique_address]].
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class NoUniqueAddressTestCase(TestBase):
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "return 0", lldb.SBFileSpec("main.cpp", False)
+        )
+
+        # Qualified/unqualified lookup to templates in namespace
+        self.expect_expr("b1", result_type="basic::Foo", result_children=[ValueCheck(
+            name="a", type="Empty"
+        )])
+
+        self.expect_expr("b2", result_type="bases::Foo", result_children=[
+            ValueCheck(type="bases::B", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::A", children=[
+                ValueCheck(name="c", type="long", value="1"),
+                ValueCheck(name="d", type="long", value="2")
+            ]),
+            ValueCheck(type="bases::C", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+        ])
+        self.expect_expr("b3", result_type="bases::Bar", result_children=[
+            ValueCheck(type="bases::B", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::C", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::A", children=[
+                ValueCheck(name="c", type="long", value="5"),
+                ValueCheck(name="d", type="long", value="6")
+            ]),
+        ])
+
+        self.expect("frame var b1")
+        self.expect("frame var b2")
+        self.expect("frame var b3")
diff --git a/lldb/test/API/lang/cpp/no_unique_address/main.cpp b/lldb/test/API/lang/cpp/no_unique_address/main.cpp
new file mode 100644
index 0000000000000..424fa90859cea
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/main.cpp
@@ -0,0 +1,35 @@
+struct Empty {};
+
+namespace basic {
+struct Foo {
+  [[no_unique_address]] Empty a;
+};
+} // namespace basic
+
+namespace bases {
+struct A {
+  long c, d;
+};
+
+struct B {
+  [[no_unique_address]] Empty x;
+};
+
+struct C {
+  [[no_unique_address]] Empty x;
+};
+
+struct Foo : B, A, C {};
+struct Bar : B, C, A {};
+} // namespace bases
+
+int main() {
+  basic::Foo b1;
+  bases::Foo b2;
+  bases::Bar b3;
+  b2.c = 1;
+  b2.d = 2;
+  b3.c = 5;
+  b3.d = 6;
+  return 0;
+}

llvmbot · 2024-05-30T11:51:48Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This is the outcome of the discussions we had in
https://discourse.llvm.org/t/rfc-lldb-handling-no-unique-address-in-lldb/77483

To summarize, LLDB creates AST nodes by parsing debug-info and hands those off to Clang for codegen. There are certain Clang attributes that affect structure layout which are not encoded in DWARF, one of which is [[no_unique_address]]. But the affects of such attributes is encoded in DWARF in terms of member offsets and sizes. So some of the correctness checks that the RecordLayoutBuilder performs aren't really necessary for layouts provided by LLDB since we know that the offsets we get from DWARF are correct (modulo corrupt DWARF). Hence this patch guards those asserts behind the DebuggerSupport flag.

This unblocks the libc++ compressed_pair refactor in #93069

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

4 Files Affected:

(modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+12-6)
(added) lldb/test/API/lang/cpp/no_unique_address/Makefile (+3)
(added) lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py (+50)
(added) lldb/test/API/lang/cpp/no_unique_address/main.cpp (+35)

diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 5169be204c14d..32fb032214e28 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -950,6 +950,9 @@ void CGRecordLowering::calculateZeroInit() {
 // Verify accumulateBitfields computed the correct storage representations.
 void CGRecordLowering::checkBitfieldClipping(bool IsNonVirtualBaseType) const {
 #ifndef NDEBUG
+  if (Context.getLangOpts().DebuggerSupport)
+    return;
+
   auto ScissorOffset = calculateTailClippingOffset(IsNonVirtualBaseType);
   auto Tail = CharUnits::Zero();
   for (const auto &M : Members) {
@@ -1008,7 +1011,8 @@ void CGRecordLowering::insertPadding() {
     if (!Member->Data)
       continue;
     CharUnits Offset = Member->Offset;
-    assert(Offset >= Size);
+    if (!Context.getLangOpts().DebuggerSupport)
+      assert(Offset >= Size);
     // Insert padding if we need to.
     if (Offset !=
         Size.alignTo(Packed ? CharUnits::One() : getAlignment(Member->Data)))
@@ -1138,8 +1142,9 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, llvm::StructType *Ty) {
   const ASTRecordLayout &Layout = getContext().getASTRecordLayout(D);
 
   uint64_t TypeSizeInBits = getContext().toBits(Layout.getSize());
-  assert(TypeSizeInBits == getDataLayout().getTypeAllocSizeInBits(Ty) &&
-         "Type size mismatch!");
+  if (!Context.getLangOpts().DebuggerSupport)
+    assert(TypeSizeInBits == getDataLayout().getTypeAllocSizeInBits(Ty) &&
+           "Type size mismatch!");
 
   if (BaseTy) {
     CharUnits NonVirtualSize  = Layout.getNonVirtualSize();
@@ -1147,9 +1152,10 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, llvm::StructType *Ty) {
     uint64_t AlignedNonVirtualTypeSizeInBits =
       getContext().toBits(NonVirtualSize);
 
-    assert(AlignedNonVirtualTypeSizeInBits ==
-           getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
-           "Type size mismatch!");
+    if (!Context.getLangOpts().DebuggerSupport)
+      assert(AlignedNonVirtualTypeSizeInBits ==
+             getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
+             "Type size mismatch!");
   }
 
   // Verify that the LLVM and AST field offsets agree.
diff --git a/lldb/test/API/lang/cpp/no_unique_address/Makefile b/lldb/test/API/lang/cpp/no_unique_address/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py b/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
new file mode 100644
index 0000000000000..373bcb9062cde
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
@@ -0,0 +1,50 @@
+"""
+Test that LLDB correctly handles fields
+marked with [[no_unique_address]].
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class NoUniqueAddressTestCase(TestBase):
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "return 0", lldb.SBFileSpec("main.cpp", False)
+        )
+
+        # Qualified/unqualified lookup to templates in namespace
+        self.expect_expr("b1", result_type="basic::Foo", result_children=[ValueCheck(
+            name="a", type="Empty"
+        )])
+
+        self.expect_expr("b2", result_type="bases::Foo", result_children=[
+            ValueCheck(type="bases::B", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::A", children=[
+                ValueCheck(name="c", type="long", value="1"),
+                ValueCheck(name="d", type="long", value="2")
+            ]),
+            ValueCheck(type="bases::C", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+        ])
+        self.expect_expr("b3", result_type="bases::Bar", result_children=[
+            ValueCheck(type="bases::B", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::C", children=[
+                ValueCheck(name="x", type="Empty")
+            ]),
+            ValueCheck(type="bases::A", children=[
+                ValueCheck(name="c", type="long", value="5"),
+                ValueCheck(name="d", type="long", value="6")
+            ]),
+        ])
+
+        self.expect("frame var b1")
+        self.expect("frame var b2")
+        self.expect("frame var b3")
diff --git a/lldb/test/API/lang/cpp/no_unique_address/main.cpp b/lldb/test/API/lang/cpp/no_unique_address/main.cpp
new file mode 100644
index 0000000000000..424fa90859cea
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/main.cpp
@@ -0,0 +1,35 @@
+struct Empty {};
+
+namespace basic {
+struct Foo {
+  [[no_unique_address]] Empty a;
+};
+} // namespace basic
+
+namespace bases {
+struct A {
+  long c, d;
+};
+
+struct B {
+  [[no_unique_address]] Empty x;
+};
+
+struct C {
+  [[no_unique_address]] Empty x;
+};
+
+struct Foo : B, A, C {};
+struct Bar : B, C, A {};
+} // namespace bases
+
+int main() {
+  basic::Foo b1;
+  bases::Foo b2;
+  bases::Bar b3;
+  b2.c = 1;
+  b2.d = 2;
+  b3.c = 5;
+  b3.d = 6;
+  return 0;
+}

github-actions · 2024-05-30T11:54:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-05-30T11:54:11Z

✅ With the latest revision this PR passed the Python code formatter.

dwblaikie · 2024-05-30T16:34:37Z

Could probably adjust the assertions to be assert (debug || whatever) rather than if (!debug) assert(whatever).

My understanding/expectation was that these assertions would be removed entirely - that whatever generated the AST should just be trusted, whether it's the debugger or the compiler frontend... but I'll certainly leave it up to @AaronBallman as to where he thinks the right tradeoff/sweetspot is.

efriedma-quic · 2024-05-30T16:58:07Z

I'm skeptical it's correct to skip all the assertions like this; the assertions are there to ensure the layout of the LLVM IR type matches the layout provided by the RecordLayout. If the LLVM IR layout is wrong, address-related computations will be wrong, and ultimately you'll miscompile. We don't really care whether the RecordLayout is consistent with language rules.

The correct answer here is probably to fix the sizes in the RecordLayout itself; in particular, the DataSize of the members.

rjmccall · 2024-05-30T17:24:49Z

I agree with Eli. We should trust external record layout to the extent that it generates a valid layout, but if it generates something with overlapping fields, or that runs outside the claimed bounds of the type, we'll just end up crashing in IRGen. I assume those things are expressible in DWARF; it'd take a bunch of sub-optimal representations to make them impossible. It's probably clang's responsibility to detect that — maybe we ought to do a special sanity check on external layouts before we feed them into RecordLayout.

Michael137 · 2024-05-31T22:25:58Z

The correct answer here is probably to fix the sizes in the RecordLayout itself; in particular, the DataSize of the members.

That would be ideal, but also means we'd have to reflect the various C++ attributes that affect layout in DWARF. Avoiding adding such language-specific constructs to DWARF is what partly motivated this patch.

We should trust external record layout to the extent that it generates a valid layout, but if it generates something with overlapping fields, or that runs outside the claimed bounds of the type, we'll just end up crashing in IRGen. I assume those things are expressible in DWARF;

Yea the idea was that we catch these malformed DWARF representations elsewhere. Not necessarily relying on the record layout layer to tell us about this. Not sure how equipped LLDB currently is in dealing with corrupted DWARF.

efriedma-quic · 2024-05-31T22:43:45Z

The correct answer here is probably to fix the sizes in the RecordLayout itself; in particular, the DataSize of the members.

That would be ideal, but also means we'd have to reflect the various C++ attributes that affect layout in DWARF. Avoiding adding such language-specific constructs to DWARF is what partly motivated this patch.

Given the offsets and sizes of the members of a struct, you can compute the datasize as the offset plus the size of the last member. That isn't really correct for POD structs, but the CGRecordLayout won't care: it can't tell the difference between padding that's illegal to reuse, vs. padding that the frontend chose not to reuse for some other reason.

Michael137 · 2024-06-17T11:52:50Z

The correct answer here is probably to fix the sizes in the RecordLayout itself; in particular, the DataSize of the members.

That would be ideal, but also means we'd have to reflect the various C++ attributes that affect layout in DWARF. Avoiding adding such language-specific constructs to DWARF is what partly motivated this patch.

Given the offsets and sizes of the members of a struct, you can compute the datasize as the offset plus the size of the last member. That isn't really correct for POD structs, but the CGRecordLayout won't care: it can't tell the difference between padding that's illegal to reuse, vs. padding that the frontend chose not to reuse for some other reason.

Just got back to looking at this again. Haven't fully figured out yet why the AST and LLVM layouts in the LLDB case don't seem to be consistent. Though the asserts that iterate over Members and FieldTypes fail because those structures are filled out conditionally on isZeroSize() (which in the LLDB case won't work because we don't have that information in the AST, i.e., the [[no_unique_address]] attribute).

Interestingly, I only just found out about the -foverride-record-layout= flag, which seems like a neat way of testing externally provided layouts. I compiled the test attached to this patch but using the layout that LLDB computes. Then compared the dump-record-layouts output to the one that Clang generates without externally provided layouts.

$ ./bin/clang -cc1 nua-bases.cpp -w -std=c++14 -fdump-record-layouts-simple -DNUA= -foverride-record-layout=nua.lldb > nua.lldb.after                

$ diff nua.lldb.after nua.before 
52c52
<   Alignment:8
---
>   Alignment:64

Interestingly the only difference here is the alignment computed for Foo.

Michael137 · 2024-06-17T15:34:08Z

Yea the problem with checking the size reported by the AST layout vs. the LLVM type layout is that in DWARF we get following representation:

// main.cpp
struct Empty {};

struct A {
  long c, d;
};

struct B {
  [[no_unique_address]] Empty x;
};

struct C {
  [[no_unique_address]] Empty x;
};

struct Foo : B, C, A {};

// dwarfdump
DW_TAG_structure_type
  DW_AT_name ("Foo")
  DW_AT_byte_size (0x10)
  DW_TAG_inheritance
    DW_AT_type (0x0000000000000085 "B")
    DW_AT_data_member_location (0x00)
  DW_TAG_inheritance
    DW_AT_type (0x00000000000000b9 "C")
    DW_AT_data_member_location (0x01)
  DW_TAG_inheritance
    DW_AT_type (0x0000000000000047 "A")
    DW_AT_data_member_location (0x00)

Which is a perfectly valid layout, and shows how B and C don't contribute any size to Foo.

However, LLDB's AST gets lowered to:

Layout: <CGRecordLayout
  LLVMType:%struct.Foo = type { %struct.B, %struct.C, %struct.E, [18446744073709551615 x i8], [14 x i8], i64 }
  NonVirtualBaseLLVMType:%struct.Foo = type { %struct.B, %struct.C, %struct.E, [18446744073709551615 x i8], [14 x i8], i64 }
  IsZeroInitializable:1
  BitFields:[
]>

(side-note, the 18446744073709551615 here happens because using the offsets from DWARF results in negative padding during lowering, but getTypeAllocSizeInBits ends up wrapping the value around in the calculations without issues it seems).

This won't pass the AST layout size == LLVM type size check. So it doesn't seem like anything unexpected is going on here, the lack of no_unique_address causes the lowering to get confused. Though I don't think we would want to fixup the size passed to RecordLayout, because that's the one that we get from Clang via DWARF, and should be the correct one to use. Don't have a good idea on how we would get around the lack of no_unique_address here (without relaxing the assertion for external sources, or encoding it in DWARF in some way).

efriedma-quic · 2024-06-17T22:35:34Z

Oh, in this particular case, the issue isn't the computed datasize, it's that FieldDecl::isZeroSize() returns the wrong result. If that's the case, maybe we can just change FieldDecl::isZeroSize() to say the field is zero size? So essentially, we pretend all empty fields are no_unique_address. Nothing in codegen should care if we treat an non-zero-size empty field as if it's zero-size.

dwblaikie · 2024-06-17T23:06:19Z

Oh, in this particular case, the issue isn't the computed datasize, it's that FieldDecl::isZeroSize() returns the wrong result. If that's the case, maybe we can just change FieldDecl::isZeroSize() to say the field is zero size? So essentially, we pretend all empty fields are no_unique_address. Nothing in codegen should care if we treat an non-zero-size empty field as if it's zero-size.

(sorry if I'm jumping in without enough context... ) - couldn't the inverse be true, then - that codegen should ignore if something isZeroSize or not? So lldb doesn't have to put any particular value here?

efriedma-quic · 2024-06-17T23:44:32Z

That would mean if someone wrote struct Empty {}; struct Z { Empty a,b,c; }, we'd lower it to { [3 x i8] } instead of {%Empty, %Empty, %Empty}, which is a bit ugly. Other than that, sure, I guess we could do that.

Michael137 · 2024-06-18T10:39:43Z

couldn't the inverse be true, then - that codegen should ignore if something isZeroSize or not?

Just to clarify, is the suggestion here to remove the special handling of isZeroSize in the RecordLayoutBuilder?

So essentially, we pretend all empty fields are no_unique_address.

Fair point, we've considered this in the past, but had concerns that this might make the Clang front-end behave in unexpected ways or have ABI implications.

But open to either suggestion (though not having to work around this in LLDB would be great, if it's not a maintenance burden on Clang)

dwblaikie · 2024-06-18T18:13:11Z

That would mean if someone wrote struct Empty {}; struct Z { Empty a,b,c; }, we'd lower it to { [3 x i8] } instead of {%Empty, %Empty, %Empty}, which is a bit ugly. Other than that, sure, I guess we could do that.

Ah, fair enough. Glad to understand and I don't feel /super/ strongly either way. Though it might help with confidence if codegen didn't depend on this property at all (that it depends on the property a bit may make it harder to detect if later codegen depends on the property in a real/ABI-breaking way).

The struct layout validation stuff that @Michael137 found may be adequate to provide confidence (especially combined with fuzzing, maybe) without the need for the codegen-is-zero-length-independent invariant.

I don't feel too strongly - mostly happy with whatever Clang owners are happy with.

efriedma-quic · 2024-06-18T19:04:45Z

couldn't the inverse be true, then - that codegen should ignore if something isZeroSize or not?

Just to clarify, is the suggestion here to remove the special handling of isZeroSize in the RecordLayoutBuilder?

We currently need to distinguish between empty fields and non-empty fields: various parts of CodeGen expect to be able to get the LLVM struct field associated with a non-empty clang field. Maybe we can reduce that dependency, but that would be a deeper refactoring.

But we don't really care whether an empty field is formally "zero-size", so we could instead just check if the field is empty.

The change would be a bit wider than just RecordLayoutBuilder; there are other places in CodeGen that check isZeroSize for similar reasons.

That would mean if someone wrote struct Empty {}; struct Z { Empty a,b,c; }, we'd lower it to { [3 x i8] } instead of {%Empty, %Empty, %Empty}, which is a bit ugly. Other than that, sure, I guess we could do that.

Ah, fair enough. Glad to understand and I don't feel /super/ strongly either way. Though it might help with confidence if codegen didn't depend on this property at all (that it depends on the property a bit may make it harder to detect if later codegen depends on the property in a real/ABI-breaking way).

I think we have enough regression tests and assertions to detect breakage from minor adjustments here.

dwblaikie · 2024-06-26T18:15:57Z

Here's the smallest patch that would put explicit alignment on any packed structure:

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index a072475ba770..bbb13ddd593b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -64,7 +64,7 @@ static uint32_t getTypeAlignIfRequired(const Type *Ty, const ASTContext &Ctx) {
   // MaxFieldAlignmentAttr is the attribute added to types
   // declared after #pragma pack(n).
   if (auto *Decl = Ty->getAsRecordDecl())
-    if (Decl->hasAttr<MaxFieldAlignmentAttr>())
+    if (Decl->hasAttr<MaxFieldAlignmentAttr>() || Decl->hasAttr<PackedAttr>())
       return TI.Align;
 
   return 0;

But I don't think that's the right approach - I think what we should do is compute the natural alignment of the structure, then compare that to the actual alignment - and if they differ, we should put an explicit alignment on the structure. This avoids the risk that other alignment-influencing effects might be missed (and avoids the case of putting alignment on a structure that, when packed, just has the same alignment anyway - which is a minor issue, but nice to get right (eg: packed struct of a single char probably shouldn't have an explicit alignment - since it's the same as the implicit alignment anyway))

dwblaikie · 2024-06-26T18:21:09Z

Oh, this code was touched really recently in 66df614 (@augusto2112 ) (see notes in previous comments about how this code should be generalized)

Michael137 · 2024-06-27T10:12:48Z

Here's the smallest patch that would put explicit alignment on any packed structure:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index a072475ba770..bbb13ddd593b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -64,7 +64,7 @@ static uint32_t getTypeAlignIfRequired(const Type *Ty, const ASTContext &Ctx) {
   // MaxFieldAlignmentAttr is the attribute added to types
   // declared after #pragma pack(n).
   if (auto *Decl = Ty->getAsRecordDecl())
-    if (Decl->hasAttr<MaxFieldAlignmentAttr>())
+    if (Decl->hasAttr<MaxFieldAlignmentAttr>() || Decl->hasAttr<PackedAttr>())
       return TI.Align;
 
   return 0;
But I don't think that's the right approach - I think what we should do is compute the natural alignment of the structure, then compare that to the actual alignment - and if they differ, we should put an explicit alignment on the structure. This avoids the risk that other alignment-influencing effects might be missed (and avoids the case of putting alignment on a structure that, when packed, just has the same alignment anyway - which is a minor issue, but nice to get right (eg: packed struct of a single char probably shouldn't have an explicit alignment - since it's the same as the implicit alignment anyway))

Thanks for the analysis! If we can emit alignment for packed attributes consistently then we probably can get rid of most of the InferAlignment logic in the RecordLayoutBuilder (it seems to me, this logic was introduced for the purposes of packed structs), which would address the issue I saw with laying out [[no_unique_address]] fields. Trying this now.

(side note, one quirk of the DW_AT_alignment emission is that it doesn't propagate to derived classes, #73623, interestingly the topic of InferAlignment came up there as well, but I forgot most of the details from the time I looked a that. I guess the idea back then was to expand InferAlignment so we transitively derive the alignment when we lay out the type. But this seems orthogonal to your suggestions).

Adds test that checks that LLDB correctly infers the alignment of packed structures. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` where it assumes that overlapping field offsets imply a packed structure and thus sets alignment to `1`. See discussion in llvm#93809. Also adds two XFAIL-ed tests: 1. where LLDB doesn't correctly infer the alignment of a derived class whose base has an explicit `DW_AT_alignment. See llvm#73623. 2. where the aforementioned `InferAlignment` kicks in for overlapping fields (but in this case incorrectly since the structure isn't actually packed).

Adds test that checks whether LLDB correctly infers the alignment of packed structures. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` where it assumes that overlapping field offsets imply a packed structure and thus sets alignment to `1`. See discussion in llvm#93809. While here, also added a test-case where we check alignment of a class whose base has an explicit `DW_AT_alignment (those don't get transitively propagated in DWARF, but don't seem like a problem for LLDB). Lastly, also added an XFAIL-ed tests where the aforementioned `InferAlignment` kicks in for overlapping fields (but in this case incorrectly since the structure isn't actually packed).

Adds test that checks whether LLDB correctly infers the alignment of packed structures. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` where it assumes that overlapping field offsets imply a packed structure and thus sets alignment to `1`. See discussion in #93809. While here, also added a test-case where we check alignment of a class whose base has an explicit `DW_AT_alignment (those don't get transitively propagated in DWARF, but don't seem like a problem for LLDB). Lastly, also added an XFAIL-ed tests where the aforementioned `InferAlignment` kicks in for overlapping fields (but in this case incorrectly since the structure isn't actually packed).

Follow-up to llvm#96932 Adds XFAILed test where LLDB incorrectly infers the alignment of a derived class whose bases are overlapping due to [[no_unique_address]]. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` assumes that overlapping base offsets imply a packed structure and thus sets alignment to 1. See discussion in llvm#93809. Also adds test where LLDB correctly infers an alignment of `1` when a packed base class is overlapping with other bases. Lastly, there were a couple of `alignof` inconsistencies which I encapsulated in an XFAIL-ed `packed-alignof.cpp`.

…97068) Follow-up to #96932 Adds XFAILed test where LLDB incorrectly infers the alignment of a derived class whose bases are overlapping due to [[no_unique_address]]. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` assumes that overlapping base offsets imply a packed structure and thus sets alignment to 1. See discussion in #93809. Also adds test where LLDB correctly infers an alignment of `1` when a packed base class is overlapping with other bases. Lastly, there were a couple of `alignof` inconsistencies which I encapsulated in an XFAIL-ed `packed-alignof.cpp`.

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 examples 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 to affect 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`.

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 examples 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 to affect 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`. (cherry picked from commit f593891)

Adds test that checks whether LLDB correctly infers the alignment of packed structures. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` where it assumes that overlapping field offsets imply a packed structure and thus sets alignment to `1`. See discussion in llvm#93809. While here, also added a test-case where we check alignment of a class whose base has an explicit `DW_AT_alignment (those don't get transitively propagated in DWARF, but don't seem like a problem for LLDB). Lastly, also added an XFAIL-ed tests where the aforementioned `InferAlignment` kicks in for overlapping fields (but in this case incorrectly since the structure isn't actually packed).

…lvm#97068) Follow-up to llvm#96932 Adds XFAILed test where LLDB incorrectly infers the alignment of a derived class whose bases are overlapping due to [[no_unique_address]]. Specifically, the `InferAlignment` code-path of the `ItaniumRecordLayoutBuilder` assumes that overlapping base offsets imply a packed structure and thus sets alignment to 1. See discussion in llvm#93809. Also adds test where LLDB correctly infers an alignment of `1` when a packed base class is overlapping with other bases. Lastly, there were a couple of `alignof` inconsistencies which I encapsulated in an XFAIL-ed `packed-alignof.cpp`.

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 examples 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 to affect 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`.

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).

Michael137 · 2024-07-16T04:00:54Z

Closing in favour of #96422

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

Michael137 requested review from ldionne, AaronBallman and dwblaikie May 30, 2024 11:51

Michael137 requested a review from JDevlieghere as a code owner May 30, 2024 11:51

llvmbot added clang Clang issues not falling into any other category lldb clang:codegen IR generation bugs: mangling, exceptions, etc. labels May 30, 2024

Michael137 added 2 commits May 30, 2024 12:56

fixup! clang-format

882d231

fixup! python format

d5e8bd0

Michael137 requested review from efriedma-quic and rjmccall May 30, 2024 12:23

efriedma-quic requested a review from urnathan May 30, 2024 16:58

Michael137 mentioned this pull request Jun 27, 2024

[lldb][test] Add test-cases for packed/aligned structures #96932

Merged

Michael137 mentioned this pull request Jun 28, 2024

[lldb][test] Add tests for alignof on class with overlapping bases #97068

Merged

Michael137 closed this Jul 16, 2024

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

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

Uh oh!

Conversation

Michael137 commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 30, 2024

Uh oh!

github-actions bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwblaikie commented May 30, 2024

Uh oh!

efriedma-quic commented May 30, 2024

Uh oh!

rjmccall commented May 30, 2024

Uh oh!

Michael137 commented May 31, 2024

Uh oh!

efriedma-quic commented May 31, 2024

Uh oh!

Michael137 commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael137 commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic commented Jun 17, 2024

Uh oh!

dwblaikie commented Jun 17, 2024

Uh oh!

efriedma-quic commented Jun 17, 2024

Uh oh!

Michael137 commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwblaikie commented Jun 18, 2024

Uh oh!

efriedma-quic commented Jun 18, 2024

Uh oh!

dwblaikie commented Jun 26, 2024

Uh oh!

dwblaikie commented Jun 26, 2024

Uh oh!

Michael137 commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael137 commented Jul 16, 2024

Uh oh!

Uh oh!

Michael137 commented May 30, 2024 •

edited

Loading

llvmbot commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

Michael137 commented Jun 17, 2024 •

edited

Loading

Michael137 commented Jun 17, 2024 •

edited

Loading

Michael137 commented Jun 18, 2024 •

edited

Loading

Michael137 commented Jun 27, 2024 •

edited

Loading