<div dir="ltr">At this point it seems like perpetuating the hack, or at least even if that's the direction we decide to go longterm, not implementing that solution fully and missing some of the corner cases.<div><br></div><div>So I think I'd rather just go with the original hack of checking the current directory at this point.  Still though, I want to make sure that we're on the same page that in the future if we're going to need more hacks of some kind to delay implementing a proper solution, that we shelve the work until the proper solution is implemented and tested.  It may not be the best thing to do in the short term, but it is in the long term, which is what i'm trying to optimize for.</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg requested changes to this revision.<br>
clayborg added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
Just need a way to verify symbols are good. See my inline comment.<br>
<br>
<br>
<br>
================<br>
Comment at: source/Commands/CommandObjectTarget.cpp:4246<br>
           if (symbol_file) {<br>
-            ObjectFile *object_file = symbol_file->GetObjectFile();<br>
<br>
----------------<br>
labath wrote:<br>
> lemo wrote:<br>
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the SymbolFileNativePDB always points to the associated PE binary. <br>
> > <br>
> > 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?<br>
> 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?<br>
> <br>
> I think this is strictly worse that the previous solution as it lets the objectless-symbol-file hack leak out of SymbolFilePDB.<br>
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:<br>
<br>
```<br>
virtual bool SymbolFile::IsValid() const {<br>
  return GetObjectFile() != nullptr;<br>
}<br>
```<br>
And we can override for SymbolFile subclasses that arenb't objfile based. How does this sound?<br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D55142/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55142/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D55142" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55142</a><br>
<br>
<br>
<br>
</blockquote></div>