Skip to content

[LLDB] Respect the DW_AT_alignment attribute. #73307

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 28, 2023
Merged

[LLDB] Respect the DW_AT_alignment attribute. #73307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fb8fd5
[LLDB] Respect the DW_AT_alignment attribute.
hokein Nov 24, 2023
fc488ec
Update a stale comment.
hokein Nov 28, 2023
9a8afb5
Make clang-format happy
hokein Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,10 @@ ParsedDWARFTypeAttributes::ParsedDWARFTypeAttributes(const DWARFDIE &die) {
byte_size = form_value.Unsigned();
break;

case DW_AT_alignment:
alignment = form_value.Unsigned();
break;

case DW_AT_byte_stride:
byte_stride = form_value.Unsigned();
break;
Expand Down Expand Up @@ -1921,17 +1925,21 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
die.GetOffset(), attrs.name.GetCString());
}

// If the byte size of the record is specified then overwrite the size
// that would be computed by Clang. This is only needed as LLDB's
// TypeSystemClang is always in C++ mode, but some compilers such as
// GCC and Clang give empty structs a size of 0 in C mode (in contrast to
// the size of 1 for empty structs that would be computed in C++ mode).
if (attrs.byte_size) {
// Setting authority byte size and alignment for empty structures.
//
// If the byte size or alignmenet of the record is specified then
// overwrite the ones that would be computed by Clang.
// This is only needed as LLDB's TypeSystemClang is always in C++ mode,
// but some compilers such as GCC and Clang give empty structs a size of 0
// in C mode (in contrast to the size of 1 for empty structs that would be
// computed in C++ mode).
if (attrs.byte_size || attrs.alignment) {
clang::RecordDecl *record_decl =
TypeSystemClang::GetAsRecordDecl(clang_type);
if (record_decl) {
ClangASTImporter::LayoutInfo layout;
layout.bit_size = *attrs.byte_size * 8;
layout.bit_size = attrs.byte_size.value_or(0) * 8;
layout.alignment = attrs.alignment.value_or(0) * 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this extra accounting for alignment here? This block used to be solely for cases where empty structs had special byte-size requirements. Is it because in your tests alignof(EmptyStruct) wouldn't trigger CompleteRecordType?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because in your tests alignof(EmptyStruct) wouldn't trigger CompleteRecordType?

Yes, exactly, this is the major reason. The codepath in CompleteRecordType has a conditional statement if (!layout_info.field_offsets.empty() || !layout_info.base_offsets.empty() || !layout_info.vbase_offsets.empty()) where an empty structure will never hit.

We could tweak the if condition there, but I'm less certain about it, my reading of code is that the location here seem to be responsible for empty structures (happy to change if this is not reasonable).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. If we have a byte size computed by clang, it was always using it and it helped with C structs being computed in a C++ type system when the size was zero or 1. Now we will also respect the alignment. Also the ClangASTImporter::LayoutInfo::bit_size is initialized to zero in the header file, so even though we will make it into this case now if we have only alignment,it shouldn't change anything AFAIK.

GetClangASTImporter().SetRecordLayout(record_decl, layout);
}
}
Expand Down Expand Up @@ -2270,6 +2278,9 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
if (layout_info.bit_size == 0)
layout_info.bit_size =
die.GetAttributeValueAsUnsigned(DW_AT_byte_size, 0) * 8;
if (layout_info.alignment == 0)
layout_info.alignment =
die.GetAttributeValueAsUnsigned(llvm::dwarf::DW_AT_alignment, 0) * 8;

clang::CXXRecordDecl *record_decl =
m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
Expand Down
1 change: 1 addition & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ struct ParsedDWARFTypeAttributes {
lldb_private::plugin::dwarf::DWARFFormValue type;
lldb::LanguageType class_language = lldb::eLanguageTypeUnknown;
std::optional<uint64_t> byte_size;
std::optional<uint64_t> alignment;
size_t calling_convention = llvm::dwarf::DW_CC_normal;
uint32_t bit_stride = 0;
uint32_t byte_stride = 0;
Expand Down
4 changes: 4 additions & 0 deletions lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,7 @@ def test(self):

# The offset of f2 should be 8 because of `alignas(8)`.
self.expect_expr("(intptr_t)&d3g.f2 - (intptr_t)&d3g", result_value="8")

# Verify specified class alignments.
self.expect_expr("alignof(B2)", result_value="8")
self.expect_expr("alignof(EmptyClassAlign8)", result_value="8")
3 changes: 3 additions & 0 deletions lldb/test/API/lang/cpp/alignas_base_class/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ struct D : B1, B2 {};

D d3g;

struct alignas(8) EmptyClassAlign8 {
} t;

int main() {}