Skip to content

[lldb][DWARFIndex] Use IDX_parent to implement GetFullyQualifiedType query #79932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

felipepiovezan
Copy link
Contributor

This commit changes DebugNamesDWARFIndex so that it now overrides GetFullyQualifiedType and attempts to use DW_IDX_parent, when available, to speed up such queries. When this type of information is not available, the base-class implementation is used.

With this commit, we now achieve the 4x speedups reported in 1.

…query

This commit changes DebugNamesDWARFIndex so that it now overrides
`GetFullyQualifiedType` and attempts to use DW_IDX_parent, when available, to
speed up such queries. When this type of information is not available, the
base-class implementation is used.

With this commit, we now achieve the 4x speedups reported in [1].

[1]: https://discourse.llvm.org/t/rfc-improve-dwarf-5-debug-names-type-lookup-parsing-speed/74151/38
@felipepiovezan
Copy link
Contributor Author

The unittests here depend on #79666

@llvmbot llvmbot added the lldb label Jan 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2024

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This commit changes DebugNamesDWARFIndex so that it now overrides GetFullyQualifiedType and attempts to use DW_IDX_parent, when available, to speed up such queries. When this type of information is not available, the base-class implementation is used.

With this commit, we now achieve the 4x speedups reported in 1.


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

6 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h (+4)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp (+99)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h (+9)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+3)
  • (modified) lldb/unittests/SymbolFile/DWARF/CMakeLists.txt (+1)
  • (added) lldb/unittests/SymbolFile/DWARF/DWARFDebugNamesIndexTest.cpp (+210)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
index a20a862d34029..7e6c5f51f4beb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h
@@ -47,6 +47,10 @@ class DWARFDeclContext {
 
   DWARFDeclContext() : m_entries() {}
 
+  DWARFDeclContext(llvm::ArrayRef<Entry> entries) {
+    llvm::append_range(m_entries, entries);
+  }
+
   void AppendDeclContext(dw_tag_t tag, const char *name) {
     m_entries.push_back(Entry(tag, name));
   }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index b718f98340a70..6891d2fb6f610 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/ADT/Sequence.h"
 #include <optional>
 
 using namespace lldb_private;
@@ -218,6 +219,104 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass(
   m_fallback.GetCompleteObjCClass(class_name, must_be_implementation, callback);
 }
 
+namespace {
+using Entry = llvm::DWARFDebugNames::Entry;
+
+/// If `entry` and all of its parents have an `IDX_parent`, use that information
+/// to build and return a list of at most `max_parents` parent Entries.
+/// `entry` itself is not included in the list.
+/// If any parent does not have an `IDX_parent`, or the Entry data is corrupted,
+/// nullopt is returned.
+static std::optional<llvm::SmallVector<Entry, 4>>
+getParentChain(Entry entry, uint32_t max_parents) {
+  llvm::SmallVector<Entry, 4> parent_entries;
+
+  do {
+    if (!entry.hasParentInformation())
+      return std::nullopt;
+
+    llvm::Expected<std::optional<Entry>> parent = entry.getParentDIEEntry();
+    if (!parent) { // Bad data.
+      consumeError(parent.takeError());
+      return std::nullopt;
+    }
+
+    // Last parent in the chain
+    if (!parent->has_value())
+      break;
+
+    parent_entries.push_back(**parent);
+    entry = **parent;
+  } while (parent_entries.size() < max_parents);
+
+  return parent_entries;
+}
+} // namespace
+
+void DebugNamesDWARFIndex::GetFullyQualifiedType(
+    const DWARFDeclContext &context,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  if (context.GetSize() == 0)
+    return;
+
+  // Fallback: use the base class implementation.
+  auto fallback_impl = [&](const DebugNames::Entry &entry) {
+    return ProcessEntry(entry, [&](DWARFDIE die) {
+      return GetFullyQualifiedTypeImpl(context, die, callback);
+    });
+  };
+
+  llvm::StringRef leaf_name = context[0].name;
+  llvm::SmallVector<llvm::StringRef> parent_names;
+  for (auto idx : llvm::seq<int>(1, context.GetSize()))
+    parent_names.emplace_back(context[idx].name);
+
+  for (const DebugNames::Entry &entry :
+       m_debug_names_up->equal_range(leaf_name)) {
+    if (!isType(entry.tag()))
+      continue;
+
+    // Grab at most one extra parent, subsequent parents are not necessary to
+    // test equality.
+    auto parent_chain = getParentChain(entry, parent_names.size() + 1);
+
+    if (!parent_chain) {
+      if (!fallback_impl(entry))
+        return;
+      continue;
+    }
+
+    if (SameParentChain(parent_names, *parent_chain) &&
+        !ProcessEntry(entry, callback))
+      return;
+  }
+}
+
+bool DebugNamesDWARFIndex::SameParentChain(
+    llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::ArrayRef<DebugNames::Entry> parent_entries) const {
+
+  if (parent_entries.size() != parent_names.size())
+    return false;
+
+  auto SameAsEntryATName = [this](llvm::StringRef name,
+                                  const DebugNames::Entry &entry) {
+    auto maybe_dieoffset = entry.getDIEUnitOffset();
+    if (!maybe_dieoffset)
+      return false;
+    auto die_ref = ToDIERef(entry);
+    if (!die_ref)
+      return false;
+    return name == m_debug_info.PeekDIEName(*die_ref);
+  };
+
+  for (auto [parent_name, parent_entry] :
+       llvm::zip_equal(parent_names, parent_entries))
+    if (!SameAsEntryATName(parent_name, parent_entry))
+      return false;
+  return true;
+}
+
 void DebugNamesDWARFIndex::GetTypes(
     ConstString name, llvm::function_ref<bool(DWARFDIE die)> callback) {
   for (const DebugNames::Entry &entry :
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
index cca0913c4124c..b54dd1162d20a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -42,6 +42,11 @@ class DebugNamesDWARFIndex : public DWARFIndex {
   void GetCompleteObjCClass(
       ConstString class_name, bool must_be_implementation,
       llvm::function_ref<bool(DWARFDIE die)> callback) override;
+
+  /// Uses DWARF5's IDX_parent fields, when available, to speed up this query.
+  void GetFullyQualifiedType(
+      const DWARFDeclContext &context,
+      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetTypes(ConstString name,
                 llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetTypes(const DWARFDeclContext &context,
@@ -83,6 +88,10 @@ class DebugNamesDWARFIndex : public DWARFIndex {
   bool ProcessEntry(const DebugNames::Entry &entry,
                     llvm::function_ref<bool(DWARFDIE die)> callback);
 
+  /// Returns true if `parent_entries` have identical names to `parent_names`.
+  bool SameParentChain(llvm::ArrayRef<llvm::StringRef> parent_names,
+                       llvm::ArrayRef<DebugNames::Entry> parent_entries) const;
+
   static void MaybeLogLookupError(llvm::Error error,
                                   const DebugNames::NameIndex &ni,
                                   llvm::StringRef name);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 26a9502f90aa0..5778f5c887b57 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -360,6 +360,9 @@ class SymbolFileDWARF : public SymbolFileCommon {
 
   Type *ResolveTypeUID(const DIERef &die_ref);
 
+  /// Returns the DWARFIndex for this symbol, if it exists.
+  DWARFIndex *getIndex() { return m_index.get(); }
+
 protected:
   SymbolFileDWARF(const SymbolFileDWARF &) = delete;
   const SymbolFileDWARF &operator=(const SymbolFileDWARF &) = delete;
diff --git a/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt b/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
index 4a37ece124291..d5b0be7ea2a28 100644
--- a/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
+++ b/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
@@ -1,5 +1,6 @@
 add_lldb_unittest(SymbolFileDWARFTests
   DWARFASTParserClangTests.cpp
+  DWARFDebugNamesIndexTest.cpp
   DWARFDIETest.cpp
   DWARFIndexCachingTest.cpp
   DWARFUnitTest.cpp
diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDebugNamesIndexTest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDebugNamesIndexTest.cpp
new file mode 100644
index 0000000000000..fb4160b5be412
--- /dev/null
+++ b/lldb/unittests/SymbolFile/DWARF/DWARFDebugNamesIndexTest.cpp
@@ -0,0 +1,210 @@
+//===-- DWARFDIETest.cpp ----------------------------------------------=---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
+#include "Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h"
+#include "TestingSupport/Symbol/YAMLModuleTester.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::plugin::dwarf;
+using StringRef = llvm::StringRef;
+
+void check_num_matches(DebugNamesDWARFIndex &index, int expected_num_matches,
+                       llvm::ArrayRef<DWARFDeclContext::Entry> ctx_entries) {
+  DWARFDeclContext ctx(ctx_entries);
+  int num_matches = 0;
+  auto increment_matches = [&](DWARFDIE die) {
+    num_matches++;
+    return true;
+  };
+
+  index.GetFullyQualifiedType(ctx, increment_matches);
+  ASSERT_EQ(num_matches, expected_num_matches);
+}
+
+TEST(DWARFDebugNamesIndexTest, FullyQualifiedQueryWithIDXParent) {
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_str:
+    - '1'
+    - '2'
+    - '3'
+  debug_abbrev:
+    - Table:
+        # We intentionally don't nest types in debug_info: if the nesting is not
+        # inferred from debug_names, we want the test to fail.
+        - Code:            0x1
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_yes
+        - Code:            0x2
+          Tag:             DW_TAG_class_type
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+  debug_info:
+    - Version:         4
+      AddrSize:        8
+      Entries:
+        - AbbrCode:        0x1
+        - AbbrCode:        0x2
+          Values:
+            - Value:       0x0 # Name "1"
+        - AbbrCode:        0x2
+          Values:
+            - Value:       0x2 # Name "2"
+        - AbbrCode:        0x2
+          Values:
+            - Value:       0x4 # Name "3"
+        - AbbrCode:        0x0
+  debug_names:
+    Abbreviations:
+    - Code:   0x11
+      Tag: DW_TAG_class_type
+      Indices:
+        - Idx:   DW_IDX_parent
+          Form:  DW_FORM_flag_present
+        - Idx:   DW_IDX_die_offset
+          Form:  DW_FORM_ref4
+    - Code:   0x22
+      Tag: DW_TAG_class_type
+      Indices:
+        - Idx:   DW_IDX_parent
+          Form:  DW_FORM_ref4
+        - Idx:   DW_IDX_die_offset
+          Form:  DW_FORM_ref4
+    Entries:
+    - Name:   0x0  # strp to Name1
+      Code:   0x11
+      Values:
+        - 0xc      # Die offset to entry named "1"
+    - Name:   0x2  # strp to Name2
+      Code:   0x22
+      Values:
+        - 0x0      # Parent = First entry ("1")
+        - 0x11     # Die offset to entry named "1:2"
+    - Name:   0x4  # strp to Name3
+      Code:   0x22
+      Values:
+        - 0x6      # Parent = Second entry ("1::2")
+        - 0x16     # Die offset to entry named "1::2::3"
+    - Name:   0x4  # strp to Name3
+      Code:   0x11
+      Values:
+        - 0x16     # Die offset to entry named "3"
+)";
+
+  YAMLModuleTester t(yamldata);
+  auto *symbol_file =
+      llvm::cast<SymbolFileDWARF>(t.GetModule()->GetSymbolFile());
+  auto *index = static_cast<DebugNamesDWARFIndex *>(symbol_file->getIndex());
+  ASSERT_NE(index, nullptr);
+
+  auto make_entry = [](const char *c) {
+    return DWARFDeclContext::Entry(dwarf::DW_TAG_class_type, c);
+  };
+  check_num_matches(*index, 1, {make_entry("1")});
+  check_num_matches(*index, 1, {make_entry("2"), make_entry("1")});
+  check_num_matches(*index, 1,
+                    {make_entry("3"), make_entry("2"), make_entry("1")});
+  check_num_matches(*index, 0, {make_entry("2")});
+  check_num_matches(*index, 1, {make_entry("3")});
+}
+
+TEST(DWARFDebugNamesIndexTest, FullyQualifiedQueryWithoutIDXParent) {
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_str:
+    - '1'
+    - '2'
+  debug_abbrev:
+    - Table:
+        - Code:            0x1
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_yes
+        - Code:            0x2
+          Tag:             DW_TAG_class_type
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+        - Code:            0x3
+          Tag:             DW_TAG_class_type
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+  debug_info:
+    - Version:         4
+      AddrSize:        8
+      Entries:
+        - AbbrCode:        0x1
+        - AbbrCode:        0x2
+          Values:
+            - Value:       0x0 # Name "1"
+        - AbbrCode:        0x3
+          Values:
+            - Value:       0x2 # Name "2"
+        - AbbrCode:        0x0
+        - AbbrCode:        0x3
+          Values:
+            - Value:       0x2 # Name "2"
+        - AbbrCode:        0x0
+  debug_names:
+    Abbreviations:
+    - Code:   0x1
+      Tag: DW_TAG_class_type
+      Indices:
+        - Idx:   DW_IDX_die_offset
+          Form:  DW_FORM_ref4
+    Entries:
+    - Name:   0x0  # strp to Name1
+      Code:   0x1
+      Values:
+        - 0xc      # Die offset to entry named "1"
+    - Name:   0x2  # strp to Name2
+      Code:   0x1
+      Values:
+        - 0x11     # Die offset to entry named "1::2"
+    - Name:   0x2  # strp to Name2
+      Code:   0x1
+      Values:
+        - 0x17     # Die offset to entry named "2"
+)";
+
+  YAMLModuleTester t(yamldata);
+  auto *symbol_file =
+      llvm::cast<SymbolFileDWARF>(t.GetModule()->GetSymbolFile());
+  auto *index = static_cast<DebugNamesDWARFIndex *>(symbol_file->getIndex());
+  ASSERT_NE(index, nullptr);
+
+  auto make_entry = [](const char *c) {
+    return DWARFDeclContext::Entry(dwarf::DW_TAG_class_type, c);
+  };
+  check_num_matches(*index, 1, {make_entry("1")});
+  check_num_matches(*index, 1, {make_entry("2"), make_entry("1")});
+  check_num_matches(*index, 1, {make_entry("2")});
+}

Comment on lines 283 to 291
if (!parent_chain) {
if (!fallback_impl(entry))
return;
continue;
}

if (SameParentChain(parent_names, *parent_chain) &&
!ProcessEntry(entry, callback))
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use less continues here?

if (parent_chain) {
  if (SameParentChain(parent_names, *parent_chain) &&
      !ProcessEntry(entry, callback))
    return;
} else {
  if (!fallback_impl(entry))
    return;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW as-written does lean a bit towards the LLVM documented style ( https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code ) - but I wouldn't be too fussed either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's ok, I propose we leave it as is because of the guidelines and because I can imagine this method growing in the future to include more code after the referenced line.

/// `entry` itself is not included in the list.
/// If any parent does not have an `IDX_parent`, or the Entry data is corrupted,
/// nullopt is returned.
static std::optional<llvm::SmallVector<Entry, 4>>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't namespace and static redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops.
Errr let me ask the specialist:
image

return std::nullopt;

llvm::Expected<std::optional<Entry>> parent = entry.getParentDIEEntry();
if (!parent) { // Bad data.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment should be on its own line.

continue;
}

if (SameParentChain(parent_names, *parent_chain) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might benefit fro a few high-level comments about what's happening in this loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add something at the top of loop

}
}

bool DebugNamesDWARFIndex::SameParentChain(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any comments that would be useful in this function?

Comment on lines 263 to 267
auto fallback_impl = [&](const DebugNames::Entry &entry) {
return ProcessEntry(entry, [&](DWARFDIE die) {
return GetFullyQualifiedTypeImpl(context, die, callback);
});
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is worth putting in a lambda? Might be simpler to write it inline in the one place it's used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've inlined this one, please have a look

Comment on lines 283 to 291
if (!parent_chain) {
if (!fallback_impl(entry))
return;
continue;
}

if (SameParentChain(parent_names, *parent_chain) &&
!ProcessEntry(entry, callback))
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW as-written does lean a bit towards the LLVM documented style ( https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code ) - but I wouldn't be too fussed either way.

Comment on lines 302 to 312
auto SameAsEntryATName = [this](llvm::StringRef name,
const DebugNames::Entry &entry) {
auto maybe_dieoffset = entry.getDIEUnitOffset();
if (!maybe_dieoffset)
return false;
auto die_ref = ToDIERef(entry);
if (!die_ref)
return false;
return name == m_debug_info.PeekDIEName(*die_ref);
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, a lambda that's only called once doesn't seem to carry its weight - might be simpler to put it in the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not attached to the lambda, but I would like to clarify my original intent with the lambda and then double check if you still prefer the inlined version with that intent in mind.

My intent was not to re-use code, but rather to give name to things and reduce cognitive burden. If we go with the suggestion, this is what we end up with:

  for (auto [parent_name, parent_entry] :
        llvm::zip_equal(parent_names, parent_entries))
    auto maybe_dieoffset = entry.getDIEUnitOffset();
     if (!maybe_dieoffset)
       return false;
     auto die_ref = ToDIERef(entry);
     if (!die_ref)
       return false;
     return name == m_debug_info.PeekDIEName(*die_ref);

A reader needs to go through that entire block and conclude: "Oh, this is just comparing the AT_name with name.

With the current version, the code tells you what it is doing
if (!SameAsEntryATName(parent_name, parent_entry))

With that in mind, if you still prefer the inlined version, I'll be happy to update it!

(the same rationale applies to the other comment about lambdas)

Comment on lines 27 to 32
auto increment_matches = [&](DWARFDIE die) {
num_matches++;
return true;
};

index.GetFullyQualifiedType(ctx, increment_matches);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably inline the lambda at the call site?

index.GetFullyQualifiedType(ctx, [*](DWARFDIE die) {
  num_matches++;
  return true;
});

Comment on lines 204 to 206
auto make_entry = [](const char *c) {
return DWARFDeclContext::Entry(dwarf::DW_TAG_class_type, c);
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's written twice, might be OK to have this as a file-static helper function rather than two separate copies as lambdas?

@felipepiovezan
Copy link
Contributor Author

Addressed all review comments (waiting for a reply on the one comment about inlining a lambda)

@felipepiovezan
Copy link
Contributor Author

@adrian-prantl I've added a log message with the Error itself being logged as well. That said, printing any information specific to the entry itself would require parsing other Form and, given that we already failed to parse one, I figured it would be better not to attempt parsing more while logging.

@adrian-prantl
Copy link
Collaborator

I've added a log message with the Error itself being logged as well

Thanks! I wasted half a day stepping through the DWARF parser recently, thinking I'm debugging a parser bug, when it was really invalid DWARF produced by an older version of dsymutil, so even just the information that there was a parse error is very useful.

@clayborg
Copy link
Collaborator

clayborg commented Feb 1, 2024

I've added a log message with the Error itself being logged as well

Thanks! I wasted half a day stepping through the DWARF parser recently, thinking I'm debugging a parser bug, when it was really invalid DWARF produced by an older version of dsymutil, so even just the information that there was a parse error is very useful.

I always run llvm-dwawfdump --verify as a first step, though it is nice to get more errors emitted in the DWARF log channels

@felipepiovezan
Copy link
Contributor Author

Any other comments here? I would like to land this once the YAML review is unblocked

@felipepiovezan
Copy link
Contributor Author

I will aim to land this sometime tomorrow if there are no further objections

@dwblaikie
Copy link
Collaborator

I will aim to land this sometime tomorrow if there are no further objections

Per the documentation ( https://llvm.org/docs/CodeReview.html#code-review-workflow ), please don't do this. Once it's sent for review, please wait for approval (ping as needed, etc) before landing.

@jimingham
Copy link
Collaborator

I will aim to land this sometime tomorrow if there are no further objections

Per the documentation ( https://llvm.org/docs/CodeReview.html#code-review-workflow ), please don't do this. Once it's sent for review, please wait for approval (ping as needed, etc) before landing.

Felipe already had Adrian's approval, and so far as I can see there were no outstanding comments he hadn't addressed. Surely in that case, you don't have to wait for all the other people selected as reviewers to weigh in do you? That would be pretty unworkable, lots of people assigned as reviewers don't...

Seems to me since this PR already had an approval and no No's, Felipe could have committed it right away. Holding on for late reviewers seems like a courtesy, not a violation of the "you must have an approval" policy.

@dwblaikie
Copy link
Collaborator

I will aim to land this sometime tomorrow if there are no further objections

Per the documentation ( https://llvm.org/docs/CodeReview.html#code-review-workflow ), please don't do this. Once it's sent for review, please wait for approval (ping as needed, etc) before landing.

Felipe already had Adrian's approval, and so far as I can see there were no outstanding comments he hadn't addressed.

Ah, sorry - I scanned up and down but missed it (the UI's not quite as explicit as Phab is on whether a patch has been reviewed - but I could've looked in the top right and seen the check next to Adrian's name).

Yeah, I don't expect multiple reviewer approval unless folks (either the submitter, or other reviewers) ask for it. If the it's approved with some optional comments and the discussion around those aspects has been resolved, fine by me to commit.

@felipepiovezan felipepiovezan merged commit 91f4a84 into llvm:main Feb 13, 2024
@felipepiovezan felipepiovezan deleted the felipe/use_idxparent_in_debugnames_index branch February 13, 2024 21:20
felipepiovezan added a commit to felipepiovezan/llvm-project that referenced this pull request Feb 13, 2024
…query (llvm#79932)

This commit changes DebugNamesDWARFIndex so that it now overrides
`GetFullyQualifiedType` and attempts to use DW_IDX_parent, when
available, to speed up such queries. When this type of information is
not available, the base-class implementation is used.

With this commit, we now achieve the 4x speedups reported in [1].

[1]:
https://discourse.llvm.org/t/rfc-improve-dwarf-5-debug-names-type-lookup-parsing-speed/74151/38

(cherry picked from commit 91f4a84)
kevinfrei pushed a commit to kevinfrei/llvm that referenced this pull request Sep 25, 2024
Summary:
Revert "[lldb][DWARFUnit] Implement PeekDIEName query (llvm#78486)"

This reverts commit 4684507.

Revert "[lldb][DWARFIndex] Use IDX_parent to implement GetFullyQualifiedType query (llvm#79932)"

This reverts commit 91f4a84.

Note(toyang): added back the DWARFDeclContext arrayref constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants