On irc earlier i was talking about this with Greg and he said it should be fine in his opinion.  I’ll point him to this review in the morning so he can comment <br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath 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">labath added inline comments.<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>
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>
<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>