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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits