diff mbox

[v2,3/3] PR remote/19496, timeout in forking-threads-plus-bkpt

Message ID 1455150506-12760-1-git-send-email-donb@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Feb. 11, 2016, 12:28 a.m. UTC
On 2/1/2016 12:09 PM, Pedro Alves wrote:
> On 02/01/2016 07:29 PM, Don Breazeal wrote:
>> On 2/1/2016 4:05 AM, Pedro Alves wrote:
> 
>> Hi Pedro,
---snip---
>> A fork event was reported to GDB before GDB knew about the parent thread,
>> followed immediately by a breakpoint event in a different thread.  The
>> parent thread was subsequently added via remote_notice_new_inferior in
>> process_stop_reply, but when the thread was added the thread_info.state
>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>> but when the fork parent was resumed via a call to keep_going, the state
>> was unchanged.
> 
> Since this is non-stop, then it sounds to me like the bug is that the
> thread should have been added in THREAD_RUNNING state.
> 
> Consider that infrun may be pulling target events out of the target_ops
> backend into its own event queue, but, not process them immediately.
> 
> E.g., infrun may be stopping all threads temporarily for a step-over-breakpoint
> operation for thread A (stop_all_threads).  The waitstatus of all threads
> is thus left pending in the thread structure (save_status), including the
> fork event of thread B.  Right at this point, if the user
> does "info threads", that should show thread B (the fork parent) running,
> not stopped, even if internally, gdb is holding it paused for a little bit.
> 

Hi Pedro,
Here is a new patch that adds the threads with the state set to
THREAD_RUNNING for fork events.
Thanks!
--Don

This patch addresses a failure in
gdb.threads/forking-threads-plus-breakpoint.exp:

FAIL: gdb.threads/forking-threads-plus-breakpoint.exp: cond_bp_target=1:
detach_on_fork=on: inferior 1 exited (timeout)

Cause:
A fork event was reported to GDB before GDB knew about the parent thread,
followed immediately by a breakpoint event in a different thread.  The
parent thread was subsequently added via remote_notice_new_inferior in
process_stop_reply, but when the thread was added the thread_info.state
was set to THREAD_STOPPED.  The fork event was then handled correctly,
but when the fork parent was resumed via a call to keep_going, the state
was unchanged.

The breakpoint event was then handled, which caused all the non-breakpoint
threads to be stopped.  When the breakpoint thread was resumed, all the
non-breakpoint threads were resumed via infrun.c:restart_threads.  Our old
fork parent wasn't restarted, because it still had thread_info.state set to
THREAD_STOPPED.  Ultimately the program under debug hung waiting for a
pthread_join while the old fork parent was stopped forever by GDB.

Fix:
Make sure to add the fork parent thread in the THREAD_RUNNING state by
calling remote_notice_new_inferior with RUNNING set to 1 when processing
a fork stop reply.

Tested on x86_64 Linux and Nios II Linux target with x86 Linux host.

gdb/ChangeLog:
2016-02-10  Don Breazeal  <donb@codesourcery.com>

	* remote.c (process_stop_reply): Call remote_notice_new_inferior
	with RUNNING set to 1 when handling fork events.

---
 gdb/remote.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Don Breazeal Feb. 25, 2016, 5:29 p.m. UTC | #1
Ping
Thanks,
--Don

On 2/10/2016 4:28 PM, Don Breazeal wrote:
> On 2/1/2016 12:09 PM, Pedro Alves wrote:
>> On 02/01/2016 07:29 PM, Don Breazeal wrote:
>>> On 2/1/2016 4:05 AM, Pedro Alves wrote:
>>
>>> Hi Pedro,
> ---snip---
>>> A fork event was reported to GDB before GDB knew about the parent thread,
>>> followed immediately by a breakpoint event in a different thread.  The
>>> parent thread was subsequently added via remote_notice_new_inferior in
>>> process_stop_reply, but when the thread was added the thread_info.state
>>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>>> but when the fork parent was resumed via a call to keep_going, the state
>>> was unchanged.
>>
>> Since this is non-stop, then it sounds to me like the bug is that the
>> thread should have been added in THREAD_RUNNING state.
>>
>> Consider that infrun may be pulling target events out of the target_ops
>> backend into its own event queue, but, not process them immediately.
>>
>> E.g., infrun may be stopping all threads temporarily for a step-over-breakpoint
>> operation for thread A (stop_all_threads).  The waitstatus of all threads
>> is thus left pending in the thread structure (save_status), including the
>> fork event of thread B.  Right at this point, if the user
>> does "info threads", that should show thread B (the fork parent) running,
>> not stopped, even if internally, gdb is holding it paused for a little bit.
>>
> 
> Hi Pedro,
> Here is a new patch that adds the threads with the state set to
> THREAD_RUNNING for fork events.
> Thanks!
> --Don
> 
> This patch addresses a failure in
> gdb.threads/forking-threads-plus-breakpoint.exp:
> 
> FAIL: gdb.threads/forking-threads-plus-breakpoint.exp: cond_bp_target=1:
> detach_on_fork=on: inferior 1 exited (timeout)
> 
> Cause:
> A fork event was reported to GDB before GDB knew about the parent thread,
> followed immediately by a breakpoint event in a different thread.  The
> parent thread was subsequently added via remote_notice_new_inferior in
> process_stop_reply, but when the thread was added the thread_info.state
> was set to THREAD_STOPPED.  The fork event was then handled correctly,
> but when the fork parent was resumed via a call to keep_going, the state
> was unchanged.
> 
> The breakpoint event was then handled, which caused all the non-breakpoint
> threads to be stopped.  When the breakpoint thread was resumed, all the
> non-breakpoint threads were resumed via infrun.c:restart_threads.  Our old
> fork parent wasn't restarted, because it still had thread_info.state set to
> THREAD_STOPPED.  Ultimately the program under debug hung waiting for a
> pthread_join while the old fork parent was stopped forever by GDB.
> 
> Fix:
> Make sure to add the fork parent thread in the THREAD_RUNNING state by
> calling remote_notice_new_inferior with RUNNING set to 1 when processing
> a fork stop reply.
> 
> Tested on x86_64 Linux and Nios II Linux target with x86 Linux host.
> 
> gdb/ChangeLog:
> 2016-02-10  Don Breazeal  <donb@codesourcery.com>
> 
> 	* remote.c (process_stop_reply): Call remote_notice_new_inferior
> 	with RUNNING set to 1 when handling fork events.
> 
> ---
>  gdb/remote.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f09a06e..ab750a7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6818,7 +6818,14 @@ process_stop_reply (struct stop_reply *stop_reply,
>  	  VEC_free (cached_reg_t, stop_reply->regcache);
>  	}
>  
> -      remote_notice_new_inferior (ptid, 0);
> +      /* If a fork event arrived before we knew about the parent thread,
> +	 make sure to mark it as running when it is created.  */
> +      if (status->kind == TARGET_WAITKIND_FORKED
> +	  || status->kind == TARGET_WAITKIND_VFORKED)
> +	remote_notice_new_inferior (ptid, 1);
> +      else
> +	remote_notice_new_inferior (ptid, 0);
> +
>        remote_thr = demand_private_info (ptid);
>        remote_thr->core = stop_reply->core;
>        remote_thr->stop_reason = stop_reply->stop_reason;
>
Don Breazeal March 3, 2016, 6:20 p.m. UTC | #2
Ping.
I checked, the patch still applies cleanly to mainline.
Thanks
--Don

On 2/25/2016 9:29 AM, Don Breazeal wrote:
> Ping
> Thanks,
> --Don
> 
> On 2/10/2016 4:28 PM, Don Breazeal wrote:
>> On 2/1/2016 12:09 PM, Pedro Alves wrote:
>>> On 02/01/2016 07:29 PM, Don Breazeal wrote:
>>>> On 2/1/2016 4:05 AM, Pedro Alves wrote:
>>>
>>>> Hi Pedro,
>> ---snip---
>>>> A fork event was reported to GDB before GDB knew about the parent thread,
>>>> followed immediately by a breakpoint event in a different thread.  The
>>>> parent thread was subsequently added via remote_notice_new_inferior in
>>>> process_stop_reply, but when the thread was added the thread_info.state
>>>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>>>> but when the fork parent was resumed via a call to keep_going, the state
>>>> was unchanged.
>>>
>>> Since this is non-stop, then it sounds to me like the bug is that the
>>> thread should have been added in THREAD_RUNNING state.
>>>
>>> Consider that infrun may be pulling target events out of the target_ops
>>> backend into its own event queue, but, not process them immediately.
>>>
>>> E.g., infrun may be stopping all threads temporarily for a step-over-breakpoint
>>> operation for thread A (stop_all_threads).  The waitstatus of all threads
>>> is thus left pending in the thread structure (save_status), including the
>>> fork event of thread B.  Right at this point, if the user
>>> does "info threads", that should show thread B (the fork parent) running,
>>> not stopped, even if internally, gdb is holding it paused for a little bit.
>>>
>>
>> Hi Pedro,
>> Here is a new patch that adds the threads with the state set to
>> THREAD_RUNNING for fork events.
>> Thanks!
>> --Don
>>
>> This patch addresses a failure in
>> gdb.threads/forking-threads-plus-breakpoint.exp:
>>
>> FAIL: gdb.threads/forking-threads-plus-breakpoint.exp: cond_bp_target=1:
>> detach_on_fork=on: inferior 1 exited (timeout)
>>
>> Cause:
>> A fork event was reported to GDB before GDB knew about the parent thread,
>> followed immediately by a breakpoint event in a different thread.  The
>> parent thread was subsequently added via remote_notice_new_inferior in
>> process_stop_reply, but when the thread was added the thread_info.state
>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>> but when the fork parent was resumed via a call to keep_going, the state
>> was unchanged.
>>
>> The breakpoint event was then handled, which caused all the non-breakpoint
>> threads to be stopped.  When the breakpoint thread was resumed, all the
>> non-breakpoint threads were resumed via infrun.c:restart_threads.  Our old
>> fork parent wasn't restarted, because it still had thread_info.state set to
>> THREAD_STOPPED.  Ultimately the program under debug hung waiting for a
>> pthread_join while the old fork parent was stopped forever by GDB.
>>
>> Fix:
>> Make sure to add the fork parent thread in the THREAD_RUNNING state by
>> calling remote_notice_new_inferior with RUNNING set to 1 when processing
>> a fork stop reply.
>>
>> Tested on x86_64 Linux and Nios II Linux target with x86 Linux host.
>>
>> gdb/ChangeLog:
>> 2016-02-10  Don Breazeal  <donb@codesourcery.com>
>>
>> 	* remote.c (process_stop_reply): Call remote_notice_new_inferior
>> 	with RUNNING set to 1 when handling fork events.
>>
>> ---
>>  gdb/remote.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index f09a06e..ab750a7 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -6818,7 +6818,14 @@ process_stop_reply (struct stop_reply *stop_reply,
>>  	  VEC_free (cached_reg_t, stop_reply->regcache);
>>  	}
>>  
>> -      remote_notice_new_inferior (ptid, 0);
>> +      /* If a fork event arrived before we knew about the parent thread,
>> +	 make sure to mark it as running when it is created.  */
>> +      if (status->kind == TARGET_WAITKIND_FORKED
>> +	  || status->kind == TARGET_WAITKIND_VFORKED)
>> +	remote_notice_new_inferior (ptid, 1);
>> +      else
>> +	remote_notice_new_inferior (ptid, 0);
>> +
>>        remote_thr = demand_private_info (ptid);
>>        remote_thr->core = stop_reply->core;
>>        remote_thr->stop_reason = stop_reply->stop_reason;
>>
>
Don Breazeal March 14, 2016, 9:30 p.m. UTC | #3
Ping.
Thanks,
--Don

On 3/3/2016 10:20 AM, Don Breazeal wrote:
> Ping.
> I checked, the patch still applies cleanly to mainline.
> Thanks
> --Don
> 
> On 2/25/2016 9:29 AM, Don Breazeal wrote:
>> Ping
>> Thanks,
>> --Don
>>
>> On 2/10/2016 4:28 PM, Don Breazeal wrote:
>>> On 2/1/2016 12:09 PM, Pedro Alves wrote:
>>>> On 02/01/2016 07:29 PM, Don Breazeal wrote:
>>>>> On 2/1/2016 4:05 AM, Pedro Alves wrote:
>>>>
>>>>> Hi Pedro,
>>> ---snip---
>>>>> A fork event was reported to GDB before GDB knew about the parent thread,
>>>>> followed immediately by a breakpoint event in a different thread.  The
>>>>> parent thread was subsequently added via remote_notice_new_inferior in
>>>>> process_stop_reply, but when the thread was added the thread_info.state
>>>>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>>>>> but when the fork parent was resumed via a call to keep_going, the state
>>>>> was unchanged.
>>>>
>>>> Since this is non-stop, then it sounds to me like the bug is that the
>>>> thread should have been added in THREAD_RUNNING state.
>>>>
>>>> Consider that infrun may be pulling target events out of the target_ops
>>>> backend into its own event queue, but, not process them immediately.
>>>>
>>>> E.g., infrun may be stopping all threads temporarily for a step-over-breakpoint
>>>> operation for thread A (stop_all_threads).  The waitstatus of all threads
>>>> is thus left pending in the thread structure (save_status), including the
>>>> fork event of thread B.  Right at this point, if the user
>>>> does "info threads", that should show thread B (the fork parent) running,
>>>> not stopped, even if internally, gdb is holding it paused for a little bit.
>>>>
>>>
>>> Hi Pedro,
>>> Here is a new patch that adds the threads with the state set to
>>> THREAD_RUNNING for fork events.
>>> Thanks!
>>> --Don
>>>
>>> This patch addresses a failure in
>>> gdb.threads/forking-threads-plus-breakpoint.exp:
>>>
>>> FAIL: gdb.threads/forking-threads-plus-breakpoint.exp: cond_bp_target=1:
>>> detach_on_fork=on: inferior 1 exited (timeout)
>>>
>>> Cause:
>>> A fork event was reported to GDB before GDB knew about the parent thread,
>>> followed immediately by a breakpoint event in a different thread.  The
>>> parent thread was subsequently added via remote_notice_new_inferior in
>>> process_stop_reply, but when the thread was added the thread_info.state
>>> was set to THREAD_STOPPED.  The fork event was then handled correctly,
>>> but when the fork parent was resumed via a call to keep_going, the state
>>> was unchanged.
>>>
>>> The breakpoint event was then handled, which caused all the non-breakpoint
>>> threads to be stopped.  When the breakpoint thread was resumed, all the
>>> non-breakpoint threads were resumed via infrun.c:restart_threads.  Our old
>>> fork parent wasn't restarted, because it still had thread_info.state set to
>>> THREAD_STOPPED.  Ultimately the program under debug hung waiting for a
>>> pthread_join while the old fork parent was stopped forever by GDB.
>>>
>>> Fix:
>>> Make sure to add the fork parent thread in the THREAD_RUNNING state by
>>> calling remote_notice_new_inferior with RUNNING set to 1 when processing
>>> a fork stop reply.
>>>
>>> Tested on x86_64 Linux and Nios II Linux target with x86 Linux host.
>>>
>>> gdb/ChangeLog:
>>> 2016-02-10  Don Breazeal  <donb@codesourcery.com>
>>>
>>> 	* remote.c (process_stop_reply): Call remote_notice_new_inferior
>>> 	with RUNNING set to 1 when handling fork events.
>>>
>>> ---
>>>  gdb/remote.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index f09a06e..ab750a7 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -6818,7 +6818,14 @@ process_stop_reply (struct stop_reply *stop_reply,
>>>  	  VEC_free (cached_reg_t, stop_reply->regcache);
>>>  	}
>>>  
>>> -      remote_notice_new_inferior (ptid, 0);
>>> +      /* If a fork event arrived before we knew about the parent thread,
>>> +	 make sure to mark it as running when it is created.  */
>>> +      if (status->kind == TARGET_WAITKIND_FORKED
>>> +	  || status->kind == TARGET_WAITKIND_VFORKED)
>>> +	remote_notice_new_inferior (ptid, 1);
>>> +      else
>>> +	remote_notice_new_inferior (ptid, 0);
>>> +
>>>        remote_thr = demand_private_info (ptid);
>>>        remote_thr->core = stop_reply->core;
>>>        remote_thr->stop_reason = stop_reply->stop_reason;
>>>
>>
>
diff mbox

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index f09a06e..ab750a7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6818,7 +6818,14 @@  process_stop_reply (struct stop_reply *stop_reply,
 	  VEC_free (cached_reg_t, stop_reply->regcache);
 	}
 
-      remote_notice_new_inferior (ptid, 0);
+      /* If a fork event arrived before we knew about the parent thread,
+	 make sure to mark it as running when it is created.  */
+      if (status->kind == TARGET_WAITKIND_FORKED
+	  || status->kind == TARGET_WAITKIND_VFORKED)
+	remote_notice_new_inferior (ptid, 1);
+      else
+	remote_notice_new_inferior (ptid, 0);
+
       remote_thr = demand_private_info (ptid);
       remote_thr->core = stop_reply->core;
       remote_thr->stop_reason = stop_reply->stop_reason;