[v4,3/3] gdb: Remove workaround for the vCont packet

  The workaround for the vCont packet is no longer required due to the
former commit "gdb: Make global feature array a per-remote target array".
The vCont packet is now checked once when the connection is started and
the supported vCont actions are set to the target's remote state
attribute.
---
 gdb/remote.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)
  

On 2022-12-21 1:39 p.m., Christina Schimpe wrote:
> The workaround for the vCont packet is no longer required due to the
> former commit "gdb: Make global feature array a per-remote target array".
> The vCont packet is now checked once when the connection is started and
> the supported vCont actions are set to the target's remote state
> attribute.

So this patch is actually doing two things:

 #1 - removing the supports_vCont_probed hack.

 #2 - moving the vCont probing earlier, to connection start.

To eliminate the workaround, you don't need #2.  Doing just #1 would simply get us back
to how it worked before the multi-target support.  #2 is an extra change.

I don't think there's any particular reason for deferring probing for vCont
support up until we first try to resume.  Looking at the history, we find that
vCont was designed in 2003, here:

  https://marc.info/?l=gdb-patches&w=2&r=30&s=vcont&q=b

and particularly "vCont?" probing was discussed here:

  https://marc.info/?l=gdb-patches&m=158570765842635&w=2

while the qSupported generic probing feature (done at connection startup) was only
invented later in 2006:

  https://sourceware.org/legacy-ml/gdb/2006-05/msg00372.html

Even without qSupported (otherwise I bet we wouldn't have vCont? but instead the support
would have been reported via qSupported), it's not clear why Daniel didn't do the probing at
initial connection time, I didn't find any message specifically about that.  Likely it was
written that way just to keep all the related code together, I guess.

Thus, the patch is OK.  But I wanted to explicitly state the two unrelated changes here
for the record, and dig a bit at the history to figure out whether we'd be breaking
some subtle use case.

Thanks for this.  Happy to see it finally done.

Pedro Alves

> ---
>  gdb/remote.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8042446ac3d..71ba7da231a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -455,9 +455,6 @@ class remote_state
>  
>    /* The status of the stub support for the various vCont actions.  */
>    vCont_action_support supports_vCont;
> -  /* Whether vCont support was probed already.  This is a workaround
> -     until packet_support is per-connection.  */
> -  bool supports_vCont_probed;
>  
>    /* True if the user has pressed Ctrl-C, but the target hasn't
>       responded to that.  */
> @@ -4951,6 +4948,10 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
>       which later probes to skip.  */
>    remote_query_supported ();
>  
> +  /* Check vCont support and set the remote state's vCont_action_support
> +     attribute.  */
> +  remote_vcont_probe ();
> +
>    /* If the stub wants to get a QAllow, compose one and send it.  */
>    if (m_features.packet_support (PACKET_QAllow) != PACKET_DISABLE)
>      set_permissions ();
> @@ -6467,7 +6468,6 @@ remote_target::remote_vcont_probe ()
>      }
>  
>    m_features.packet_ok (rs->buf, PACKET_vCont);
> -  rs->supports_vCont_probed = true;
>  }
>  
>  /* Helper function for building "vCont" resumptions.  Write a
> @@ -6653,9 +6653,6 @@ remote_target::remote_resume_with_vcont (ptid_t scope_ptid, int step,
>    if (::execution_direction == EXEC_REVERSE)
>      return 0;
>  
> -  if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
> -    remote_vcont_probe ();
> -
>    if (m_features.packet_support (PACKET_vCont) == PACKET_DISABLE)
>      return 0;
>  
> @@ -7189,12 +7186,6 @@ remote_target::remote_stop_ns (ptid_t ptid)
>  	  }
>        }
>  
> -  /* FIXME: This supports_vCont_probed check is a workaround until
> -     packet_support is per-connection.  */
> -  if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN
> -      || !rs->supports_vCont_probed)
> -    remote_vcont_probe ();
> -
>    if (!rs->supports_vCont.t)
>      error (_("Remote server does not support stopping threads"));
>  
> @@ -14516,9 +14507,6 @@ remote_target::can_do_single_step ()
>      {
>        struct remote_state *rs = get_remote_state ();
>  
> -      if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
> -	remote_vcont_probe ();
> -
>        return rs->supports_vCont.s && rs->supports_vCont.S;
>      }
>    else
> @@ -14827,9 +14815,6 @@ show_range_stepping (struct ui_file *file, int from_tty,
>  bool
>  remote_target::vcont_r_supported ()
>  {
> -  if (m_features.packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
> -    remote_vcont_probe ();
> -
>    return (m_features.packet_support (PACKET_vCont) == PACKET_ENABLE
>  	  && get_remote_state ()->supports_vCont.r);
>  }
>
  

Hi Pedro,

> On 2022-12-21 1:39 p.m., Christina Schimpe wrote:
> > The workaround for the vCont packet is no longer required due to the
> > former commit "gdb: Make global feature array a per-remote target array".
> > The vCont packet is now checked once when the connection is started
> > and the supported vCont actions are set to the target's remote state
> > attribute.
> 
> So this patch is actually doing two things:
> 
>  #1 - removing the supports_vCont_probed hack.
> 
>  #2 - moving the vCont probing earlier, to connection start.
> 
> To eliminate the workaround, you don't need #2.  Doing just #1 would simply get
> us back to how it worked before the multi-target support.  #2 is an extra change.
> 
> I don't think there's any particular reason for deferring probing for vCont
> support up until we first try to resume.  Looking at the history, we find that
> vCont was designed in 2003, here:
> 
>   https://marc.info/?l=gdb-patches&w=2&r=30&s=vcont&q=b
> 
> and particularly "vCont?" probing was discussed here:
> 
>   https://marc.info/?l=gdb-patches&m=158570765842635&w=2
> 
> while the qSupported generic probing feature (done at connection startup) was
> only invented later in 2006:
> 
>   https://sourceware.org/legacy-ml/gdb/2006-05/msg00372.html
> 
> Even without qSupported (otherwise I bet we wouldn't have vCont? but instead
> the support would have been reported via qSupported), it's not clear why Daniel
> didn't do the probing at initial connection time, I didn't find any message
> specifically about that.  Likely it was written that way just to keep all the related
> code together, I guess.
> 
> Thus, the patch is OK.  But I wanted to explicitly state the two unrelated changes
> here for the record, and dig a bit at the history to figure out whether we'd be
> breaking some subtle use case.
> 
> Thanks for this.  Happy to see it finally done.
> 
> Pedro Alves

Thank you for the investigation, I pushed the patch now.

Best Regards,
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928