[PATCH] D53002: Create a new symbol file plugin for cross-platform PDB symbolization

[Stella Stamenova via Phabricator via llvm-commits]

stella.stamenova added a comment.

I looked through the code and it mostly looks good. I had a couple of nits and a couple of actual questions (especially about asserts). I agree with Aleksandr the comments are going to make a big difference in maintenance/improvements going forward (until they eventually get out of sync with the code...).



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:149
+
+    uint16_t segment;
+    uint32_t offset;
----------------
Nit: I've noticed you initialized the variables some places, but not all places. I'm all for initializing always.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:153
+    lldb::addr_t va = MakeVirtualAddress(segment, offset);
+    assert(cci.m_symbols_by_va.count(va) == 0);
+
----------------
If the assert fails, does this mean that the symbol map won't be built correctly? Should it be an actual error instead of just an assert?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:131
+    // S_PROCREF symbols are 1-based
+    assert(sym.Module > 0);
+    return makeCompilandId(sym.Module - 1);
----------------
This assert looks like it really should be an error instead of just an assert.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:341
+    const lldb_private::SymbolContext &sc) {
+  // What fields should I expect to be filled out on the SymbolContext?  Is it
+  // safe to assume that `sc.comp_unit` is valid?
----------------
Did you mean to leave this comment here? Maybe with a FIXME?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:592
+                                   lldb_private::SymbolContextList &sc_list) {
+  return 0;
+}
----------------
It looks like a couple more functions should have FIXME comments as they are not yet implemented.


https://reviews.llvm.org/D53002