[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

1500

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

Perhaps even putting it into its own helper function

1505

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

1503

1505

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

1520

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

2 ↗

(On Diff #511362)

Description needs fixing

22 ↗

(On Diff #511362)

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

1 ↗

(On Diff #511362)

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

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

3 ↗

(On Diff #511362)

Just for sanity checking in the test

23 ↗

(On Diff #511362)

same as above

[Michael137]

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

1505

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

25–42 ↗

(On Diff #511362)

[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

1505

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

1505

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

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

2227

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?

2234

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.

2246

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

2227

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)

2234

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

2246

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

protected:
   // module.
   lldb::ModuleSP GetModuleForType(const DWARFDIE &die);
 
 private:
   struct FieldInfo {
     uint64_t bit_size = 0;
     uint64_t bit_offset = 0;
     bool is_bitfield = false;
+    clang::FieldDecl *field_decl = nullptr;
 
     FieldInfo() = default;
 
     void SetIsBitfield(bool flag) { is_bitfield = flag; }
     bool IsBitfield() { return is_bitfield; }
 
     bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
       // Any subsequent bitfields must not overlap and must be at a higher
private:
   lldb::TypeSP ParseEnum(const lldb_private::SymbolContext &sc,
                          const DWARFDIE &die, ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParseSubroutine(const DWARFDIE &die,
                                ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParseArrayType(const DWARFDIE &die,
                               const ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParsePointerToMemberType(const DWARFDIE &die,
                                         const ParsedDWARFTypeAttributes &attrs);
+  void FixupBaseClasses(
+      std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
+      const lldb_private::ClangASTImporter::LayoutInfo &layout_info,
+      long byte_offset);
 
   /// Parses a DW_TAG_inheritance DIE into a base/super class.
   ///
   /// \param die The DW_TAG_inheritance DIE to parse.
   /// \param parent_die The parent DIE of the given DIE.
   /// \param class_clang_type The C++/Objective-C class representing parent_die.
   /// For an Objective-C class this method sets the super class on success. For
   /// a C++ class this will *not* add the result as a base class.

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

if (std::optional<uint64_t> clang_type_size =
           clang_type.GetByteSize(nullptr)) {
     return dwarf->MakeType(die.GetID(), attrs.name, *clang_type_size, nullptr,
                            LLDB_INVALID_UID, Type::eEncodingIsUID, nullptr,
                            clang_type, Type::ResolveState::Forward);
   }
   return nullptr;
 }
 
+void DWARFASTParserClang::FixupBaseClasses(
+    std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
+    const ClangASTImporter::LayoutInfo &layout_info, long byte_offset) {
+  for (auto it = base_classes.rbegin(); it != base_classes.rend(); ++it) {
+    clang::CXXRecordDecl *prev_base_decl =
+        (*it)->getType()->getAsCXXRecordDecl();
+    // We've already marked this class, exit.
+    if (prev_base_decl->isEmpty())
+      break;
+    auto it_layout = layout_info.base_offsets.find(prev_base_decl);
+    if (it_layout == layout_info.base_offsets.end())
+      continue;
+    // We found a normal base class, exit.
+    if (it_layout->second.getQuantity() < byte_offset)
+      break;
+    prev_base_decl->markEmpty();
+    for (auto *field : prev_base_decl->fields())
+      field->addAttr(clang::NoUniqueAddressAttr::Create(m_ast.getASTContext(),
+                                                        clang::SourceRange()));
+  }
+}
+
 void DWARFASTParserClang::ParseInheritance(
     const DWARFDIE &die, const DWARFDIE &parent_die,
     const CompilerType class_clang_type, const AccessType default_accessibility,
     const lldb::ModuleSP &module_sp,
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
     ClangASTImporter::LayoutInfo &layout_info) {
   auto ast =
       class_clang_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
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;
 
-  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 {
+    // DWARF doesn't have any representation for [[no_unique_address]]
+    // attribute. Empty base classes with [[no_unique_address]] fields
+    // confuse lldb and prevent construction of object memory layout.
+    // To fix this we scan base classes in reverse order to determine
+    // overlapping offsets. Wnen found we consider such class as empty
+    // base with all fields having [[no_unique_address]] attribute.
+    FixupBaseClasses(base_classes, layout_info, member_byte_offset);
     layout_info.base_offsets.insert(std::make_pair(
         ast->GetAsCXXRecordDecl(base_class_clang_type.GetOpaqueQualType()),
         clang::CharUnits::fromQuantity(member_byte_offset)));
   }
+  base_classes.push_back(std::move(result));
 }
 
 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 (sc_parent_tag == DW_TAG_compile_unit ||
       sc_parent_tag == DW_TAG_partial_unit) {
     symbol_context_scope = sc.comp_unit;
   } else if (sc.function != nullptr && sc_parent_die) {
     symbol_context_scope =
         sc.function->GetBlock(true).FindBlockByID(sc_parent_die.GetID());
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(
void DWARFASTParserClang::ParseSingleMember(
 
   TypeSystemClang::RequireCompleteType(member_clang_type);
 
   clang::FieldDecl *field_decl = TypeSystemClang::AddFieldToRecordType(
       class_clang_type, attrs.name, member_clang_type, attrs.accessibility,
       attrs.bit_size);
 
   m_ast.SetMetadataAsUserID(field_decl, die.GetID());
-
+  last_field_info.field_decl = field_decl;
   layout_info.field_offsets.insert(
       std::make_pair(field_decl, field_bit_offset));
 }
 
 bool DWARFASTParserClang::ParseChildMembers(
     const DWARFDIE &parent_die, CompilerType &class_clang_type,
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
     std::vector<DWARFDIE> &member_function_dies,
     DelayedPropertyList &delayed_properties,
     const AccessType default_accessibility,
     ClangASTImporter::LayoutInfo &layout_info) {
   if (!parent_die)
     return false;
 
   FieldInfo last_field_info;
+  std::vector<FieldInfo> seen_fields;
 
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
   auto ts = class_clang_type.GetTypeSystem();
   auto ast = ts.dyn_cast_or_null<TypeSystemClang>();
   if (ast == nullptr)
     return false;
 
   for (DWARFDIE die : parent_die.children()) {
     dw_tag_t tag = die.Tag();
 
     switch (tag) {
     case DW_TAG_APPLE_property:
       ParseObjCProperty(die, parent_die, class_clang_type, delayed_properties);
       break;
 
     case DW_TAG_member:
       ParseSingleMember(die, parent_die, class_clang_type,
                         default_accessibility, layout_info, last_field_info);
+      if (!last_field_info.field_decl)
+        break;
+      for (auto it = seen_fields.rbegin(); it != seen_fields.rend(); ++it) {
+        FieldInfo &fi = *it;
+        if (fi.field_decl->hasAttr<clang::NoUniqueAddressAttr>())
+          break;
+        if (fi.bit_offset >= last_field_info.bit_offset)
+          fi.field_decl->addAttr(clang::NoUniqueAddressAttr::Create(
+              ast->getASTContext(), clang::SourceRange()));
+        else
+          break;
+      }
+      FixupBaseClasses(base_classes, layout_info,
+                        last_field_info.bit_offset / 8);
+      seen_fields.push_back(last_field_info);
       break;
 
     case DW_TAG_subprogram:
       // Let the type parsing code handle this one for us.
       member_function_dies.push_back(die);
       break;
 
     case DW_TAG_inheritance:

lldb/test/API/lang/cpp/no_unique_address/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py

+"""
+Test that we correctly handle [[no_unique_address]] attribute.
+"""
+
+import lldb
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestInlineNamespace(TestBase):
+
+    def test(self):
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(self,
+            "// Set breakpoint here.", lldb.SBFileSpec("main.cpp"))
+
+        self.expect_expr("_f1.a", result_type="long", result_value="42")
+        self.expect_expr("_f1.b", result_type="long", result_value="52")
+        self.expect_expr("_f2.v", result_type="long", result_value="42")
+        self.expect_expr("_f3.v", result_type="long", result_value="42")
+        self.expect_expr("_f4.v", result_type="long", result_value="42")
+        self.expect_expr("_f5.v1", result_type="long", result_value="42")
+        self.expect_expr("_f5.v2", result_type="long", result_value="52")
+        self.expect_expr("_f6.v1", result_type="long", result_value="42")
+        self.expect_expr("_f6.v2", result_type="long", result_value="52")

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

+struct C
+{
+ long c,d;
+};
+
+struct Q
+{
+ long h;
+};
+
+struct D
+{
+};
+
+struct B
+{
+  [[no_unique_address]] D x;
+};
+
+struct E
+{
+  [[no_unique_address]] D x;
+};
+
+struct Foo1 : B,E,C
+{
+ long a = 42,b = 52;
+} _f1;
+
+struct Foo2 : B,E
+{
+ long v = 42;
+} _f2;
+
+struct Foo3 : C,B,E
+{
+ long v = 42;
+} _f3;
+
+struct Foo4 : B,C,E,Q
+{
+ long v = 42;
+} _f4;
+
+struct Foo5 : B,C,E
+{
+ [[no_unique_address]] D x1;
+ [[no_unique_address]] D x2;
+ long v1 = 42;
+ [[no_unique_address]] D y1;
+ [[no_unique_address]] D y2;
+ long v2 = 52;
+ [[no_unique_address]] D z1;
+ [[no_unique_address]] D z2;
+} _f5;
+
+struct Foo6 : B,E
+{
+ long v1 = 42;
+ [[no_unique_address]] D y1;
+ [[no_unique_address]] D y2;
+ long v2 = 52;
+} _f6;
+
+int main() {
+  return 0; // Set breakpoint here.
+}