[lldb][DWARFASTParserClang] Make GetCXXObjectParameter public and call it from unit-tests

[Michael137]

My goal is to remove the object_pointer member on
ParsedDWARFTypeAttributes since it's duplicating information that we
retrieve with GetCXXObjectParameter anyway. To continue having
coverage for the DW_AT_object_pointer code-paths, instead of checking the
attrs.object_pointer I'm now calling GetCXXObjectParameter directly.
We could find some very roundabout way to go via the Clang AST to check
that the object parameter was parsed correctly, but that quickly became
quite painful.

Depends on #144876

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

My goal is to remove the object_pointer member on
ParsedDWARFTypeAttributes since it's duplicating information that we
retrieve with GetCXXObjectParameter anyway. To continue having
coverage for the DW_AT_object_pointer code-paths, instead of checking the
attrs.object_pointer I'm now calling GetCXXObjectParameter directly.
We could find some very roundabout way to go via the Clang AST to check
that the object parameter was parsed correctly, but that quickly became
quite painful.

Depends on #144876

---

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

3 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+10-9)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (+4)
• (modified) lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp (+26-12)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 620501b304e63..4f79c8aa3f811 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -163,14 +163,15 @@ static bool TagIsRecordType(dw_tag_t tag) {
 /// a default DWARFDIE. If \c containing_decl_ctx is not a valid
 /// C++ declaration context for class methods, assume no object
 /// parameter exists for the given \c subprogram.
-static DWARFDIE
-GetCXXObjectParameter(const DWARFDIE &subprogram,
-                      const clang::DeclContext &containing_decl_ctx) {
+DWARFDIE
+DWARFASTParserClang::GetCXXObjectParameter(const DWARFDIE &subprogram,
+                                           const DWARFDIE &decl_ctx_die) {
+  assert(subprogram);
   assert(subprogram.Tag() == DW_TAG_subprogram ||
          subprogram.Tag() == DW_TAG_inlined_subroutine ||
          subprogram.Tag() == DW_TAG_subroutine_type);
 
-  if (!DeclKindIsCXXClass(containing_decl_ctx.getDeclKind()))
+  if (!decl_ctx_die.IsStructUnionOrClass())
     return {};
 
   if (DWARFDIE object_parameter =
@@ -1304,8 +1305,7 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
   clang::CallingConv calling_convention =
       ConvertDWARFCallingConventionToClang(attrs);
 
-  const DWARFDIE object_parameter =
-      GetCXXObjectParameter(die, *containing_decl_ctx);
+  const DWARFDIE object_parameter = GetCXXObjectParameter(die, decl_ctx_die);
 
   // clang_type will get the function prototype clang type after this
   // call
@@ -2411,12 +2411,13 @@ DWARFASTParserClang::ConstructDemangledNameFromDWARF(const DWARFDIE &die) {
   DWARFDeclContext decl_ctx = die.GetDWARFDeclContext();
   sstr << decl_ctx.GetQualifiedName();
 
+  DWARFDIE decl_ctx_die;
   clang::DeclContext *containing_decl_ctx =
-      GetClangDeclContextContainingDIE(die, nullptr);
+      GetClangDeclContextContainingDIE(die, &decl_ctx_die);
   assert(containing_decl_ctx);
 
-  const unsigned cv_quals = GetCXXMethodCVQuals(
-      die, GetCXXObjectParameter(die, *containing_decl_ctx));
+  const unsigned cv_quals =
+      GetCXXMethodCVQuals(die, GetCXXObjectParameter(die, decl_ctx_die));
 
   ParseChildParameters(containing_decl_ctx, die, is_variadic,
                        has_template_params, param_types, param_names);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 3994726aa6b3e..111604ce4068a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -112,6 +112,10 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
   void MapDeclDIEToDefDIE(const lldb_private::plugin::dwarf::DWARFDIE &decl_die,
                           const lldb_private::plugin::dwarf::DWARFDIE &def_die);
 
+  lldb_private::plugin::dwarf::DWARFDIE GetCXXObjectParameter(
+      const lldb_private::plugin::dwarf::DWARFDIE &subprogram,
+      const lldb_private::plugin::dwarf::DWARFDIE &decl_ctx_die);
+
 protected:
   /// Protected typedefs and members.
   /// @{
diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
index 6c77736113da3..2d4b79fed4a55 100644
--- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -889,18 +889,32 @@ TEST_F(DWARFASTParserClangTests, TestParseDWARFAttributes_ObjectPointer) {
   ASSERT_TRUE(context_die.IsValid());
   ASSERT_EQ(context_die.Tag(), DW_TAG_structure_type);
 
-  auto subprogram_definition = context_die.GetSibling();
-  ASSERT_TRUE(subprogram_definition.IsValid());
-  ASSERT_EQ(subprogram_definition.Tag(), DW_TAG_subprogram);
-  ASSERT_FALSE(subprogram_definition.GetAttributeValueAsOptionalUnsigned(
-      DW_AT_external));
-
-  auto param_die = subprogram_definition.GetFirstChild();
-  ASSERT_TRUE(param_die.IsValid());
-
-  ParsedDWARFTypeAttributes attrs(subprogram_definition);
-  EXPECT_TRUE(attrs.object_pointer.IsValid());
-  EXPECT_EQ(attrs.object_pointer, param_die);
+  {
+    auto decl_die = context_die.GetFirstChild();
+    ASSERT_TRUE(decl_die.IsValid());
+    ASSERT_EQ(decl_die.Tag(), DW_TAG_subprogram);
+    ASSERT_TRUE(decl_die.GetAttributeValueAsOptionalUnsigned(DW_AT_external));
+
+    auto param_die = decl_die.GetFirstChild();
+    ASSERT_TRUE(param_die.IsValid());
+
+    EXPECT_EQ(param_die,
+              ast_parser.GetCXXObjectParameter(decl_die, context_die));
+  }
+
+  {
+    auto subprogram_definition = context_die.GetSibling();
+    ASSERT_TRUE(subprogram_definition.IsValid());
+    ASSERT_EQ(subprogram_definition.Tag(), DW_TAG_subprogram);
+    ASSERT_FALSE(subprogram_definition.GetAttributeValueAsOptionalUnsigned(
+        DW_AT_external));
+
+    auto param_die = subprogram_definition.GetFirstChild();
+    ASSERT_TRUE(param_die.IsValid());
+
+    EXPECT_EQ(param_die, ast_parser.GetCXXObjectParameter(subprogram_definition,
+                                                          context_die));
+  }
 }
 
 TEST_F(DWARFASTParserClangTests, TestParseSubroutine_ExplicitObjectParameter) {

[labath]

Boy, that's a lot of setup for a simple function like this.

I might try to test this via yaml2obj + FileCheck over clang-ast dump of the module. Presumably, an error here should somehow manifest itself in the generated ast (or in a crash while producing it).

[Michael137]

Boy, that's a lot of setup for a simple function like this.

I might try to test this via yaml2obj + FileCheck over clang-ast dump of the module. Presumably, an error here should somehow manifest itself in the generated ast (or in a crash while producing it).

Good point, will try rewriting this in upcoming iterations!