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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
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();
----------------
lemo wrote:
> zturner wrote:
> > lemo wrote:
> > > zturner wrote:
> > > > lemo wrote:
> > > > > lldbassert() ? 
> > > > > 
> > > > > (same comment for the rest of assert()s in the CR)
> > > > `lldbassert` is nice to validate things that you expect should happen but want to log an error if the condition is not held.  `assert` is better when you want to guarantee invariants of your API.  In this case I use `assert` because the precondition of this function is "`file` is non-null".  `lldbassert` wouldn't help here, because the very next line of code is to just dereference the pointer and we'd crash anyway.
> > > I believe that lldbassert has been fixed (lldb_private::lldb_assert() unconditionally ends with calling abort())
> > Hmm, that's new.  So it looks to me like the only difference between `assert` and `lldbassert` is that you can't turn `lldbassert` off in release mode.  Do I have that right?  Do we care about that?
> my main concern is consistency - having multiple flavors of assert can be confusing (one should not have to think too much about the subtleties around the various flavors of "assert")
> 
> so my 2c: use lldbassert() in the LLDB codebase
It has been "fixed", though I'm not sure if people who were advocating for "lldbassert" actually saw the "fix".


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:37-38
+  if (!ExpectedDBI) {
+    llvm::consumeError(ExpectedDBI.takeError());
+    return nullptr;
+  }
----------------
zturner wrote:
> lemo wrote:
> > 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?
> Logging is possible.  But I think there's such a thing as too verbose logging.  There's a balance between code simplicity and providing useful information in the log file.  This code is called at a stage when LLDB still hasn't even figured out that the PDB symbol file plugin is ultimately the right plugin for the job.  It's *asking* us if we can handle it.  So if we log an error and say "Invalid file, PDB doesn't have a DBI stream" it seems too verbose in my opinion.  Imagine if LLDB tried the DWARF plugin first (because it just goes down the list of looking for anyone who claims they can handle this file), and DWARF logged an error saying "Invalid DWARF debug information format", but it's not even DWARF in the first place.
> 
> I don't know what the best thing to do here is other than to say "we can't handle this", for which a single `nullptr` return value is sufficient.
My main reason for suggesting this was simplicity. Having this function return an error means you can turn a bunch of:
```
if (!Foo) {
  consumeError(Foo.takeError());
  return nullptr;
}
```
into
```
if (!Foo)
  return Foo.takeError();
```

Then, in the caller (which is in a better position to determine what to do with the error imho), you can discard the error with a single consumeError call (or a log message, or whatever).


================
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);
+
----------------
zturner wrote:
> stella.stamenova wrote:
> > 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?
> I think the only way this assert would fail is if two symbols in the PDB were indicated as having the same address.  The only time we call `BuildAddrToSymbolMap` is if `cci.m_symbols_by_va.empty()` is true.  Then, we only insert items into it at the bottom of this loop.  So over the course of the loop, the same address would have had to have been added already.  
> 
> If the debug info says two symbols have the same address, there isn't much we can do other than to change this to some kind of multimap, but that just pushes the decision of how to handle the incorrect debug info to the caller, which doesn't have any better strategy for resolving this kind of ambiguity.  That said, down below we call `insert` which assumes the item is not already in the map.  We could change that to a `try_emplace`, then assert that the insert succeeded.  This would have the effect of asserting in the same case that this asserts, but the program still "working" (it would just drop anything other than the first symbol with a given address).
It was my understanding we don't want malformed debug info to ever crash the debugger: <https://reviews.llvm.org/D18646#388424> (last paragraph).


================
Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:78-85
+static bool ShouldUseNativeReader() {
+#if !defined(_WIN32)
+  return true;
+#endif
+  llvm::StringRef use_native = ::getenv("LLDB_USE_NATIVE_PDB_READER");
+  return use_native.equals_lower("on") || use_native.equals_lower("yes") ||
+         use_native.equals_lower("1") || use_native.equals_lower("true");
----------------
zturner wrote:
> labath wrote:
> > I am wondering whether it wouldn't be better to use an lldb setting instead of an env var (plugin.symbol-file.pdb.native ?). For this to work, we'd need to delay to decision of which plugin to use to until `CreateInstance` time, but that shouldn't be too hard.
> I thought about this but decided against it.  There are two reasons I didn't want to do this:
> 
> 1) On non-Windows platforms, the setting doesn't make any sense.  There is no other option.
> 2) It doesn't seem like something we want to "support" opting into or out of.  It's either in experimental / development mode, like right now, in which case the only reason you have to use it is if you're working on it.  Or it's complete, in which case we just delete the old one and make everything use the new one with no alternative.
> 
> So I couldn't come up with a valid use case to justify having it be a setting.
That's fine. I'll leave that up to you.


https://reviews.llvm.org/D53002