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 <ztur...@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 <
> revi...@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
>>
>>
>>
>>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
