[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.

[Pavel Labath via Phabricator via lldb-commits]

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