Skip to content
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

DebugInfo: No alignment info for "transitive" alignment-enforced structures. #73623

Open
hokein opened this issue Nov 28, 2023 · 7 comments
Open

Comments

@hokein
Copy link
Collaborator

hokein commented Nov 28, 2023

Split from #72913.

Consider the following case:

struct alignas(8) Base {} a;

struct Derived : Base {} b;

Debug info emitted by clang ./bin/clang -cc1 -emit-llvm -debug-info-kind=limited /tmp/t.cpp -o -

!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Derived", file: !7, line: 4, size: 64, flags: DIFlagTypePassByValue, elements: !9, identifier: "_ZTS7Derived")
!9 = !{!10}
!10 = !DIDerivedType(tag: DW_TAG_inheritance, scope: !8, baseType: !11, extraData: i32 0)
!11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Base", file: !7, line: 2, size: 64, align: 64, flags: DIFlagTypePassByValue, elements: !12, identifier: "_ZTS4Base")

The DICompositeType for Derived is missing the align attribute, it is a critical information for lldb to get correct layout for the structure.

This is a regression caused by https://reviews.llvm.org/D24426.

Comments from @Michael137:

FWIW, GCC does do this "alignment propagation" and attaches DW_AT_alignment to the derived class too: https://godbolt.org/z/j5MWdWYaf

@llvmbot
Copy link
Member

llvmbot commented Nov 28, 2023

@llvm/issue-subscribers-debuginfo

Author: Haojian Wu (hokein)

Split from #72913.

Consider the following case:

struct alignas(8) Base {} a;

struct Derived : Base {} b;

Debug info emitted by clang ./bin/clang -cc1 -emit-llvm -debug-info-kind=limited /tmp/t.cpp -o -

!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Derived", file: !7, line: 4, size: 64, flags: DIFlagTypePassByValue, elements: !9, identifier: "_ZTS7Derived")
!9 = !{!10}
!10 = !DIDerivedType(tag: DW_TAG_inheritance, scope: !8, baseType: !11, extraData: i32 0)
!11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Base", file: !7, line: 2, size: 64, align: 64, flags: DIFlagTypePassByValue, elements: !12, identifier: "_ZTS4Base")

The DICompositeType for Derived is missing the align attribute, it is a critical information for lldb to get correct layout for the structure.

This is a regression caused by https://reviews.llvm.org/D24426.

@llvmbot
Copy link
Member

llvmbot commented Nov 28, 2023

@llvm/issue-subscribers-lldb

Author: Haojian Wu (hokein)

Split from #72913.

Consider the following case:

struct alignas(8) Base {} a;

struct Derived : Base {} b;

Debug info emitted by clang ./bin/clang -cc1 -emit-llvm -debug-info-kind=limited /tmp/t.cpp -o -

!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Derived", file: !7, line: 4, size: 64, flags: DIFlagTypePassByValue, elements: !9, identifier: "_ZTS7Derived")
!9 = !{!10}
!10 = !DIDerivedType(tag: DW_TAG_inheritance, scope: !8, baseType: !11, extraData: i32 0)
!11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Base", file: !7, line: 2, size: 64, align: 64, flags: DIFlagTypePassByValue, elements: !12, identifier: "_ZTS4Base")

The DICompositeType for Derived is missing the align attribute, it is a critical information for lldb to get correct layout for the structure.

This is a regression caused by https://reviews.llvm.org/D24426.

@dwblaikie
Copy link
Collaborator

GCC Does include the alignment, Clang does not: https://godbolt.org/z/q5rGMvEMn

I could see an argument for Clang being correct & leaving it up to the consumer to do the layout - but DWARF does encode the member offsets, etc, when those could be derived with sufficient ABI knowledge, etc, so explicitly including the alignment when it's non-trivial seems maybe suitable.
(on the other hand neither GCC nor Clang include the alignment when it isn't overriden - and consumers derive it themselves by looking at the member types, etc, presumably? So maybe it's not that much of a burden for them to derive it in this case too?)

@Michael137
Copy link
Member

(on the other hand neither GCC nor Clang include the alignment when it isn't overriden - and consumers derive it themselves by looking at the member types, etc, presumably? So maybe it's not that much of a burden for them to derive it in this case too?

I had the same initial thought, since when alignment isn't specified, the ItaniumRecordLayoutBuilder has an InferAlignment path for each record. But in practice the inference isn't setting the alignment on derived classes appropriately (given #72913 still crashes after LLDB started respecting DW_AT_alignment in 439b16e). I guess an alternative to propagating this in DWARF could be to "fix" the InferAlignment path of the layout builder, if feasible.

@Michael137
Copy link
Member

Interestingly, I hacked something together that addresses #73563. With that in place we end up inferring the alignment correctly, presumably because the alignment calculations rely on data-size being set correctly.

So I suggest we focus on the other issue first, and see if that resolves all our problems

@hokein
Copy link
Collaborator Author

hokein commented Apr 2, 2024

Hi, @Michael137

You seem to take a look on this issue and #73563, is there any progress or update on them?

@Michael137
Copy link
Member

Michael137 commented Apr 2, 2024

Hi, @Michael137

You seem to take a look on this issue and #73563, is there any progress or update on them?

I've been meaning to follow-up on https://discourse.llvm.org/t/rfc-lldb-handling-no-unique-address-in-lldb/77483 but was sidetracked with other things. I think the direction we're taking is to make the Clang layout-builder assertions less restrictive for external sources (such as LLDB). This would resolve the original issue raised in #72913 (but wouldn't address the "DW_AT_alignment isn't propagated problem" raised here).

Michael137 added a commit to Michael137/llvm-project that referenced this issue 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants