[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Thu Dec 13 13:39:23 PST 2018

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just need a way to verify symbols are good. See my inline comment.

================
Comment at: source/Commands/CommandObjectTarget.cpp:4246
           if (symbol_file) {
-            ObjectFile *object_file = symbol_file->GetObjectFile();

----------------
labath wrote:
> lemo wrote:
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the SymbolFileNativePDB always points to the associated PE binary. 
> > 
> > the check itself seems valuable as a diagnostic but not strictly required. Should I add a TODO comment and/or open a bug to revisit this?
> I not sure this is a good idea. Isn't this the only way of providing feedback about whether the symbols were actually added? If we are unable to load the symbol file specified (perhaps because the user made a typo, or the file is corrupted), then the symbol vendor will just create a default SymbolFile backed by the original object file. Doesn't that mean this will basically always return true now?
> 
> I think this is strictly worse that the previous solution as it lets the objectless-symbol-file hack leak out of SymbolFilePDB.
We need to add some sanity check where we ask the symbol file if it is valid. It should be virtual function in SymbolFile that defaults to:

```
virtual bool SymbolFile::IsValid() const {
  return GetObjectFile() != nullptr;
}
```
And we can override for SymbolFile subclasses that arenb't objfile based. How does this sound?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55142/new/

https://reviews.llvm.org/D55142