Re: [PATCH 1/8] [GDBserver] Leave child suspended when step over parent

[Luis Machado <lgustavo@codesourcery.com>]

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. :-)