diff mbox

[1/8,GDBserver] Leave child suspended when step over parent

Message ID 1455892594-2294-2-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Feb. 19, 2016, 2:36 p.m. UTC 
  I see the following GDBserver internal error in two cases,

 gdb/gdbserver/linux-low.c:1922: A problem internal to GDBserver has been detected.
 unsuspend LWP 17200, suspended=-1

 1. step over a breakpoint on fork/vfork syscall instruction,
 2. step over a breakpoint on clone syscall instruction and child
    threads hits a breakpoint,

the stack backtrace is

 #0  internal_error (file=file@entry=0x44c4c0 "gdb/gdbserver/linux-low.c", line=line@entry=1922,
    fmt=fmt@entry=0x44c7d0 "unsuspend LWP %ld, suspended=%d\n") at gdb/gdbserver/../common/errors.c:51
 #1  0x0000000000424014 in lwp_suspended_decr (lwp=<optimised out>, lwp=<optimised out>) at gdb/gdbserver/linux-low.c:1922
 #2  0x000000000042403a in unsuspend_one_lwp (entry=<optimised out>, except=0x66e8c0) at gdb/gdbserver/linux-low.c:2885
 #3  0x0000000000405f45 in find_inferior (list=<optimised out>, func=func@entry=0x424020 <unsuspend_one_lwp>, arg=arg@entry=0x66e8c0)
    at gdb/gdbserver/inferiors.c:243
 #4  0x00000000004297de in unsuspend_all_lwps (except=0x66e8c0) at gdb/gdbserver/linux-low.c:2895
 #5  linux_wait_1 (ptid=..., ourstatus=ourstatus@entry=0x665ec0 <last_status>, target_options=target_options@entry=0)
    at gdb/gdbserver/linux-low.c:3632
 #6  0x000000000042a764 in linux_wait (ptid=..., ourstatus=0x665ec0 <last_status>, target_options=0)
    at gdb/gdbserver/linux-low.c:3770
 #7  0x0000000000411163 in mywait (ptid=..., ourstatus=ourstatus@entry=0x665ec0 <last_status>, options=options@entry=0, connected_wait=connected_wait@entry=1)
    at gdb/gdbserver/target.c:214
 #8  0x000000000040b1f2 in resume (actions=0x66f800, num_actions=1) at gdb/gdbserver/server.c:2757
 #9  0x000000000040f660 in handle_v_cont (own_buf=0x66a630 "vCont;c:p45e9.-1") at gdb/gdbserver/server.c:2719

when GDBserver steps over a thread, other threads have been suspended,
the "stepping" thread may create new thread, but GDBserver doesn't set
it suspend count to 1.  When GDBserver unsuspend threads, the child's
suspend count goes to -1, and the assert is triggered.  In fact, GDBserver
has already taken care of suspend count of new thread when GDBserver is
suspending all threads except the one GDBserver wants to step over by
https://sourceware.org/ml/gdb-patches/2015-07/msg00946.html

+	  /* If we're suspending all threads, leave this one suspended
+	     too.  */
+	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
+	    {
+	      if (debug_threads)
+		debug_printf ("HEW: leaving child suspended\n");
+	      child_lwp->suspended = 1;
+	    }

but that is not enough, because new thread can be still spawned in
the thread which is being stepped over.  This patch extends the
condition that GDBserver set child's suspend count to one if it is
suspending threads or stepping over the thread.

gdb/gdbserver:

2016-02-19  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (handle_extended_wait): Set child suspended
	if event_lwp->bp_reinsert isn't zero.
---
 gdb/gdbserver/linux-low.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Luis Machado Feb. 23, 2016, 8:23 p.m. UTC | #1 
On 02/19/2016 12:36 PM, Yao Qi wrote:
> I see the following GDBserver internal error in two cases,
>
>   gdb/gdbserver/linux-low.c:1922: A problem internal to GDBserver has been detected.
>   unsuspend LWP 17200, suspended=-1
>
>   1. step over a breakpoint on fork/vfork syscall instruction,
>   2. step over a breakpoint on clone syscall instruction and child
>      threads hits a breakpoint,
>
> the stack backtrace is
>
>   #0  internal_error (file=file@entry=0x44c4c0 "gdb/gdbserver/linux-low.c", line=line@entry=1922,
>      fmt=fmt@entry=0x44c7d0 "unsuspend LWP %ld, suspended=%d\n") at gdb/gdbserver/../common/errors.c:51
>   #1  0x0000000000424014 in lwp_suspended_decr (lwp=<optimised out>, lwp=<optimised out>) at gdb/gdbserver/linux-low.c:1922
>   #2  0x000000000042403a in unsuspend_one_lwp (entry=<optimised out>, except=0x66e8c0) at gdb/gdbserver/linux-low.c:2885
>   #3  0x0000000000405f45 in find_inferior (list=<optimised out>, func=func@entry=0x424020 <unsuspend_one_lwp>, arg=arg@entry=0x66e8c0)
>      at gdb/gdbserver/inferiors.c:243
>   #4  0x00000000004297de in unsuspend_all_lwps (except=0x66e8c0) at gdb/gdbserver/linux-low.c:2895
>   #5  linux_wait_1 (ptid=..., ourstatus=ourstatus@entry=0x665ec0 <last_status>, target_options=target_options@entry=0)
>      at gdb/gdbserver/linux-low.c:3632
>   #6  0x000000000042a764 in linux_wait (ptid=..., ourstatus=0x665ec0 <last_status>, target_options=0)
>      at gdb/gdbserver/linux-low.c:3770
>   #7  0x0000000000411163 in mywait (ptid=..., ourstatus=ourstatus@entry=0x665ec0 <last_status>, options=options@entry=0, connected_wait=connected_wait@entry=1)
>      at gdb/gdbserver/target.c:214
>   #8  0x000000000040b1f2 in resume (actions=0x66f800, num_actions=1) at gdb/gdbserver/server.c:2757
>   #9  0x000000000040f660 in handle_v_cont (own_buf=0x66a630 "vCont;c:p45e9.-1") at gdb/gdbserver/server.c:2719
>
> when GDBserver steps over a thread, other threads have been suspended,
> the "stepping" thread may create new thread, but GDBserver doesn't set
> it suspend count to 1.  When GDBserver unsuspend threads, the child's
> suspend count goes to -1, and the assert is triggered.  In fact, GDBserver
> has already taken care of suspend count of new thread when GDBserver is
> suspending all threads except the one GDBserver wants to step over by
> https://sourceware.org/ml/gdb-patches/2015-07/msg00946.html
>
> +	  /* If we're suspending all threads, leave this one suspended
> +	     too.  */
> +	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
> +	    {
> +	      if (debug_threads)
> +		debug_printf ("HEW: leaving child suspended\n");
> +	      child_lwp->suspended = 1;
> +	    }
>
> but that is not enough, because new thread can be still spawned in
> the thread which is being stepped over.  This patch extends the
> condition that GDBserver set child's suspend count to one if it is
> suspending threads or stepping over the thread.
>
> gdb/gdbserver:
>
> 2016-02-19  Yao Qi  <yao.qi@linaro.org>
>
> 	* linux-low.c (handle_extended_wait): Set child suspended
> 	if event_lwp->bp_reinsert isn't zero.
> ---
>   gdb/gdbserver/linux-low.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 3f085fd..3765e08 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -529,8 +529,10 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>   	  child_thr->last_status.kind = TARGET_WAITKIND_STOPPED;
>
>   	  /* If we're suspending all threads, leave this one suspended
> -	     too.  */
> -	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
> +	     too.  If we're stepping over the parent, all other threads
> +	     have been suspended already, leave this one suspended too.  */
> +	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> +	      || event_lwp->bp_reinsert != 0)
>   	    {
>   	      if (debug_threads)
>   		debug_printf ("HEW: leaving child suspended\n");
> @@ -583,9 +585,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>   	 before calling linux_resume_one_lwp.  */
>         new_lwp->stopped = 1;
>
> -     /* If we're suspending all threads, leave this one suspended
> -	too.  */
> -      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
> +      /* If we're suspending all threads, leave this one suspended
> +	 too.  If we're stepping over the parent, all other threads
> +	 have been suspended already, leave this one suspended too.  */
> +      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> +	  || event_lwp->bp_reinsert != 0)
>   	new_lwp->suspended = 1;
>
>         /* Normally we will get the pending SIGSTOP.  But in some cases
>

The comment is a bit off with regards to what is really being checked 
here. Maybe it is the code, but "event_lwp->bp_reinsert != 0" doesn't 
exactly translate to "stepping over the parent".

Maybe things would improve if we defined what "this one" means in the 
second case. It is a new thread or a child process thread, right?

I'm looking at the two hunks above and they seem to want to be unified, 
but it is beside the point of the patch. :-)
Pedro Alves Feb. 25, 2016, 5:11 p.m. UTC | #2 
LGTM.

On 02/19/2016 02:36 PM, Yao Qi wrote:

>  	  /* If we're suspending all threads, leave this one suspended
> -	     too.  */
> -	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
> +	     too.  If we're stepping over the parent, all other threads
> +	     have been suspended already, leave this one suspended too.  */

I'd write:

	     too.  If the fork/clone parent is stepping over a breakpoint, all
             other threads have been suspended already.  Leave the child
             suspended too.  */

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3f085fd..3765e08 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -529,8 +529,10 @@  handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	  child_thr->last_status.kind = TARGET_WAITKIND_STOPPED;
 
 	  /* If we're suspending all threads, leave this one suspended
-	     too.  */
-	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
+	     too.  If we're stepping over the parent, all other threads
+	     have been suspended already, leave this one suspended too.  */
+	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
+	      || event_lwp->bp_reinsert != 0)
 	    {
 	      if (debug_threads)
 		debug_printf ("HEW: leaving child suspended\n");
@@ -583,9 +585,11 @@  handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	 before calling linux_resume_one_lwp.  */
       new_lwp->stopped = 1;
 
-     /* If we're suspending all threads, leave this one suspended
-	too.  */
-      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS)
+      /* If we're suspending all threads, leave this one suspended
+	 too.  If we're stepping over the parent, all other threads
+	 have been suspended already, leave this one suspended too.  */
+      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
+	  || event_lwp->bp_reinsert != 0)
 	new_lwp->suspended = 1;
 
       /* Normally we will get the pending SIGSTOP.  But in some cases