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

[Leonard Mosescu via lldb-commits]

>
> 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/0deb9b66/attachment.html>