[Lldb-commits] [PATCH] D77970: 2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, clayborg, shafik.
jankratochvil added a project: LLDB.
Herald added subscribers: arphaman, aprantl.

@labath was saying  in D73206 
:

> And /I think/ we could make it interface returns DWARFDies directly


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77970

Files:
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -32,7 +32,7 @@
   DWARFCompileUnit *GetDWOCompileUnitForHash(uint64_t hash);
 
   void GetObjCMethods(lldb_private::ConstString class_name,
-  llvm::function_ref callback) override;
+  llvm::function_ref callback) override;
 
   llvm::Expected
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -97,7 +97,7 @@
 
 void SymbolFileDWARFDwo::GetObjCMethods(
 lldb_private::ConstString class_name,
-llvm::function_ref callback) {
+llvm::function_ref callback) {
   GetBaseSymbolFile().GetObjCMethods(class_name, callback);
 }
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -238,7 +238,7 @@
   GetCompUnitForDWARFCompUnit(DWARFCompileUnit &dwarf_cu);
 
   virtual void GetObjCMethods(lldb_private::ConstString class_name,
-  llvm::function_ref callback);
+  llvm::function_ref callback);
 
   bool Supports_DW_AT_APPLE_objc_complete_type(DWARFUnit *cu);
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1464,7 +1464,7 @@
 }
 
 void SymbolFileDWARF::GetObjCMethods(
-ConstString class_name, llvm::function_ref callback) {
+ConstString class_name, llvm::function_ref callback) {
   m_index->GetObjCMethods(class_name, callback);
 }
 
@@ -2051,17 +2051,11 @@
   uint32_t pruned_idx = original_size;
 
   SymbolContext sc;
-  m_index->GetGlobalVariables(ConstString(basename), [&](DIERef die_ref) {
+  m_index->GetGlobalVariables(ConstString(basename), [&](DWARFDIE die) {
 if (!sc.module_sp)
   sc.module_sp = m_objfile_sp->GetModule();
 assert(sc.module_sp);
 
-DWARFDIE die = GetDIE(die_ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
-  return false;
-}
-
 if (die.Tag() != DW_TAG_variable)
   return false;
 
@@ -2123,17 +2117,11 @@
   const uint32_t original_size = variables.GetSize();
 
   SymbolContext sc;
-  m_index->GetGlobalVariables(regex, [&](DIERef die_ref) {
+  m_index->GetGlobalVariables(regex, [&](DWARFDIE die) {
 if (!sc.module_sp)
   sc.module_sp = m_objfile_sp->GetModule();
 assert(sc.module_sp);
 
-DWARFDIE die = GetDIE(die_ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(die_ref, regex.GetText());
-  return false;
-}
-
 DWARFCompileUnit *dwarf_cu = llvm::dyn_cast(die.GetCU());
 if (!dwarf_cu)
   return false;
@@ -2292,14 +2280,8 @@
 regex.GetText().str().c_str());
   }
 
-  DWARFDebugInfo &info = DebugInfo();
   llvm::DenseSet resolved_dies;
-  m_index->GetFunctions(regex, [&](DIERef ref) {
-DWARFDIE die = info.GetDIE(ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(ref, regex.GetText());
-  return false;
-}
+  m_index->GetFunctions(regex, [&](DWARFDIE die) {
 if (resolved_dies.insert(die.GetDIE()).second)
   ResolveFunction(die, in

[Lldb-commits] [PATCH] D77970: 2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 2 inline comments as done.
jankratochvil added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h:82
+  /// The object will remain valid during the whole call statement:
+  /// Function(name, DIERefCallback({callback, name}));
+  struct DIERefCallbackArgs {

This calling with curly brackets is a bit tricky but I found it the least worst 
option.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h:93
+
+  void ReportInvalidDIERef(DIERef ref, llvm::StringRef name) const;
 };

Invalid `DIERef` is now reported for all Indexes, not just `AppleDWARFIndex`.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77970/new/

https://reviews.llvm.org/D77970



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I am open to different approaches to this problem, this is opening attempt at 
fixing it but I may be missing other interactions.

AFAICT starting the definition of `ToRecord` the way  `CompleteDecl(…)`  does 
when we know `FromRecord` is not defined is just broken for the `RecordDecl` 
case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue 
debugging `llc`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78000/new/

https://reviews.llvm.org/D78000



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.

I am open to different approaches to this problem, this is opening attempt at 
fixing it but I may be missing other interactions.

AFAICT starting the definition of `ToRecord` the way  `CompleteDecl(…)`  does 
when we know `FromRecord` is not defined is just broken for the `RecordDecl` 
case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue 
debugging `llc`.


In `ImportContext(…)` we may call into `CompleteDecl(…)` which if `FromRecrord` 
is not defined will start the definition of a `ToRecord` but from what I can 
tell at least one of the paths though here don't ensure we complete the 
definition. 
For a `RecordDecl` this can be problematic since this means we won’t import 
base classes and we won’t have any of the methods or types we inherit from 
these bases.

One such path is through `VisitTypedefNameDecl(…)` which is exercised by the 
reproducer.

For LLDB expression parsing this results in expressions failing but sometimes 
succeeding in subsequent attempts e.g.:

  (lldb) p BB->dump()
  error: no member named 'dump' in 'B'
  (lldb) p BB->dump()
  (lldb) 

This happens because the first time through `FromRecord` is not defined but in 
subsequent attempts through it may be.


https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp


Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
   FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
 return std::move(Err);
 } else {
-  CompleteDecl(ToRecord);
+  // If FromRecord is not defined we need to force it to be.
+  // Simply calling CompleteDecl(...) for a RecordDecl will break some 
cases
+  // it will start the definition but we never finish it.
+  // If a RecordDecl has base classes they won't be imported and we will
+  // be missing anything that we inherit from those bases.
+  if (FromRecord->hasExternalLexicalStorage() &&
+  !FromRecord->isCompleteDefinition())
+FromRecord->getASTContext().getExternalSource()->CompleteType(
+FromRecord);
+
+  if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+  FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+return std::move(Err);
 }
   } else if (auto *ToEnum = dyn_cast(ToDC)) {
 auto *FromEnum = cast(FromDC);


Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {