<div dir="ltr"><div>Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as necessary assuming we all agree on the plan: we could implement a ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup detail goes away. </div><div><br></div><div>My intention was to enable other developers to start consuming PDBs with minidumps, but perhaps I rushed ahead too much. The long discussion clearly suggests that the changes are not ready, so I'll shelve them for now and keep them until the timing is right.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>* It makes the behavior dependent on the environment, much like using an environment variable.  This is a potential source of flakiness in tests, or different behavior on different peoples' machines.</div></blockquote><div>I mostly agree. Although 1) this matches what LLDB already does for DWARF and 2) I think the issues with flakiness are a bit overblown: there's a lot of stuff which depends on the CWD, and the environment for tests should be predictable in general. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>* It doesn't match WinDbg or MSVC</div></blockquote><div>Sure, but neither does looking next to the .dmp file (which is purely a VisualStudio invention - and any IDE can do the same on top of LLDB if they really choose to, but it should not be backed in IMO).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>* It's temporary functionality, and temporary functionality more often than not ends up not being so temporary, thereby contributing to technical debt.</div></blockquote><div>If we unify the logic with the DWARF SymbolVendor then only the implementation itself it temporary, the lookup logic would not change, right?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>* We already know what the permanent solution is, and we're going to have to implement it anyway, so we could avoid this by just implementing the permanent solution first</div></blockquote><div>The catch22 is that we can't test anything else involving minidumps + PDBs in the meantime. I found that exercising that combination to be very useful in uncovering other parts which need attention.</div><div><br></div><div>Also, just a sanity check: what do you think is the permanent solution?</div><div> </div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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" target="_blank">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>
</blockquote></div>