Re: [PATCH 5/7] range stepping: New command 'maint set range stepping'

[Pedro Alves <palves@redhat.com>]

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

> @@ -4647,6 +4651,7 @@ remote_vcont_probe (struct remote_state *rs)
>        support_C = 0;
>        rs->support_vCont.t = 0;
>        rs->support_vCont.r = 0;
> +      use_range_stepping = 0;
>        while (p && *p == ';')
>  	{
>  	  p++;
> @@ -4661,7 +4666,10 @@ remote_vcont_probe (struct remote_state *rs)
>  	  else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0))
>  	    rs->support_vCont.t = 1;
>  	  else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0))
> -	    rs->support_vCont.r = 1;
> +	    {
> +	      rs->support_vCont.r = 1;
> +	      use_range_stepping = 1;
> +	    }

I don't think this is a good approach.  :-(  If the user does
"maint set range-stepping", and then connects, she'll reasonably
assume range stepping will be disabled.  She could have a stub that
has known broken range stepping, for example.  However, with this
approach, if the target supports it, range stepping will always end
up forced enabled as soon as GDB sends the first vCont packet, even
if the user set range stepping off.

So I think the "maint set range-stepping" toggle should be enabled
by default, and it should only represent GDB's willingness to use
the feature.

>  
>  	  p = strchr (p, ';');
>  	}
> @@ -4708,6 +4716,8 @@ append_resumption (char *p, char *endp,
>  
>        pc = regcache_read_pc (get_thread_regcache (ptid));
>        if (rs->support_vCont.r /* Target supports step range.  */
> +	  /* GDB is willing to do range stepping.  */
> +	  && use_range_stepping
>  	  /* Can't do range stepping for all threads of a process
>  	     'pPID.-1'.  */
>  	  && !(remote_multi_process_p (rs) && ptid_is_pid (ptid))
> @@ -11488,6 +11498,45 @@ remote_upload_trace_state_variables (struct uploaded_tsv **utsvp)
>    return 0;
>  }
>  
> +static void
> +maint_show_range_stepping (struct ui_file *file, int from_tty,
> +			   struct cmd_list_element *c,
> +			   const char *value)
> +{
> +  fprintf_filtered (file,
> +		    _("Debugger's willingness to do range-stepping "
> +		      "is %s.\n"), value);
> +}
> +
> +static void
> +maint_set_range_stepping (char *ignore_args, int from_tty,
> +			  struct cmd_list_element *c)
> +{
> +  /* Check range stepping is supported when turns it on.  */
> +  if (use_range_stepping)
> +    {
> +      if (remote_desc != NULL)
> +	{
> +	  struct remote_state *rs = get_remote_state ();
> +
> +	  if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN)
> +	    remote_vcont_probe (rs);
> +
> +	  if (remote_protocol_packets[PACKET_vCont].support == PACKET_DISABLE
> +	      || !rs->support_vCont.r)

If we check instead for PACKET_ENABLE && rs->support_vCont.r here,


> +	    {
> +	      use_range_stepping = 0;
> +	      error (_("Range stepping is not supported"));
> +	    }
> +	}
> +      else
> +	{
> +	  use_range_stepping = 0;
> +	  error (_("Range stepping is not supported"));
> +	}

Then we can merge these two errors.  Following up on the "willingness-only"
idea, I'd rather make this a warning though.

In v3, I've make moved the setting out of "maint".

-- 
Pedro Alves