Re: [PATCH 3/7] range stepping: gdbserver on x86/linux

[Pedro Alves <palves@redhat.com>]

On 03/11/2013 12:51 PM, Yao Qi wrote:


> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 922a5bf..0b330d1 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -1871,10 +1871,15 @@ handle_v_cont (char *own_buf)
>    p = &own_buf[5];
>    while (*p)
>      {
> +      CORE_ADDR start = 0;
> +      CORE_ADDR end = 0;
> +
>        p++;
>  
>        if (p[0] == 's' || p[0] == 'S')
>  	resume_info[i].kind = resume_step;
> +      else if (p[0] == 'r')
> +	resume_info[i].kind = resume_step;
>        else if (p[0] == 'c' || p[0] == 'C')
>  	resume_info[i].kind = resume_continue;
>        else if (p[0] == 't')
> @@ -1894,6 +1899,21 @@ handle_v_cont (char *own_buf)
>  	    goto err;
>  	  resume_info[i].sig = gdb_signal_to_host (sig);
>  	}
> +      else if (p[0] == 'r')
> +	{
> +	  char *p1;
> +
> +	  p = p + 1;
> +	  p1 = strchr (p, ',');
> +	  decode_address (&start, p, p1 - p);
> +
> +	  p = p1 + 1;
> +	  p1 = strchr (p, ':');
> +	  decode_address (&end, p, p1 - p);
> +
> +	  resume_info[i].sig = 0;
> +	  p = p1;
> +	}
>        else
>  	{
>  	  resume_info[i].sig = 0;
> @@ -1919,6 +1939,21 @@ handle_v_cont (char *own_buf)
>  	    goto err;
>  
>  	  resume_info[i].thread = ptid;
> +	  if (end > 0)
> +	    {
> +	      struct thread_info *tp = find_thread_ptid (ptid);
> +
> +	      /* GDB should not send range stepping for all threads of
> +		 a process, like 'vCont;rSTART,END:pPID.-1', TP can't
> +		 be NULL.  */
> +	      gdb_assert (tp != NULL);
> +
> +	      tp->start = start;
> +	      tp->end = end;
> +
> +	      start = 0;
> +	      end = 0;
> +	    }

My main issues with this are:

 - it assumes GDB will ever only send one r action per vCont.  I'd much
   rather we don't bake in that assumption.

 - GDBserver should not gdb_assert for unexpected input it can't control.

 - It seems like this leaves threads with stale step ranges.  Because
   linux-low.c only clears the stepping range of the thread that is reporting
   the event (here):

> @@ -2685,6 +2705,8 @@ Check if we're already there.\n",
>  	fprintf (stderr, "Hit a non-gdbserver trap event.\n");
>      }
>
> +  thread_clear_range_stepping (current_inferior);
> +
>    /* Alright, we're going to report a stop.  */
>
>    if (!non_stop && !stabilizing_threads)
> @@ -5081,6 +5103,15 @@ linux_supports_agent (void)
>    return 1;
>  }

That is, e.g.,:

    - user "step"s thread 1 (vCont;r)
    - thread 2 hits breakpoint (step range of thread 2 is cleared)
    - user goes back to thread 1, and does "si".
    - GDB sends vCont;s, but thread 1 still has the stale step range
      from the previous vCont;r.

We should probably have an explicit test for this, though I
haven't added one myself (at least yet).

-- 
Pedro Alves