Re: [PATCH] Fix PR remote/21852: Remote run without specifying a local binary crashes GDB

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, August 22 2017, Pedro Alves wrote:

> On 08/22/2017 03:04 PM, Sergio Durigan Junior wrote:
>> The fix for PR gdb/20609:
>> 
>>   commit bb805577d2b212411fb7b0a2d01644567fac4e8d
>>   Author: Jan Kratochvil <jan.kratochvil@redhat.com>
>>   Date:   Thu Sep 29 17:38:16 2016 +0200
>> 
>> Introduced the concept of deferring the call to breakpoint_re_set on
>> certain useful occasions.  However, there is one specific scenario
>> where delaying needs to be done and still isn't: the case when we're
>> starting a GDB to debug a remote inferior without specifying a local
>> binary, as in for example:
>> 
>>   ./gdb -nx -q --data-directory=data-directory -ex "tar ext :1234" \
>>     -ex "set remote exec-file /bin/ls" -ex r
>> 
>> In this case, when calling exec_file_locate_attach to locate the
>> inferior, GDB is incorrectly resetting the breakpoints without a
>> thread/inferior even running, which causes an assertion to be
>> triggered:
>> 
>>   binutils-gdb/gdb/thread.c:1609: internal-error: scoped_restore_current_thread::scoped_restore_current_thread(): Assertion `tp != NULL' failed.
>>   A problem internal to GDB has been detected,
>>   further debugging may prove unreliable.
>>   Quit this debugging session? (y or n)
>> 
>> The right thing to do is to defer resetting the breakpoints when
>> locating the binary, which is what this patch does.
>
> Hmm, I think we're missing more rationale.  There may well be
> other reasons for doing that, but this case just looks like a
> case of remote.c breaking invariants to me -- making inferior_ptid
> point to a non-existing thread and then calling common code is
> recipe for disaster.  Seems to me that the fix is just to
> not do that?  See patch below.  It fixes your test for me
> as well, though I haven't run the full testsuite.

Thanks for the review.

Well, what can I say.  My fix looked right from my perspective, and I
confess that at the beginning I had the same thought: remote.c is
causing the problem by making inferior_ptid point to a non existing
thread.  However, I quickly found that the culprit was on the call chain
leading to exec_file_locate_attach and concentrated my focus on that.

Your patch looks more complete and to the point indeed.  Although it
seems to me, from what I observed, that calling breakpoint_re_set on
exec_file_locate_attach when dealing with a remote inferior doesn't make
sense either.

Anyway, I'll resubmit my patch using your approach and leave my first
patch aside for a bit, until I hear what you think about not calling
breakpoint_re_set on this specific case.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/