[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

1480

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

Perhaps even putting it into its own helper function

1485

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

1483

1485

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

1500

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

1485

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

1485

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

1485

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

39

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

2212

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?

2219

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.

2231

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

2212

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)

2219

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

2231

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

if (is_virtual) {
     //      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 {
     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 (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 (symbol_context_scope == nullptr)
       symbol_context_scope = sc.function;
   } else {
     symbol_context_scope = sc.module_sp.get();
   }
 
   if (symbol_context_scope != nullptr)
     type_sp->SetSymbolContextScope(symbol_context_scope);
if (qualified_name.empty())
     qualified_name.append("::");
 
   qualified_name.append(name);
   qualified_name.append(GetDIEClassTemplateParams(die).AsCString(""));
 
   return qualified_name;
 }
 
+static bool IsEmptyStruct(const DWARFDIE &die) {
+  if (!die.HasChildren())
+    return true;
+
+  // Empty templates are actually empty structures.
+  return llvm::all_of(die.children(), [](const DWARFDIE &die) {
+    return die.Tag() == DW_TAG_template_type_parameter;
+  });
+}
+
 TypeSP
 DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
                                            const DWARFDIE &die,
                                            ParsedDWARFTypeAttributes &attrs) {
   TypeSP type_sp;
   CompilerType clang_type;
   const dw_tag_t tag = die.Tag();
   SymbolFileDWARF *dwarf = die.GetDWARF();
DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
   dwarf->GetUniqueDWARFASTTypeMap().Insert(unique_typename,
                                            *unique_ast_entry_up);
 
   if (!attrs.is_forward_declaration) {
     // Always start the definition for a class type so that if the class
     // has child classes or types that require the class to be created
     // for use as their decl contexts the class will be ready to accept
     // these child definitions.
-    if (!die.HasChildren()) {
+    if (IsEmptyStruct(die)) {
       // No children for this struct/union/class, lets finish it
       if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
         TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
       } else {
         dwarf->GetObjectFile()->GetModule()->ReportError(
 
             "DWARF DIE at {0:x16} named \"{1}\" was not able to start "
             "its "
             "definition.\nPlease file a bug and attach the file at the "
             "start of this error message",
             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) {
         clang::RecordDecl *record_decl =
             TypeSystemClang::GetAsRecordDecl(clang_type);
         if (record_decl) {
+          if (auto *cxx_record_decl =
+                  llvm::dyn_cast<clang::CXXRecordDecl>(record_decl))
+            cxx_record_decl->markEmpty();
           ClangASTImporter::LayoutInfo layout;
           layout.bit_size = *attrs.byte_size * 8;
           GetClangASTImporter().SetRecordLayout(record_decl, layout);
         }
       }
     } else if (clang_type_was_created) {
       // Start the definition if the class is not objective C since the
       // underlying decls respond to isCompleteDefinition(). Objective
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)
+    if (record_decl) {
+      bool is_empty = true;
+      if (layout_info.vbase_offsets.empty()) {
+        for (auto *field : record_decl->fields()) {
+          if (auto *cxx_record_decl = field->getType()->getAsCXXRecordDecl()) {
+            bool is_empty_field = cxx_record_decl->isEmpty();
+            if (is_empty_field)
+              field->addAttr(clang::NoUniqueAddressAttr::Create(
+                  m_ast.getASTContext(), clang::SourceRange()));
+            is_empty &= is_empty_field;
+
+          } else {
+            is_empty = false;
+          }
+        }
+        if (is_empty)
+          for (auto &parent : layout_info.base_offsets)
+            is_empty &= parent.first->isEmpty();
+        if (is_empty)
+          record_decl->markEmpty();
+      }
       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)) {

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("_f3", result_type="Foo3")
+        self.expect_expr("_f7", result_type="Foo7")
+
+        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")
+        self.expect_expr("_f7.v", result_type="long", result_value="42")

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;
+
+namespace NS {
+template<class T> struct Wrap {};
+}
+
+struct Foo7 : NS::Wrap<Foo6>,B,E {
+  NS::Wrap<Foo5> w1;
+  B b1;
+  long v = 42;
+} _f7;
+
+int main() {
+  return 0; // Set breakpoint here.
+}