-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Conversation
…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.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Michael Buch (Michael137) ChangesThis is the outcome of the discussions we had in 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 This unblocks the libc++ Full diff: https://github.com/llvm/llvm-project/pull/93809.diff 4 Files Affected:
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;
+}
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis is the outcome of the discussions we had in 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 This unblocks the libc++ Full diff: https://github.com/llvm/llvm-project/pull/93809.diff 4 Files Affected:
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;
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
✅ With the latest revision this PR passed the Python code formatter. |
Could probably adjust the assertions to be 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. |
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. |
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 |
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.
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. |
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 Interestingly, I only just found out about the
Interestingly the only difference here is the alignment computed for |
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:
Which is a perfectly valid layout, and shows how However, LLDB's AST gets lowered to:
(side-note, the This won't pass the |
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 |
That would mean if someone wrote |
Just to clarify, is the suggestion here to remove the special handling of
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) |
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. |
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.
I think we have enough regression tests and assertions to detect breakage from minor adjustments here. |
Here's the smallest patch that would put explicit alignment on any packed structure:
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)) |
Oh, this code was touched really recently in 66df614 (@augusto2112 ) (see notes in previous comments about how this code should be generalized) |
Thanks for the analysis! If we can emit alignment for packed attributes consistently then we probably can get rid of most of the (side note, one quirk of the |
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 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`.
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).
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
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 theRecordLayoutBuilder
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 theDebuggerSupport
flag.This unblocks the libc++
compressed_pair
refactor in #93069