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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
