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

Thu Dec 13 13:59:18 PST 2018

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/9601fd12/attachment-0001.html>