[PATCH] D53002: Create a new symbol file plugin for cross-platform PDB symbolization
Stella Stamenova via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 9 08:30:18 PDT 2018
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
More information about the llvm-commits
mailing list