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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:22-24
+/// - file_index: identifies the dwo file in the Module. If this field is not
+/// set,
+///   the DIERef references the main, dwo or .o file.
----------------



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:32
   enum Section : uint8_t { DebugInfo, DebugTypes };
-
-  DIERef(std::optional<uint32_t> dwo_num, Section section,
+  // Making constructors protected to limit where DIERef is used directly.
+  DIERef(std::optional<uint32_t> file_index, Section section,
----------------
labath wrote:
> they're not actually protected
There were for a bit to control access to this, but in reality we ended up friending too many classes and then ran into issues with the DIERefTest stuff, so we decided to make them public again. We can remove this comment.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp:75
+  if (ref)
+    return DIERef(*GetDWARF(), *ref).get_id();
+
----------------
labath wrote:
> Is this the only call site of the `DIERef(SymbolFileDWARF&)` constructor?
> 
> If so, and if we make it such that `DWARFBaseDIE::GetDIERef` returns the fully filled in DIERef, then this function can just call get_id() on the result, and we can delete that constructor.
This line doesn't make sense. If we got a valid DIERef back from GetDIERef(), then we just return that as it would have used the SymbolFileDWARF to fill everything in already. So we might not need that extra constructor if this is the only place as Pavel suggested.


================
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());
 }
----------------
labath wrote:
> clayborg wrote:
> > 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.
> +1
Ok. So lets do this - change "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid)" to just be:

```
DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid) {
  return GetDIE(DIERef(uid));
}
```
And then change the current "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid)" to be the one that does all of the work:

```
DWARFDIE SymbolFileDWARF::GetDIE(DIERef die_ref) {
  std::optional<uint32_t> file_index = die_ref.file_index();
  if (file_index) {
    if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile())
      symbol_file = debug_map->GetSymbolFileByOSOIndex(*file_index); // OSO case
    else if (*file_index == DIERef::k_file_index_mask)
      symbol_file = m_dwp_symfile.get(); // DWP case
    else
      symbol_file = this->DebugInfo()
                        .GetUnitAtIndex(*die_ref.file_index())
                        ->GetDwoSymbolFile(); // DWO case
  } else if (die_ref.die_offset() == DW_INVALID_OFFSET) {
    symbol_file = nullptr;
  } else {
    symbol_file = this;
  }

  if (symbol_file)
    return symbol_file->GetDIE(die_ref);

  return DWARFDIE();
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618