Fix PR remote/21852: Remote run without specifying a local binary crashes GDB
Commit Message
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.
From: Pedro Alves <palves@redhat.com>
Date: 2017-08-22 18:42:27 +0100
fix remote.c
---
gdb/remote.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
Comments
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,
On 08/22/2017 11:56 PM, Sergio Durigan Junior wrote:
> 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.
... doesn't make sense because? The thing is we're missing an explicit
rationale from first principles.
> 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.
Well, I was hoping you'd go first. :-)
Thanks,
Pedro Alves
@@ -3831,19 +3831,16 @@ add_current_inferior_and_thread (char *wait_status)
{
struct remote_state *rs = get_remote_state ();
int fake_pid_p = 0;
- ptid_t ptid;
inferior_ptid = null_ptid;
/* Now, if we have thread information, update inferior_ptid. */
- ptid = get_current_thread (wait_status);
+ ptid_t curr_ptid = get_current_thread (wait_status);
- if (!ptid_equal (ptid, null_ptid))
+ if (curr_ptid != null_ptid)
{
if (!remote_multi_process_p (rs))
fake_pid_p = 1;
-
- inferior_ptid = ptid;
}
else
{
@@ -3851,14 +3848,17 @@ add_current_inferior_and_thread (char *wait_status)
(such as kill) won't work. This variable serves (at least)
double duty as both the pid of the target process (if it has
such), and as a flag indicating that a target is active. */
- inferior_ptid = magic_null_ptid;
+ curr_ptid = magic_null_ptid;
fake_pid_p = 1;
}
- remote_add_inferior (fake_pid_p, ptid_get_pid (inferior_ptid), -1, 1);
+ remote_add_inferior (fake_pid_p, ptid_get_pid (curr_ptid), -1, 1);
- /* Add the main thread. */
- add_thread_silent (inferior_ptid);
+ /* Add the main thread and switch to it. Don't try reading
+ registers yes, since we haven't fetched the target description
+ yet. */
+ thread_info *tp = add_thread_silent (curr_ptid);
+ switch_to_thread_no_regs (tp);
}
/* Print info about a thread that was found already stopped on