[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

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 :)

On Thu, Dec 13, 2018 at 2:19 PM Zachary Turner <zturner at google.com> wrote:

> 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.
>
> On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu <mosescu at google.com>
> wrote:
>
>> 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.
>>
>> 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.
>>
>>
>>> * 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.
>>>
>> 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.
>>
>>
>>> * It doesn't match WinDbg or MSVC
>>>
>> 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).
>>
>>
>>> * It's temporary functionality, and temporary functionality more often
>>> than not ends up not being so temporary, thereby contributing to technical
>>> debt.
>>>
>> If we unify the logic with the DWARF SymbolVendor then only the
>> implementation itself it temporary, the lookup logic would not change,
>> right?
>>
>>
>>> * 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
>>>
>> 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.
>>
>> Also, just a sanity check: what do you think is the permanent solution?
>>
>>
>> On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> 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.
>>>
>>> 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.
>>>
>>> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
>>> reviews at reviews.llvm.org> wrote:
>>>
>>>> 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
>>>>
>>>>
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181213/4969b210/attachment-0001.html>