Skip to content
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

[lldb][DWARF] Do not complete type from declaration die. #91799

Merged
merged 2 commits into from
May 10, 2024

Conversation

ZequanWu
Copy link
Contributor

Fix the problem: #90663 (comment) by enhancing a double-check for #90663

@ZequanWu ZequanWu requested a review from clayborg May 10, 2024 20:01
@ZequanWu ZequanWu requested a review from JDevlieghere as a code owner May 10, 2024 20:01
@llvmbot llvmbot added the lldb label May 10, 2024
@llvmbot
Copy link
Member

llvmbot commented May 10, 2024

@llvm/pr-subscribers-lldb

Author: Zequan Wu (ZequanWu)

Changes

Fix the problem: #90663 (comment) by enhancing a double-check for #90663


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

1 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+4-1)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index e0b1b430b266f..315ba4cc0ccdf 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2343,7 +2343,10 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die,
   // clang::ExternalASTSource queries for this type.
   m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), false);
 
-  if (!die)
+  ParsedDWARFTypeAttributes attrs(die);
+  bool is_forward_declaration = IsForwardDeclaration(
+      die, attrs, SymbolFileDWARF::GetLanguage(*die.GetCU()));
+  if (!die || is_forward_declaration)
     return false;
 
   const dw_tag_t tag = die.Tag();

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

Let me verify this works. I would also like this to fix:

bool DebugNamesDWARFIndex::ProcessEntry(
    const DebugNames::Entry &entry,
    llvm::function_ref<bool(DWARFDIE die)> callback) {
  std::optional<DIERef> ref = ToDIERef(entry);
  if (!ref)
    return true;
  SymbolFileDWARF &dwarf = *llvm::cast<SymbolFileDWARF>(
      m_module.GetSymbolFile()->GetBackingSymbolFile());
  DWARFDIE die = dwarf.GetDIE(*ref);
  if (!die)
    return true;
  // Watch out for forward declarations that appear in the .debug_names tables
  // only due to being there for a DW_IDX_parent.
  if (die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0))
    return true;
  return callback(die);
}

This adds:

  // Watch out for forward declarations that appear in the .debug_names tables
  // only due to being there for a DW_IDX_parent.
  if (die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0))
    return true;

To the above function to ensure we don't waste any time trying to parse any DIE information for forward declaration when using .debug_names. This will cause a TON of churn on our DWARF parser and cause us to pull in and parse way too much.

@ZequanWu
Copy link
Contributor Author

To the above function to ensure we don't waste any time trying to parse any DIE information for forward declaration when using .debug_names. This will cause a TON of churn on our DWARF parser and cause us to pull in and parse way too much.

That sounds like a better fix.

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

This does fix things on your side. I will take care of a new PR for not searching all definition DIEs from the .debug_names.

@ZequanWu ZequanWu merged commit a7eff59 into llvm:main May 10, 2024
4 checks passed
jimingham added a commit that referenced this pull request May 14, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 23, 2024
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.

3 participants