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

Zachary Turner via lldb-commits lldb-commits at lists.llvm.org
Thu Dec 13 14:19:07 PST 2018

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?
>>>   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/5d0eebf4/attachment.html>

More information about the lldb-commits mailing list