Skip to content

[lldb][DWARFASTParserClang][NFC] Remove unused parameter to CompleteRecordType #120456

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

Merged

Conversation

Michael137
Copy link
Member

Became unused since the recent
#110648

@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Became unused since the recent
#110648


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

2 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+1-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (-1)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 37c1132c1c9f9a..7c57484f56a7d2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2066,7 +2066,6 @@ bool DWARFASTParserClang::ParseTemplateParameterInfos(
 }
 
 bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
-                                             lldb_private::Type *type,
                                              const CompilerType &clang_type) {
   const dw_tag_t tag = die.Tag();
   SymbolFileDWARF *dwarf = die.GetDWARF();
@@ -2203,7 +2202,7 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(
   case DW_TAG_structure_type:
   case DW_TAG_union_type:
   case DW_TAG_class_type:
-    CompleteRecordType(die, type, clang_type);
+    CompleteRecordType(die, clang_type);
     break;
   case DW_TAG_enumeration_type:
     CompleteEnumType(die, type, clang_type);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 6c3aac6d452e49..55f8e38d7486d6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -421,7 +421,6 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
       const lldb_private::CompilerType &class_clang_type);
 
   bool CompleteRecordType(const lldb_private::plugin::dwarf::DWARFDIE &die,
-                          lldb_private::Type *type,
                           const lldb_private::CompilerType &clang_type);
   bool CompleteEnumType(const lldb_private::plugin::dwarf::DWARFDIE &die,
                         lldb_private::Type *type,

Michael137 added a commit to Michael137/llvm-project that referenced this pull request Dec 19, 2024
…DIE's SymbolFile

The problem here manifests as follows:
1. We are stopped in main.o, so the first `ParseTypeFromDWARF`
on `FooImpl<char>` gets called on `main.o`'s SymbolFile. This
adds a mapping from *declaration die* -> `TypeSP` into `main.o`'s
`GetDIEToType` map.
2. We then `CompleteType(FooImpl<char>)`. Depending on the order of
entries in the debug-map, this might call `CompleteType` on `lib.o`'s
SymbolFile. In which case, `GetDIEToType().lookup(decl_die)` will
return a `nullptr`. This is already a bit iffy because surrounding code
used to assume we don't call `CompleteTypeFromDWARF` with a `nullptr` `Type*`.
This used to be more of an issue when `CompleteRecordType` actually made use of
the `Type*` parameter (see llvm#120456).
3. While in `CompleteTypeFromDWARF`, we call `ParseTypeFromDWARF` again.
This will parse the member function `FooImpl::Create` and its return
type which is a typedef to `FooImpl*`. But now we're inside `lib.o`'s SymbolFile,
so we call it on the definition DIE. In step (2) we just inserted a `nullptr` into
`GetDIEToType` for the definition DIE, so we trivially return a `nullptr` from
`ParseTypeFromDWARF`. Instead of reporting back this parse failure to
the user LLDB trucks on and marks `FooImpl::Ref` to be `void*`.

This test-case will trigger an assert in `TypeSystemClang::VerifyDecl`
even if we just `frame var` (but only in debug-builds). In release
builds where this function is a no-op, we'll create an incorrect Clang
AST node for the `Ref` typedef.
Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

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

Nice.

@Michael137 Michael137 merged commit 6fd267d into llvm:main Dec 20, 2024
9 checks passed
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Dec 23, 2024
…ecordType (llvm#120456)

Became unused since the recent
llvm#110648

(cherry picked from commit 6fd267d)
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