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

[Zachary Turner via lldb-commits]

The problems I have with current directory lookup are:

* 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.
* It doesn't match WinDbg or MSVC
* It's temporary functionality, and temporary functionality more often than
not ends up not being so temporary, thereby contributing to technical debt.
* 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.

Anyway, I don't want to hold this up any longer, but in the future let's
just implement the permanent solutions first so that we can avoid this type
of unnecessary blockage.

On Thu, Dec 13, 2018 at 1:10 PM Leonard Mosescu <mosescu at google.com> wrote:

> I think we can fix that by changing the line to:
>>
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>>
>
> The problem is that SymbolFileNativePDB returns the module object file as
> it's own object file. Are you suggesting we should track the module object
> file separate from SymbolFile::m_obj_file? In a way that would be a more
> accurate representation of what's going on (we don't have an ObjectFilePDB
> yet) - but the check is still a bit nonsensical (using the lack of an
> object file to indicate that the operation succeeded)
>
> I agree that the case that Pavel pointed out is an issue, although the
> non-nonsensical response only happens in the unlikely case you already have
> symbols (so there's little reason to explicitly re-add symbols). It's also
> not happening with PDBs (which is why it seemed safe).
>
> At this point I have to ask: what is the problem with the current
> directory lookup again? I seems to me that we complicated ourselves for no
> good reason (using a questionable functionality in a high level Windows IDE
> as reference).
>
>
> On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner <zturner at google.com>
> wrote:
>
>> I think we can fix that by changing the line to:
>>
>> ```
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>> ```
>>
>> On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath <pavel at labath.sk> wrote:
>>
>>> On 13/12/2018 19:32, Leonard Mosescu wrote:
>>> > What's the consensus?
>>> >
>>> > Personally I think that, even considering the potential issue that
>>> Paval
>>> > pointed out, the "target symbols add ..." is the most conservative
>>> > approach in terms of introducing new behavior. I'm fine with the
>>> current
>>> > directory lookup as well (the original change) since it's consistent
>>> > with DWARF lookup.
>>>
>>>
>>> Yes, but it also regresses existing functionality. Now if I do something
>>> completely nonsensical like:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> error: symbol file '/tmp/a.txt' does not match any existing module
>>>
>>> lldb will print a nice error for me. If I remove the safeguards like you
>>> did in your patch, it turns into this:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>>>
>>> which is a blatant lie, because /bin/ls will continue to use symbols
>>> from the object file.
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181213/b2b93805/attachment-0001.html>