labath added inline comments.
================
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:283-285
+ /// Return a hash for this symbol file, such as a dwo_id.
+ virtual llvm::Optional<uint64_t> GetSymbolHash() { return {}; }
+
----------------
Can we remove this and put a cast in
`SymbolFileDWARF::UpdateExternalModuleListIfNeeded` instead? It looks like that
function should check that it has really found a dwarf file (and not a pdb for
instance) anyway...
================
Comment at: lldb/source/Host/common/Host.cpp:304
+ if (log)
+ log->VAPrintf(format, args);
}
----------------
Are you sure it's legal to recycle the `va_list` this way? Should you maybe
re-initialize it?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1570
+static uint64_t GetDWOId(DWARFCompileUnit &dwarf_cu,
+ DWARFDebugInfoEntry cu_die) {
+ uint64_t dwo_id =
----------------
`DWARFDebugInfoEntry` objects assume that they are living in one big vector and
do pointer arithmetic on their `this` pointers. I don't think it's wise to copy
them even if that happens to work in this case...
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1579
+llvm::Optional<uint64_t> SymbolFileDWARF::GetSymbolHash() {
+ if (auto comp_unit = GetCompileUnitAtIndex(0)) {
+ if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null<DWARFCompileUnit>(
----------------
With this implementation, the function will return the dwo id of the first
skeleton unit in the main module in the dwo scenario. I think this is very
unexpected and not very useful. I think this should check that the file
contains a just a single compile unit, at least.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1724-1725
+
+ // Verify the DWO hash.
+ // FIXME: Technically "0" is a valid hash.
+ uint64_t dwo_id = GetDWOId(*dwarf_cu, *die.GetDIE());
----------------
Maybe you could have GetDWOId return Optional<uint>. That way, the FIXME will
be inside that function, and not in all of it's callers.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70272/new/
https://reviews.llvm.org/D70272
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
