[LLDB] Respect the DW_AT_alignment attribute. #73307

hokein · 2023-11-24T09:53:09Z

Part of fixes for #72913.

clang emits DW_AT_alignment attribute, however LLDB didn't respect it, resulting in incorrect RecordDecls built by lldb.

This only fixes non-inheritance cases. The inheritance case will be handled in a follow-up patch.

Part of fixes for llvm#72913. clang emits the DW_AT_alignment attribute, however LLDB didn't respect it, resulting an incorrect the RecordDecl built by lldb. This only fixes non-inheritance cases. The inheritance case will be handled in a follow-up patch.

llvmbot · 2023-11-24T09:53:41Z

@llvm/pr-subscribers-lldb

Author: Haojian Wu (hokein)

Changes

Part of fixes for #72913.

clang emits DW_AT_alignment attribute, however LLDB didn't respect it, resulting in incorrect RecordDecls built by lldb.

This only fixes non-inheritance cases. The inheritance case will be handled in a follow-up patch.

Full diff: https://github.com/llvm/llvm-project/pull/73307.diff

4 Files Affected:

(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+10-2)
(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (+1)
(modified) lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py (+4)
(modified) lldb/test/API/lang/cpp/alignas_base_class/main.cpp (+2)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index abe3c673e2cce69..a55ca0bf0f0fc1a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -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;
@@ -1926,12 +1930,13 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
       // 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) {
+      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;
           GetClangASTImporter().SetRecordLayout(record_decl, layout);
         }
       }
@@ -2270,6 +2275,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());
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 0247783217008e8..81b705a036189eb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -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;
diff --git a/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py b/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
index c7a88987733e176..7d97b0c42b7e166 100644
--- a/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
+++ b/lldb/test/API/lang/cpp/alignas_base_class/TestAlignAsBaseClass.py
@@ -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")
diff --git a/lldb/test/API/lang/cpp/alignas_base_class/main.cpp b/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
index 8dfced6c784e102..cf727e808017bcc 100644
--- a/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
+++ b/lldb/test/API/lang/cpp/alignas_base_class/main.cpp
@@ -10,4 +10,6 @@ struct D : B1, B2 {};
 
 D d3g;
 
+struct alignas(8) EmptyClassAlign8 {} t;
+
 int main() {}

github-actions · 2023-11-24T09:55:53Z

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

Michael137 · 2023-11-24T11:40:04Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

        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;


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?

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

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.

clayborg

LGTM

clayborg · 2023-11-28T00:23:06Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

        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;


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.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

hokein

Thanks for the review.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

hokein · 2023-11-28T11:27:35Z

@Michael137, I'm merging this patch now. I'm happy to address any post comments.

Michael137 · 2023-11-28T11:29:11Z

LGTM, I don't expect this to change with any of the other outstanding bugs, so seems fine to merge now

Part of fixes for llvm#72913. clang emits `DW_AT_alignment` attribute, however LLDB didn't respect it, resulting in incorrect RecordDecls built by lldb. This only fixes non-inheritance cases. The inheritance case will be handled in a follow-up patch. (cherry picked from commit 439b16e)

hokein requested a review from Michael137 November 24, 2023 09:53

hokein requested a review from JDevlieghere as a code owner November 24, 2023 09:53

llvmbot added the lldb label Nov 24, 2023

Michael137 reviewed Nov 24, 2023

View reviewed changes

clayborg approved these changes Nov 28, 2023

View reviewed changes

Michael137 reviewed Nov 28, 2023

View reviewed changes

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Show resolved Hide resolved

Update a stale comment.

fc488ec

hokein commented Nov 28, 2023

View reviewed changes

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Show resolved Hide resolved

Make clang-format happy

9a8afb5

hokein merged commit 439b16e into llvm:main Nov 28, 2023

hokein deleted the class-alignas branch November 28, 2023 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LLDB] Respect the DW_AT_alignment attribute. #73307

[LLDB] Respect the DW_AT_alignment attribute. #73307

Uh oh!

hokein commented Nov 24, 2023

Uh oh!

llvmbot commented Nov 24, 2023

Uh oh!

github-actions bot commented Nov 24, 2023 •

edited

Loading

Uh oh!

Michael137 Nov 24, 2023

Uh oh!

hokein Nov 24, 2023

Uh oh!

clayborg Nov 28, 2023

Uh oh!

clayborg left a comment

Uh oh!

clayborg Nov 28, 2023

Uh oh!

Uh oh!

hokein left a comment

Uh oh!

Uh oh!

hokein commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Uh oh!

[LLDB] Respect the DW_AT_alignment attribute. #73307

[LLDB] Respect the DW_AT_alignment attribute. #73307

Uh oh!

Conversation

hokein commented Nov 24, 2023

Uh oh!

llvmbot commented Nov 24, 2023

Uh oh!

github-actions bot commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael137 Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

hokein Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

clayborg Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

clayborg Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Uh oh!

github-actions bot commented Nov 24, 2023 •

edited

Loading