[lldb][DWARF] Infer no_unique_address attribute

[kpdev42]

Summary

The no_unique_address attribute is currently a part of C++20 standard and is being used in both libstdc++ and libc++. This causes problems when debugging C++ code with lldb because the latter doesn't expect two non-empty base classes having the same offset and crashes with assertion. Patch attempts to infer this attribute by detecting two consecutive base classes with the same offset to derived class and marking first of those classes as empty and adding no_unique_address attribute to all of its fields.

~~

Huawei RRI, OS Lab

[Michael137]

From a first glance looks ok but ideally the clang changes would be separate reviews. E.g.,

1. Adding markEmpty()
2. ASTImporter change with unit-test
3. LLDB changes

[clayborg]

Changes look fine to me. I would like someone that specializes in the expression parser to give the final ok though.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

551 ↗

(On Diff #494936)

revert whitespace only changes

[balazske]

clang/lib/AST/ASTImporter.cpp

3896 ↗

(On Diff #494936)

The import of attributes is handled in function ASTImporter::Import(Decl*). This new line will probably copy all attributes, that may not work in all cases dependent on the attribute types. This may interfere with the later import of attributes, probably these will be duplicated. What was the need for this line? (Simple attributes that do not have references to other nodes could be copied at this place.)

[kpdev42]

clang/lib/AST/ASTImporter.cpp

3896 ↗

(On Diff #494936)

Unfortunately it is too late to copy attribute in ASTImporter::Import(Decl*), because field has already been added to record in a call to ImportImpl (VisitFieldDecl/addDeclInternal). I've reused the current way of cloning attributes in VisitFieldDecl.

[kpdev42]

Update patch after landing D145057

[kpdev42]

ping

[Michael137]

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

1482

Should we add a comment describing why this block is necessary?

Perhaps even putting it into its own helper function

1487

The idea seem reasonable in general.

Though this wouldn't work for overlapping member offsets. E.g.,:

struct C
{
 long c,d;
};

struct D
{
};

struct B
{
  [[no_unique_address]] D x;
};

struct E
{
  [[no_unique_address]] D x;
};

struct Foo : B, E, C {

};

Here B and C have offset 0x0, but E can have offset 0x1. So we wouldn't attach the attribute for E and would still crash. Maybe we should check for overlapping offsets, not just equal

[kpdev42]

Address review comments

[Michael137]

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

1485

1487

The main problem I still see with this is that if we have something like:

struct A : C, B {

};

then we mark C's fields as empty and leave B as is. This still leads to the same crash later on.

Perhaps we should mark we could check the size of the struct and decide based on that which one is the "empty" one

1502

I think it would still be nice to have this as a private helper function on DWARFASTParserClang. But don't feel very strongly about it so feel free to ignore

lldb/test/API/types/TestEmptyBase.py

3

Description needs fixing

23

Ideally this test would live in lldb/test/API/lang/cpp/no_unique_address/

The usual structure is that we have a Makefile in the same directory which consists of:

CXX_SOURCES := main.cpp
                       
include Makefile.rules

Then in the test(self) you just call self.build(). You don't need the setUp(self) and setTearDownCleanup(...) calls then.

Check lldb/test/API/commands/expression/inline-namespace/TestInlineNamespace.py for example.

lldb/test/API/types/empty_base_type.cpp

2

Can we add more test cases. E.g.,:

struct A : C, B {
 ...
};

4

Just for sanity checking in the test

24

same as above

[Michael137]

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

1487

Interestingly there was a discussion on the DWARF mailing list about this some time ago: https://www.mail-archive.com/[e-mail]/msg00880.html

There might be room to changing the emitted DWARF to make it easier to determine the empty structure. I will gauge opinions on this thread later today

[Michael137]

lldb/test/API/types/TestEmptyBase.py

26–43

[Michael137]

FYI, this bugfix was attempted back in 2021 here: https://reviews.llvm.org/D101237

[kpdev42]

Address review comments

[kpdev42]

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

1487

Unfortunately we cannot analyze record size, because it is always 1 for empty records, whether or not [[no_unique_address]] is used. However we still can analyze field offsets, I think. This what an updated patch does and it seems to handle more different cases

[kpdev42]

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

1487

Yes, this will help a lot, however many people use older versions of clang compiler and also gcc. This fix might be useful for them so far

[Michael137]

lldb/test/API/lang/cpp/no_unique_address/main.cpp

38 ↗

(On Diff #511683)

I haven't checked the reworked logic yet, but it still crashes on this:

self.expect_expr("_f3")

I'm leaning towards not trying to support older compilers if it gets too complicated. Proposing a DW_AT_no_unique_address seems like the best option to me since that's pretty trivial to implement and requires almost no support on the LLDB side.

CC: @dblaikie @aprantl (since both were part of the original discussion)

[kpdev42]

Thanks for pointing out the bug @Michael137 . It seems that clang assigns arbitrary offsets for non_unique_address so analyzing them brings me nowhere. In this patch I tried assigning no_unique_address to every empty structure and this fixed issue you found, making the code changes much smaller and simpler. The lldb test suite pass for me as well

[Michael137]

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

2215

Generally I'm not sure if attaching a clang::NoUniqueAddressAttr to every empty field is the right approach. That goes slightly against our attempts to construct an AST that's faithful to the source to avoid unpredictable behaviour (which isn't always possible but for the most part we try). This approach was considered in https://reviews.llvm.org/D101237 but concern was raised about it affecting ABI, etc., leading to subtle issues down the line.

Based on the the discussion in https://reviews.llvm.org/D101237 it seemed to me like the only two viable solutions are:

1. Add a DW_AT_byte_size of 0 to the empty field
2. Add a DW_AT_no_unique_address

AFAICT Jan tried to implement (1) but never seemed to be able to fully add support for this in the ASTImporter/LLDB. Another issue I see with this is that sometimes the byte-size of said field is not 0, depending on the context in which the structure is used.

I'm still leaning towards proposing a DW_AT_no_unique_address. Which is pretty easy to implement and also reason about from LLDB's perspective. @dblaikie @aprantl does that sound reasonable to you?

2222

Typically the call to record_decl->fields() below would worry me, because if the decl !hasLoadedFieldsFromExternalStorage() then we'd start another ASTImport process, which could lead to some unpredictable behaviour if we are already in the middle of an import. But since CompleteTagDeclarationDefinition sets setHasLoadedFieldsFromExternalStorage(true) I *think* we'd be ok. Might warrant a comment.

2234

Why do we need to mark the parents empty here again? Wouldn't they have been marked in ParseStructureLikeDIE?

[kpdev42]

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

2215

I think that lldb jitter relies on value of DW_AT_member location when/if empty structure address is taken, so assigning no_unique_address attribute shouldn't, in my opinion, affect anything. Also, as I understand, AST obtained from DWARF will not (and cannot) be identical to the source (e.g. because of optimizations)

2222

We can probably use keys of DenseMap in layout_info.base_offsets to stay safe, can't we?

2234

ParseStructureLikeDIE marks only trivially empty records (with no children or with only children being template parameters). All non-trivially empty structs (with only children being other empty structs) are marked here.

[kpdev42]

So what are next steps? Are we going for implementation of DW_AT_no_unique_address (which is going to be a non-standard extension) ?
@dblaikie @aprantl @Michael137

[Michael137]

In D143347#4298613, @kpdev42 wrote:

So what are next steps? Are we going for implementation of DW_AT_no_unique_address (which is going to be a non-standard extension) ?
@dblaikie @aprantl @Michael137

in my opinion that would be the most convenient way forward. Perhaps something like DW_AT_LLVM_no_unique_address even?

Btw what does GCC/gdb do in this case?

[aprantl]

If we can come up with a counterexample where the heuristic in this patch is doing the wrong thin then I think emitting a DW_AT_LLVM_no_unique_address attribute sounds reasonable to me. But otherwise I don't see a problem with using a heuristic.

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

void DWARFASTParserClang::ParseInheritance(
   }
   std::unique_ptr<clang::CXXBaseSpecifier> result =
       ast->CreateBaseClassSpecifier(base_class_clang_type.GetOpaqueQualType(),
                                     accessibility, is_virtual,
                                     is_base_of_class);
   if (!result)
     return;
 
+  const clang::CXXBaseSpecifier *prev_base =
+      base_classes.empty() ? nullptr : base_classes.back().get();
   base_classes.push_back(std::move(result));
 
   if (is_virtual) {
     // Do not specify any offset for virtual inheritance. The DWARF
     // produced by clang doesn't give us a constant offset, but gives
     // us a DWARF expressions that requires an actual object in memory.
     // the DW_AT_data_member_location for a virtual base class looks
     // like:
     //      DW_AT_data_member_location( DW_OP_dup, DW_OP_deref,
     //      DW_OP_constu(0x00000018), DW_OP_minus, DW_OP_deref,
     //      DW_OP_plus )
     // Given this, there is really no valid response we can give to
     // clang for virtual base class offsets, and this should eventually
     // be removed from LayoutRecordType() in the external
     // AST source in clang.
   } else {
+    if (prev_base) {
+      clang::CXXRecordDecl *prev_base_decl =
+          prev_base->getType()->getAsCXXRecordDecl();
+      if (prev_base_decl && !prev_base_decl->isEmpty()) {
+        auto it = layout_info.base_offsets.find(prev_base_decl);
+        assert(it != layout_info.base_offsets.end());
+        if (it->second.getQuantity() == member_byte_offset) {
+          prev_base_decl->markEmpty();
+          for (auto *field : prev_base_decl->fields())
+            field->addAttr(clang::NoUniqueAddressAttr::Create(
+                ast->getASTContext(), clang::SourceRange()));
+        }
+      }
+    }
     layout_info.base_offsets.insert(std::make_pair(
         ast->GetAsCXXRecordDecl(base_class_clang_type.GetOpaqueQualType()),
         clang::CharUnits::fromQuantity(member_byte_offset)));
   }
 }
 
 TypeSP DWARFASTParserClang::UpdateSymbolContextScopeForType(
     const SymbolContext &sc, const DWARFDIE &die, TypeSP type_sp) {
   if (!type_sp)
     return type_sp;
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   DWARFDIE sc_parent_die = SymbolFileDWARF::GetParentSymbolContextDIE(die);
   dw_tag_t sc_parent_tag = sc_parent_die.Tag();
 
   SymbolContextScope *symbol_context_scope = nullptr;
if (!layout_info.field_offsets.empty() || !layout_info.base_offsets.empty() ||
     if (type)
       layout_info.bit_size = type->GetByteSize(nullptr).value_or(0) * 8;
     if (layout_info.bit_size == 0)
       layout_info.bit_size =
           die.GetAttributeValueAsUnsigned(DW_AT_byte_size, 0) * 8;
 
     clang::CXXRecordDecl *record_decl =
         m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
     if (record_decl)
       GetClangASTImporter().SetRecordLayout(record_decl, layout_info);
   }
 
   return (bool)clang_type;
 }
 
 bool DWARFASTParserClang::CompleteEnumType(const DWARFDIE &die,
                                            lldb_private::Type *type,
                                            CompilerType &clang_type) {
   if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
     if (die.HasChildren()) {
       bool is_signed = false;
       clang_type.IsIntegerType(is_signed);
       ParseChildEnumerators(clang_type, is_signed,
                             type->GetByteSize(nullptr).value_or(0), die);
     }
     TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
   }
   return (bool)clang_type;
 }
 
 bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die,
                                                 lldb_private::Type *type,
                                                 CompilerType &clang_type) {
   SymbolFileDWARF *dwarf = die.GetDWARF();
 
   std::lock_guard<std::recursive_mutex> guard(

lldb/test/API/types/TestEmptyBase.py

+"""
+Test that recursive types are handled correctly.
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class EmptyBaseTestCase(TestBase):
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+        # Find the line number to break for main.c.
+        self.line = line_number('empty_base_type.cpp',
+                                '// Set breakpoint here.')
+
+        self.sources = {
+            'CXX_SOURCES': 'empty_base_type.cpp'}
+
+    def test(self):
+        """Test that recursive structs are displayed correctly."""
+        self.build(dictionary=self.sources)
+        self.setTearDownCleanup(dictionary=self.sources)
+        self.run_expr()
+
+    def run_expr(self):
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line(
+            self,
+            "empty_base_type.cpp",
+            self.line,
+            num_expected_locations=-1,
+            loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+        self.runCmd("expression _a")

lldb/test/API/types/empty_base_type.cpp

+struct C
+{
+ long c,d;
+};
+
+struct D
+{
+};
+
+struct B
+{
+  [[no_unique_address]] D x;
+};
+
+struct A : B,C
+{
+ long a,b;
+} _a;
+
+
+int main() {
+  return _a.a; // Set breakpoint here.
+}