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

[Zachary Turner via lldb-commits]

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/95b01556/attachment.html>