[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Very clean patch now, just a few nits about asserts!



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp:129
     DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
-  lldbassert(!cu.GetSymbolFileDWARF().GetDwoNum());
+  lldbassert(!cu.GetSymbolFileDWARF().GetFileIndex());
   uint64_t cu_offset = cu.GetOffset();
----------------
Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:402
     DWARFUnit &unit, llvm::function_ref<bool(DWARFDIE die)> callback) {
-  lldbassert(!unit.GetSymbolFileDWARF().GetDwoNum());
+  lldbassert(!unit.GetSymbolFileDWARF().GetFileIndex());
   Index();
----------------
Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp:53
     DWARFUnit &s_unit, llvm::function_ref<bool(DIERef ref)> callback) const {
-  lldbassert(!s_unit.GetSymbolFileDWARF().GetDwoNum());
+  lldbassert(!s_unit.GetSymbolFileDWARF().GetFileIndex());
   const DWARFUnit &ns_unit = s_unit.GetNonSkeletonUnit();
----------------
Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1405
+DWARFDIE
+SymbolFileDWARF::GetDIE(lldb::user_id_t uid) {
   // This method can be called without going through the symbol vendor so we
----------------
Pavel: note we now really on "SymbolFileDWARF::GetDie(user_id_t)" to be the one source of truth when finding a DIE. We could make "SymbolFileDWARF:GetDie(DIERef ref)" be the one source of truth and then have "SymbolFileDWARF::GetDie(user_id_t)" just create a local DIERef and then call "SymbolFileDWARF:GetDie(DIERef ref)" if that would be cleaner.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1682
 SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
-  if (die_ref.dwo_num()) {
-    SymbolFileDWARF *dwarf = *die_ref.dwo_num() == 0x3fffffff
-                                 ? m_dwp_symfile.get()
-                                 : this->DebugInfo()
-                                       .GetUnitAtIndex(*die_ref.dwo_num())
-                                       ->GetDwoSymbolFile();
-    return dwarf->DebugInfo().GetDIE(die_ref);
-  }
-
-  return DebugInfo().GetDIE(die_ref);
+  return GetDIE(die_ref.get_id());
 }
----------------
Pavel: note we now really on "SymbolFileDWARF::GetDie(user_id_t)" to be the one source of truth when finding a DIE. We could make "SymbolFileDWARF:GetDie(DIERef ref)" be the one source of truth and then have "SymbolFileDWARF::GetDie(user_id_t)" just create a local DIERef and then call "SymbolFileDWARF:GetDie(DIERef ref)" if that would be cleaner.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:570-572
+  /// If this DWARF file a .DWO file or a DWARF .o file on mac when
+  /// no dSYM file is being used, this file index will be set to a
+  /// valid value that can be used in DIERef objects.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618