Re: [PATCH v3 1/3] gdb: Make global feature array a per-remote target array

[Pedro Alves <pedro@palves.net>]

Hi Christina,

Other than a couple tiny nits, just one issue with the data structures below.  I think once this is resolved,
it'll be ready to go.  Thanks for the patience.

On 2022-09-09 4:37 p.m., Christina Schimpe wrote:
>  
> +/* Convert the packet support auto_boolean to a name used for gdb printing.  */
> +
> +static const char *
> +get_packet_support_name (auto_boolean support)
> +{
> +  switch (support)
> +    {
> +      case AUTO_BOOLEAN_TRUE:
> +	return _("on");
> +      case AUTO_BOOLEAN_FALSE:
> +	return _("off");
> +      case AUTO_BOOLEAN_AUTO:
> +	return _("auto");

These shouldn't actually be translated, as GDB only accepts these in English.  Like, in

  (gdb) set foo off

"off" is always "off".

> +      default:
> +	return _("internal-error");

I'd just remove this one.

> +
>  struct threads_listing_context;
>  
>  /* Stub vCont actions support.
> @@ -395,6 +574,90 @@ static const target_info remote_target_info = {
>    remote_doc
>  };
>  
> +/* Description and configuration of a remote packet.  */
> +
> +struct packet_config
> +  {
> +    /* Name of the packet used for gdb output.  */
> +    const char *name;
> +
> +    /* Title of the packet, used by the set/show remote name-packet
> +       commands to identify the individual packages and gdb output.  */
> +    const char *title;
> +
> +    /* If auto, GDB auto-detects support for this packet or feature,
> +       either through qSupported, or by trying the packet and looking
> +       at the response.  If true, GDB assumes the target supports this
> +       packet.  If false, the packet is disabled.  Configs that don't
> +       have an associated command always have this set to auto.  */
> +    enum auto_boolean detect;
> +
> +    /* The "show remote foo-packet" command created for this packet.  */
> +    cmd_list_element *show_cmd;
> +
> +    /* The "set remote foo-packet" command created for this packet.  */
> +    cmd_list_element *set_cmd;
> +
> +    /* Does the target support this packet?  */
> +    enum packet_support support;
> +  };

So this structure is now a bit odd.  It holds both packet description stuff, like
the name/title, which is shared between all the remote connections; and configuration
stuff, which is per-connection.  I think we should split this in two structs / arrays, so
we only have one copy of the description stuff.

In fact, aren't show_cmd/set_cmd above essentially unused after this patch?
I can only see places writing to those fields, I see nowhere actually
reading from those fields.  I think you can just drop them entirely.

> +
> +/* This global array contains the default configuration for every new
> +   per-remote target array.  */
> +static packet_config remote_protocol_packets[PACKET_MAX];


> @@ -1967,10 +2256,13 @@ add_packet_config_cmd (struct packet_config *config, const char *name,
>      = add_setshow_auto_boolean_cmd (cmd_name.release (), class_obscure,
>  				    &config->detect, set_doc.get (),
>  				    show_doc.get (), NULL, /* help_doc */
> -				    NULL,
> +				    set_remote_protocol_packet_cmd,
>  				    show_remote_protocol_packet_cmd,
>  				    &remote_set_cmdlist, &remote_show_cmdlist);
>    config->show_cmd = cmds.show;
> +  config->set_cmd = cmds.set;

These two above, are the only references to config->show_cmd / config->set_cmd AFAICT.

> +  cmds.show->set_context(config);
> +  cmds.set->set_context(config);

Missing space before parens in those two set_context calls.

> @@ -2273,19 +2401,24 @@ show_remote_protocol_packet_cmd (struct ui_file *file, int from_tty,
>  				 struct cmd_list_element *c,
>  				 const char *value)
>  {
> -  struct packet_config *packet;
> +  remote_target *remote = get_current_remote_target ();
>    gdb_assert (c->var.has_value ());
>  
> -  for (packet = remote_protocol_packets;
> -       packet < &remote_protocol_packets[PACKET_MAX];
> -       packet++)
> +  auto *default_config = static_cast<packet_config *> (c->context ());
> +  const int packet_idx = std::distance (remote_protocol_packets,
> +					default_config);
> +
> +  if (packet_idx >= 0 && packet_idx  < PACKET_MAX)

Spurious double space in "packet_idx  <".

>      {
> -      if (c == packet->show_cmd)
> -	{
> -	  show_packet_config_cmd (file, packet);
> -	  return;
> -	}
> +       if (remote != nullptr)
> +	 show_packet_config_cmd
> +	   (file, &remote->m_features.m_protocol_packets[packet_idx], true);
> +       else
> +	 show_packet_config_cmd
> +	   (file, &remote_protocol_packets[packet_idx], false);
> +       return;
>      }
> +
>    internal_error (__FILE__, __LINE__, _("Could not find config for %s"),
>  		  c->name);
>  }