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

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo marked 16 inline comments as done.
ayermolo added inline comments.


================
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,
----------------
clayborg wrote:
> 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.
Oops, forgot to remove. For one of the internal revisions I experimented with making them protected.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:198
               "attach the file at the start of this error message",
-              m_offset, (unsigned)form);
+              (uint64_t)m_offset, (unsigned)form);
           *offset_ptr = m_offset;
----------------
clayborg wrote:
> Needed? Same as above
m_offset is is bit field now, so without it clang produces error.


================
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());
 }
----------------
clayborg wrote:
> 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();
> }
> ```
> 
ah, yes, great suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618