[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

2020-01-22 Thread Jan Kratochvil via Phabricator via lldb-commits 
jankratochvil abandoned this revision.
jankratochvil marked an inline comment as done.
jankratochvil added a comment.

I tried to use `DIERef` instead of `DWARFDIE` everywhere as @labath does not 
like to increase `DWARFDIE` size from 16 bytes to 24 bytes.  But that is not 
really feasible. For `DIERef` one needs to also carry `SymbolFileDWARF *` along 
and also resolving `DIERef` into `DWARFDIE` is slow as it has to bisect CUs 
from the DIE offset. I will post a different proposal how to implement DWZ.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70647



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

[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

2019-11-26 Thread Jan Kratochvil via Phabricator via lldb-commits 
jankratochvil planned changes to this revision.
jankratochvil marked 2 inline comments as done.
jankratochvil added inline comments.



Comment at: lldb/include/lldb/Symbol/TypeSystem.h:108-110
+  virtual DWARFASTParser *GetDWARFParser(SymbolFileDWARF ) {
+return nullptr;
+  }

labath wrote:
> This part looks pretty dodgy. I'd like to avoid introducing plugin references 
> in non-plugin code. It looks like this class isn't particularly clean already 
> (DWARFDIE forward decl), but this seems to make the problem much worse.
> 
> Since this class already contains a `SymbolFile` pointer, maybe we could 
> create some kind of a ast-parser constructing method on the SymbolFile class 
> to avoid mentioning the SymbolFileDWARF directly.
> 
> If I was doing this, I'd probably try to take this one step further and merge 
> the `GetDWARFParser`/`GetPDBParser` methods (whose only calls are in the 
> relevant symbol file plugins) into a single `GetASTParser` method and do 
> appropriate casts in the plugins themselves.
TBH I did not do the described refactoring, I just used the existing 
`m_sym_file`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70647



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

[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

2019-11-26 Thread Jan Kratochvil via Phabricator via lldb-commits 
jankratochvil updated this revision to Diff 231124.
jankratochvil added a comment.

Remove that dodgy new parameter `SymbolFileDWARF `.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70647

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  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
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -48,7 +48,7 @@
 TEST_F(DWARFASTParserClangTests,
EnsureAllDIEsInDeclContextHaveBeenParsedParsesOnlyMatchingEntries) {
   ClangASTContext ast_ctx;
-  DWARFASTParserClangStub ast_parser(ast_ctx);
+  DWARFASTParserClangStub ast_parser(ast_ctx, *(SymbolFileDWARF *)1LL);
 
   DWARFUnit *unit = nullptr;
   std::vector dies = {DWARFDIE(unit, (DWARFDebugInfoEntry *)1LL),
Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -95,6 +95,7 @@
 
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
 #include "Plugins/SymbolFile/DWARF/DWARFASTParserClang.h"
+#include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
 #include "Plugins/SymbolFile/PDB/PDBASTParser.h"
 
 #include 
@@ -9812,8 +9813,11 @@
 }
 
 DWARFASTParser *ClangASTContext::GetDWARFParser() {
-  if (!m_dwarf_ast_parser_up)
-m_dwarf_ast_parser_up.reset(new DWARFASTParserClang(*this));
+  if (!m_dwarf_ast_parser_up) {
+SymbolFileDWARF *dwarf = llvm::dyn_cast(m_sym_file);
+if (dwarf)
+  m_dwarf_ast_parser_up.reset(new DWARFASTParserClang(*this, *dwarf));
+  }
   return m_dwarf_ast_parser_up.get();
 }
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -44,8 +44,7 @@
   llvm::Expected
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  DWARFDIE
-  GetDIE(const DIERef _ref) override;
+  DWARFUnit *GetUnit(DIERef die_ref) override;
 
   std::unique_ptr
   GetDwoSymbolFileForCompileUnit(DWARFUnit _cu,
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -147,9 +147,8 @@
   return GetBaseSymbolFile().GetTypeSystemForLanguage(language);
 }
 
-DWARFDIE
-SymbolFileDWARFDwo::GetDIE(const DIERef _ref) {
+DWARFUnit *SymbolFileDWARFDwo::GetUnit(DIERef die_ref) {
   if (*die_ref.dwo_num() == GetDwoNum())
-return DebugInfo()->GetDIE(die_ref);
-  return GetBaseSymbolFile().GetDIE(die_ref);
+return DebugInfo()->GetUnit(die_ref);
+  return GetBaseSymbolFile().GetUnit(die_ref);
 }
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -275,7 +275,9 @@
   return m_external_type_modules;
   }
 
-  virtual DWARFDIE GetDIE(const DIERef _ref);
+  virtual DWARFUnit *GetUnit(DIERef die_ref);
+
+  DWARFDIE GetDIE(DIERef die_ref);
 
   DWARFDIE GetDIE(lldb::user_id_t uid);
 
@@ -303,9 +305,9 @@
   /// If this is a DWARF object with a single CU, return its DW_AT_dwo_id.
   llvm::Optional GetDWOId();
 
-  static bool
+  bool
   DIEInDeclContext(const lldb_private::CompilerDeclContext *parent_decl_ctx,
-   const DWARFDIE );
+   const DWARFDIE , DIERef die_ref);
 
   std::vector>
   ParseCallEdgesInFunction(UserID func_id) override;
@@ -318,26 +320,29 @@
 
   lldb_private::FileSpec GetFile(DWARFUnit , size_t file_idx);
 
-  static llvm::Expected
-  GetTypeSystem(DWARFUnit );
+  lldb::LanguageType GetLanguage(DIERef die_ref);
+
+  DWARFASTParser *GetDWARFParser(lldb::LanguageType language);
+
+  DWARFASTParser *GetDWARFParser(DIERef die_ref);
 
-  static DWARFASTParser *GetDWARFParser(DWARFUnit );
+  DWARFASTParser *GetDWARFParser(lldb::user_id_t die_uid,
+

[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

2019-11-25 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/include/lldb/Symbol/TypeSystem.h:108-110
+  virtual DWARFASTParser *GetDWARFParser(SymbolFileDWARF ) {
+return nullptr;
+  }

This part looks pretty dodgy. I'd like to avoid introducing plugin references 
in non-plugin code. It looks like this class isn't particularly clean already 
(DWARFDIE forward decl), but this seems to make the problem much worse.

Since this class already contains a `SymbolFile` pointer, maybe we could create 
some kind of a ast-parser constructing method on the SymbolFile class to avoid 
mentioning the SymbolFileDWARF directly.

If I was doing this, I'd probably try to take this one step further and merge 
the `GetDWARFParser`/`GetPDBParser` methods (whose only calls are in the 
relevant symbol file plugins) into a single `GetASTParser` method and do 
appropriate casts in the plugins themselves.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70647



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

[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

2019-11-24 Thread Jan Kratochvil via Phabricator via lldb-commits 
jankratochvil created this revision.
jankratochvil added a reviewer: labath.
jankratochvil added a project: LLDB.
Herald added subscribers: arphaman, aprantl.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: shafik.
jankratochvil added a parent revision: D70646: RFC 2/3: Move non-DWARF code: 
`DWARFUnit` -> `SymbolFileDWARF`.

This patchset is removing dependency on `DWARFDIE`'s `DWARFUnit *m_cu` for 
stuff which is unavailable in `DW_TAG_partial_unit` without accessing its 
parent `DW_TAG_compile_unit` (which includes `DW_TAG_partial_unit` by 
`DW_AT_import`).
This patch has no regressions but to really make it usable for DWZ one still 
needs to adjust `DWARFBaseDIE::GetDIERef()` as one cannot generate `DIERef` 
(containing main unit idenitifier for DWZ) purely from `DWARFDIE` (not 
containing `DWARFUnit *main_cu`).
Refactored `DWARFBaseDIE::GetDIERef()` (adding some parameter) would then need 
adjustment also of functions calling it directly - `DWARFBaseDIE::GetID()` and 
`SymbolFileDWARF::GetUID(const DWARFBaseDIE )` (and many functions calling 
these).
I do not plan to check it in yet. I plan to finish it for use by DWZ patchset 
first (D40474  et al.). Asking whether this is 
a good way forward.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70647

Files:
  lldb/include/lldb/Symbol/ClangASTContext.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  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
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -48,7 +48,7 @@
 TEST_F(DWARFASTParserClangTests,
EnsureAllDIEsInDeclContextHaveBeenParsedParsesOnlyMatchingEntries) {
   ClangASTContext ast_ctx;
-  DWARFASTParserClangStub ast_parser(ast_ctx);
+  DWARFASTParserClangStub ast_parser(ast_ctx, *(SymbolFileDWARF *)1LL);
 
   DWARFUnit *unit = nullptr;
   std::vector dies = {DWARFDIE(unit, (DWARFDebugInfoEntry *)1LL),
Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -9811,9 +9811,9 @@
   }
 }
 
-DWARFASTParser *ClangASTContext::GetDWARFParser() {
+DWARFASTParser *ClangASTContext::GetDWARFParser(SymbolFileDWARF ) {
   if (!m_dwarf_ast_parser_up)
-m_dwarf_ast_parser_up.reset(new DWARFASTParserClang(*this));
+m_dwarf_ast_parser_up.reset(new DWARFASTParserClang(*this, dwarf));
   return m_dwarf_ast_parser_up.get();
 }
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -44,8 +44,7 @@
   llvm::Expected
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  DWARFDIE
-  GetDIE(const DIERef _ref) override;
+  DWARFUnit *GetUnit(DIERef die_ref) override;
 
   std::unique_ptr
   GetDwoSymbolFileForCompileUnit(DWARFUnit _cu,
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -147,9 +147,8 @@
   return GetBaseSymbolFile().GetTypeSystemForLanguage(language);
 }
 
-DWARFDIE
-SymbolFileDWARFDwo::GetDIE(const DIERef _ref) {
+DWARFUnit *SymbolFileDWARFDwo::GetUnit(DIERef die_ref) {
   if (*die_ref.dwo_num() == GetDwoNum())
-return DebugInfo()->GetDIE(die_ref);
-  return GetBaseSymbolFile().GetDIE(die_ref);
+return DebugInfo()->GetUnit(die_ref);
+  return GetBaseSymbolFile().GetUnit(die_ref);
 }
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -275,7 +275,9 @@
   return m_external_type_modules;
   }
 
-  virtual DWARFDIE GetDIE(const DIERef _ref);
+  virtual DWARFUnit *GetUnit(DIERef die_ref);
+
+  DWARFDIE GetDIE(DIERef die_ref);
 
   DWARFDIE GetDIE(lldb::user_id_t uid);