Re: [PATCH 4/7] range stepping: gdb

[Pedro Alves <palves@redhat.com>]

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

> @@ -4689,7 +4694,36 @@ append_resumption (char *p, char *endp,
>    if (step && siggnal != GDB_SIGNAL_0)
>      p += xsnprintf (p, endp - p, ";S%02x", siggnal);
>    else if (step)
> -    p += xsnprintf (p, endp - p, ";s");
> +    {
> +      struct thread_info *tp = NULL;
> +      CORE_ADDR pc;
> +
> +      gdb_assert (!ptid_equal (ptid, minus_one_ptid));

I think you'll trip on this in case the target supports
vCont but not actually listing thread ids (magic_null_ptid).


> +
> +      if (ptid_is_pid (ptid))
> +	tp = find_thread_ptid (inferior_ptid);
> +      else
> +	tp = find_thread_ptid (ptid);
> +      gdb_assert (tp != NULL);
> +
> +      pc = regcache_read_pc (get_thread_regcache (ptid));
> +      if (rs->support_vCont.r /* Target supports step range.  */
> +	  /* Can't do range stepping for all threads of a process
> +	     'pPID.-1'.  */
> +	  && !(remote_multi_process_p (rs) && ptid_is_pid (ptid))
> +	  /* Not step over a breakpoint.  */
> +	  && !tp->control.trap_expected
> +	  /* We have a range to single step.  */
> +	  && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc))

This is problematic.  It's better to _not_ have target itself decide
when to range step or to single step, and peeking at "infrun-owned"
variables.  For example, with software watchpoints, GDB needs to have control
of single-steps, in order to evaluate the watchpoints at after each
instruction is executed, so trap_expected isn't enough.  (My v3 adds a test for
that, that v2 fails.)
I dislike the design of using PC checks here too :-/.  That
seems fragile, and potentially inefficient (considering GDB ever
sending more than one range action per packet, that might end up
fetching registers for threads unnecessarily).  IMO, it's better to have
the core run control (infrun.c/infcmd.c) decide when to range step or
single-step.

> +	{
> +	  p += xsnprintf (p, endp - p, ";r");
> +	  p += hexnumstr (p, tp->control.step_range_start);
> +	  p += xsnprintf (p, endp - p, ",");
> +	  p += hexnumstr (p, tp->control.step_range_end);
> +	}
> +      else
> +	p += xsnprintf (p, endp - p, ";s");
> +    }
>    else if (siggnal != GDB_SIGNAL_0)
>      p += xsnprintf (p, endp - p, ";C%02x", siggnal);
>    else
> 

Thanks,
-- 
Pedro Alves