Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented May 30, 2024

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.
@Michael137 Michael137 requested a review from JDevlieghere as a code owner May 30, 2024 11:51
@llvmbot llvmbot added clang Clang issues not falling into any other category lldb clang:codegen IR generation bugs: mangling, exceptions, etc. labels May 30, 2024
@llvmbot
Copy link
Member

llvmbot commented May 30, 2024

@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
Copy link
Member

llvmbot commented May 30, 2024

@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;
+}

Copy link

github-actions bot commented May 30, 2024

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

Copy link

github-actions bot commented May 30, 2024

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

@dwblaikie
Copy link
Collaborator

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
Copy link
Collaborator

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.

@efriedma-quic efriedma-quic requested a review from urnathan May 30, 2024 16:58
@rjmccall
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member Author

Michael137 commented Jun 17, 2024

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
Copy link
Member Author

Michael137 commented Jun 17, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

Michael137 commented Jun 18, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@Michael137
Copy link
Member Author

Michael137 commented Jun 27, 2024

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

Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jun 27, 2024
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).
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jun 27, 2024
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).
Michael137 added a commit that referenced this pull request Jun 28, 2024
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).
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jun 28, 2024
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`.
Michael137 added a commit that referenced this pull request Jun 29, 2024
…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`.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jul 1, 2024
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`.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jul 3, 2024
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)
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
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).
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…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`.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jul 5, 2024
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`.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jul 9, 2024
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`.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Jul 15, 2024
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`.
Michael137 added a commit that referenced this pull request Jul 16, 2024
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
Copy link
Member Author

Closing in favour of #96422

@Michael137 Michael137 closed this Jul 16, 2024
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category lldb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants