[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
https://github.com/Michael137 closed https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote: Closing in favour of https://github.com/llvm/llvm-project/pull/96422 https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote:
> 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 ) {
>// MaxFieldAlignmentAttr is the attribute added to types
>// declared after #pragma pack(n).
>if (auto *Decl = Ty->getAsRecordDecl())
> -if (Decl->hasAttr())
> +if (Decl->hasAttr() ||
> Decl->hasAttr())
>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 most of that logic was put
introduced there for the purpose of packed structs), which would address the
issue I saw with laying out `[[no_unique_address]]` fields. Trying this now
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote: Oh, this code was touched really recently in 66df6141659375e738d9b9b74bf79b2317576042 (@augusto2112 ) (see notes in previous comments about how this code should be generalized) https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote:
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 ) {
// MaxFieldAlignmentAttr is the attribute added to types
// declared after #pragma pack(n).
if (auto *Decl = Ty->getAsRecordDecl())
-if (Decl->hasAttr())
+if (Decl->hasAttr() || Decl->hasAttr())
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))
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote: The other effects of `packed` are encoded (the changes to the member offsets) but that's not the only effect, and we shouldn't use that effect (which isn't guaranteed to be observable - as in the case of "packed struct with a single member/that just happens to not have padding anyway") to try to divine the alignment. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote: oh, slight misquote above - `aligned(1)` is a no-op, as that attribute can't reduce the alignment... But I think my argument still stands, roughly as - if increases in alignment are explicitly specified, decreases in alignment should be too. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote:
Ah, I think this right solution to /this/ case is that the compiler should be
emitting the alignment for the structure?
Like there's no way for the debugger to know that this structure is
underaligned at the moment:
```
struct __attribute__((packed)) t1 {
int i;
};
```
Which means the debugger might generate code that assumes `t1` is naturally
aligned, and break (ARM crashes on underaligned operations, doesn't it? so if
you got a pointer to t1 back from some API that happened to have put it in an
underaligned location - then in your expression you tried to read `i`, this
could generate crashing code?)
Essentially the `packed` attribute on the above structure is providing the same
value/effect as `aligned(1)` and should produce the same DWARF, but it doesn't.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote:
While fixing the libc++ formatters in preparation for the [compressed_pair
change](https://github.com/llvm/llvm-project/issues/93069), i encountered
another issue which I'm not sure entirely how to best reconcile. There's [this
assumption in
`RecordLayoutBuilder`](https://github.com/llvm/llvm-project/blob/f782ff8fc6426890863be0791a9ace2394da3887/clang/lib/AST/RecordLayoutBuilder.cpp#L2258-L2264)
for external layouts, where, if we encounter overlapping field offsets, we
assume the structure is packed and set the alignment back to `1`:
```
uint64_t
ItaniumRecordLayoutBuilder::updateExternalFieldOffset(const FieldDecl *Field,
uint64_t ComputedOffset) {
uint64_t ExternalFieldOffset = External.getExternalFieldOffset(Field);
if (InferAlignment && ExternalFieldOffset < ComputedOffset) {
// The externally-supplied field offset is before the field offset we
// computed. Assume that the structure is packed.
Alignment = CharUnits::One();
PreferredAlignment = CharUnits::One();
InferAlignment = false;
}
...
```
The assumption in that comment doesn't hold for layouts where the overlap
occurred because of `[[no_unique_address]]`. In those cases, the alignment of
`1` will prevent us from aligning the `FieldOffset` correctly and cause LLDB to
read out the fields incorrectly.
We can't guard this with `Field->isZeroSize()` because we don't have the
attribute in the AST.
Can we infer packedness more accurately here?
Or maybe that's just putting a bandaid on a bigger underlying issue
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote: > It's not that hard to compute "no-data": non-RecordDecls are never no-data, > RecordDecls are no-data if they don't have a vtable pointer > (isDynamicClass()), and all fields are no-data. We can save it in the > CGRecordLayout. > > Assuming that's the route we want to go, of course, as opposed to just making > LLDB add no_unique_address markings to fields. Turns out there's `CodeGen::isEmptyField` which seems usable for this purpose. Created a draft PR here: https://github.com/llvm/llvm-project/pull/96422 for now https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: It's not that hard to compute "no-data": non-RecordDecls are never no-data, RecordDecls are no-data if they don't have a vtable pointer (isDynamicClass()), and all fields are no-data. We can save it in the CGRecordLayout. Assuming that's the route we want to go, of course, as opposed to just making LLDB add no_unique_address markings to fields. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote: Ah got it, thanks for clarifying. > so we could instead just check if the field is empty. Couldn't find an existing API for this on `FieldDecl`. Here "empty" would be "field is of a type which is empty"? Which might still cause complications because a `CXXRecordDecl::isEmpty` itself depends on `isZeroSize` of its fields. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote:
> > 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.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote:
> 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.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote:
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.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote: > 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? https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: 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. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote:
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 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 (0x0085 "B")
DW_AT_data_member_location (0x00)
DW_TAG_inheritance
DW_AT_type (0x00b9 "C")
DW_AT_data_member_location (0x01)
DW_TAG_inheritance
DW_AT_type (0x0047 "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, the LLDB's AST gets lowered to:
```
Layout:
```
(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).
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote: > > > 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 ~/Git/llvm-project/clang/test/CodeGenCXX/override-layout-no_unique_address.cpp -w -std=c++14 -fdump-record-layouts-simple -DNUA= -foverride-record-layout=nua.lldb > nua.lldb.after michaelbuch@Michaels-MacBook-Pro-6:~/Git/lldb-build-main-rel $ diff nua.lldb.after nua.before 52c52 < Alignment:8 --- > Alignment:64 ``` Interestingly the only difference here is the alignment computed for `Foo`. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: > > 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. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
Michael137 wrote: > 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. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
rjmccall wrote: 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`. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: 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. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
dwblaikie wrote: 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. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/93809
>From 91276f5b2dc05032a285b465c0c8a69541bb25c4 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Thu, 2 May 2024 09:05:01 +0100
Subject: [PATCH 1/3] [clang][lldb] Don't assert structure layout correctness
for layouts provided 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.
---
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 18 ---
.../API/lang/cpp/no_unique_address/Makefile | 3 ++
.../no_unique_address/TestNoUniqueAddress.py | 50 +++
.../API/lang/cpp/no_unique_address/main.cpp | 35 +
4 files changed, 100 insertions(+), 6 deletions(-)
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/Makefile
create mode 100644
lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/main.cpp
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 : 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 = 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 0..8b20bcb05
--- /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 0..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
+
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/93809
>From 91276f5b2dc05032a285b465c0c8a69541bb25c4 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Thu, 2 May 2024 09:05:01 +0100
Subject: [PATCH 1/2] [clang][lldb] Don't assert structure layout correctness
for layouts provided 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.
---
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 18 ---
.../API/lang/cpp/no_unique_address/Makefile | 3 ++
.../no_unique_address/TestNoUniqueAddress.py | 50 +++
.../API/lang/cpp/no_unique_address/main.cpp | 35 +
4 files changed, 100 insertions(+), 6 deletions(-)
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/Makefile
create mode 100644
lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/main.cpp
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 : 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 = 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 0..8b20bcb05
--- /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 0..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
+
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
github-actions[bot] wrote:
:warning: Python code formatter, darker found issues in your code. :warning:
You can test this locally with the following command:
``bash
darker --check --diff -r
971f1aaad3ca3680bfbab76212f498ca15b280a2...91276f5b2dc05032a285b465c0c8a69541bb25c4
lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
``
View the diff from darker here.
``diff
--- TestNoUniqueAddress.py 2024-05-30 11:43:43.00 +
+++ TestNoUniqueAddress.py 2024-05-30 11:53:51.778148 +
@@ -6,45 +6,62 @@
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(
+"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_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")
``
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 971f1aaad3ca3680bfbab76212f498ca15b280a2
91276f5b2dc05032a285b465c0c8a69541bb25c4 --
lldb/test/API/lang/cpp/no_unique_address/main.cpp
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index 32fb032214..b7aab51660 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -1154,7 +1154,7 @@ CodeGenTypes::ComputeRecordLayout(const RecordDecl *D,
llvm::StructType *Ty) {
if (!Context.getLangOpts().DebuggerSupport)
assert(AlignedNonVirtualTypeSizeInBits ==
- getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
+ getDataLayout().getTypeAllocSizeInBits(BaseTy) &&
"Type size mismatch!");
}
``
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
llvmbot wrote:
@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
https://github.com/llvm/llvm-project/issues/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
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 : 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 = 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 0..8b20bcb05
--- /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 0..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=[
+
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
llvmbot wrote:
@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
https://github.com/llvm/llvm-project/issues/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
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 : 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 = 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 0..8b20bcb05
--- /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 0..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=[
+
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
https://github.com/Michael137 created
https://github.com/llvm/llvm-project/pull/93809
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
https://github.com/llvm/llvm-project/issues/93069
>From 91276f5b2dc05032a285b465c0c8a69541bb25c4 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Thu, 2 May 2024 09:05:01 +0100
Subject: [PATCH] [clang][lldb] Don't assert structure layout correctness for
layouts provided 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.
---
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 18 ---
.../API/lang/cpp/no_unique_address/Makefile | 3 ++
.../no_unique_address/TestNoUniqueAddress.py | 50 +++
.../API/lang/cpp/no_unique_address/main.cpp | 35 +
4 files changed, 100 insertions(+), 6 deletions(-)
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/Makefile
create mode 100644
lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
create mode 100644 lldb/test/API/lang/cpp/no_unique_address/main.cpp
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 : 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 = 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 0..8b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/no_unique_address/Makefile
@@ -0,0 +1,3 @@
