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

Tue Oct 9 14:29:35 PDT 2018

lemo added a comment.

Yay! Nice to see this is coming along, thanks Zach!

Looks good to me, a few comments inline.

================
Comment at: lldb/lit/SymbolFile/NativePDB/simple-breakpoints.cpp:11
+// CHECK: Current executable set to '{{.*}}simple-breakpoints.cpp.tmp.exe' (x86_64).
+// CHECK: (lldb) break set -n main
+// CHECK: Breakpoint 1: where = simple-breakpoints.cpp.tmp.exe`main + 23 at simple-breakpoints.cpp:15, address = 0x0000000140001017
----------------
just an idea: add a negative test as well? (break -s n non_existent_function) 

================
Comment at: lldb/lit/SymbolFile/NativePDB/simple-breakpoints.cpp:12
+// CHECK: (lldb) break set -n main
+// CHECK: Breakpoint 1: where = simple-breakpoints.cpp.tmp.exe`main + 23 at simple-breakpoints.cpp:15, address = 0x0000000140001017
+
----------------
Is the offset and address going to be stable enough to rely on it for testing?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt:12
+    lldbSymbol
+	lldbUtility
+  LINK_COMPONENTS
----------------
nit: fix indentation 

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp:84
+
+  if (item.m_obj_name && item.m_compile_opts)
+    return;
----------------
what's the meaning of this check?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp:103
+    }
+    if (++found >= 3)
+      break;
----------------
is the intention to count unique kinds? can we have duplicates?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp:121
+CompileUnitIndex::GetOrCreateCompiland(PdbSymUid compiland_uid) {
+  auto result = m_comp_units.try_emplace(compiland_uid.toOpaqueId(), nullptr);
+  if (!result.second)
----------------
is C++ 17 available? structured bindings can make this more readable:

const auto& [it, inserted] = ...;
if (!inserted)
  ...

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h:34
+
+struct CompilandIndexItem {
+  CompilandIndexItem(PdbSymUid uid,
----------------
add a brief description of the struct?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h:71
+
+class CompileUnitIndex {
+  PdbIndex &m_index;
----------------
ditto: class description comment?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:34
+PdbIndex::create(std::unique_ptr<llvm::pdb::PDBFile> file) {
+  assert(file);
+  auto ExpectedDBI = file->getPDBDbiStream();
----------------
lldbassert() ? 

(same comment for the rest of assert()s in the CR)

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:37-38
+  if (!ExpectedDBI) {
+    llvm::consumeError(ExpectedDBI.takeError());
+    return nullptr;
+  }
----------------
zturner wrote:
> labath wrote:
> > Maybe propagate this error to the caller and have the caller log the error when ignoring it?
> These are a bit tricky.  In all cases, there is only possible thing the caller can do, which is to give up.  I don't think the caller particularly cares about the details of the error that occurred, the only way to handle it is to say "we're not going to use this plugin".  So I think it's fine to consume the error and just return a generic failure here.
how do you feel about adding case-specific logging?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:122
+void PdbIndex::ParseSectionContribs() {
+  struct Visitor : public ISectionContribVisitor {
+    Visitor(PdbIndex &ctx, llvm::IntervalMap<uint64_t, uint16_t> &imap)
----------------
class? (not just for the notation clarity, but there's no reason to make the visitor members public, right?

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

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:151
+    uint32_t offset;
+    std::tie(segment, offset) = GetSegmentAndOffset(*iter);
+    lldb::addr_t va = MakeVirtualAddress(segment, offset);
----------------
C++ 17 structured bindings?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:186
+  // In order to find this, we use the upper_bound of the key value which would
+  // be the first symbol whose starting address is higher than the element we're
+  // searching for.
----------------
but is that what we really want? shouldn't we be looking for the "inner-most" range which contains the address?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:191
+
+  for (auto iter = cci.m_symbols_by_va.begin(); iter != ub; ++iter) {
+    const PdbCuSymId &cu_sym_id = iter->second.asCuSym();
----------------
Ok, I see what we're doing. Wouldn't make more sense to iterate backward? (efficiency wise and more importantly to locate the most "inner" range, right?)

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.h:58
+  /// type records.
+  llvm::pdb::DbiStream *m_dbi;
+
----------------
initialize these raw pointers in-class (NSDMI) ?

llvm::pdb::DbiStream *m_dbi = nullptr;

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.h:95
+
+  CompileUnitIndex m_cus;
+
----------------
comment?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:162
+    uint64_t result;
+    ::memcpy(&result, &m_uid, sizeof(m_uid));
+    return result;
----------------
should't the last argument be sizeof(result) ? 

also, what is the meaning of this opaque id?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp:124
+template <typename RecordT>
+static std::pair<uint16_t, uint32_t> GetSegmentAndOffset(const CVSymbol &sym) {
+  RecordT record = createRecord<RecordT>(sym);
----------------
I'd prefer a dedicated SegmentAndOffset struct rather than the generic pair<>, what do you think?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:86
+  auto File = llvm::make_unique<PDBFile>(Path, std::move(Stream), Allocator);
+  if (auto EC = File->parseFileHeaders())
+    return nullptr;
----------------
add logging?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:116
+  if (ec)
+    return nullptr;
+
----------------
logging, please?

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:128
+    llvm::consumeError(expected_info.takeError());
+    return nullptr;
+  }
----------------
ditto: logging? (very valuable for someone trying to troubleshoot symbol loading issues w/o debugging lldb)

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:131
+  llvm::codeview::GUID guid;
+  memcpy(&guid, pdb_info->PDB70.Signature, 16);
+
----------------
sizeof(guid) ?

https://reviews.llvm.org/D53002