<div dir="ltr">That said, as you mentioned this enables other developers to start working on things, and if that means we can get the SymbolVendor in more quickly, then we can get rid of this more quickly.  I just really want to converge towards the permanent solution, rather than away from it, so if committing this gets us there quicker then I can get behind it.  I trust your judgement here :)</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 2:19 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">The permanent solution would be figuring out what to do about the ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we want to live in a world where SymbolFiles need not be backed by ObjectFiles?), and then regardless of what we decide there, implementing the SymbolVendor that can search a combination of locations including (but not limited to) a symbol server.  lldb already has the notion of a symbol path.  So using this we could just use that existing logic to search the symbol path, and before you run LLDB make sure your minidump .pdb files are in that search path.  If we make an ObjectFilePDB, then this should be able to fall out naturally of the existing design with no additional work, because it will use the normal SymbolVendor search logic.  If we allow SymbolFilePDB to live without an ObjectFilePDB, then we would have to make changes similar to what you proposed earlier where we can still get a SymbolVendor and use it to execute its default search algorithm, plus probably some other changes to make sure various other things work correctly in the presence of ObjectFile-less SymbolFiles.</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu <<a href="mailto:mosescu@google.com" target="_blank">mosescu@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"><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><div dir="ltr"><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><div dir="ltr"><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 dir="ltr"><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><div dir="ltr"><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 dir="ltr"><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><div dir="ltr"><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 dir="ltr"><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><div dir="ltr"><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" target="_blank">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>
</blockquote></div>
</blockquote></div>