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

Pedro Alves
2013-05-14T18:30:41Z

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