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

[Pedro Alves <pedro@palves.net>]

On 2022-03-29 14:11, Christina Schimpe via Gdb-patches wrote:
> This patch applies the appropriate FIXME notes described in commit 5b6d1e4
> "Multi-target support".
> 
> "You'll notice that remote.c includes some FIXME notes.  These refer to
> the fact that the global arrays that hold data for the remote packets
> supported are still globals.  For example, if we connect to two
> different servers/stubs, then each might support different remote
> protocol features.  They might even be different architectures, like
> e.g., one ARM baremetal stub, and a x86 gdbserver, to debug a
> host/controller scenario as a single program.  That isn't going to
> work correctly today, because of said globals.  I'm leaving fixing
> that for another pass, since it does not appear to be trivial, and I'd
> rather land the base work first.  It's already useful to be able to
> debug multiple instances of the same server (e.g., a distributed
> cluster, where you have full control over the servers installed), so I
> think as is it's already reasonable incremental progress."
> 
> Using this patch it is possible to configure per-remote targets'
> feature packets.

Thanks for working on this.  And so sorry for coming in late!  I wish I had gotten
involved in this discussion sooner.

I'm not 100% sure about the solution here.

What is the reasoning for making "set remote foo" affect future
connections in addition to the current connection?  I didn't see a discussion about
that, and it seems counterintuitive to me offhand.  I would think that:

 - if connected, the set command affects the current connection.

 - if not connected, the set command affects future connections.

... would be the simplest solution.  Thus, if the inferior you have selected is connected
and you want to configure future connections, you'd first drop the connection, or switch
to an inferior that is not connected.

It seems odd that set affects both current and future connections, while show only mentions the
state about the current connection, for instance.

Also, I think it would be better if both the set and the show commands used the
same wording.  Currently you have, when not connected:

 (gdb) set remote X-packet off 
 Use of the 'p' packet for future remote targets is set to "off".
 (gdb) show remote X-packet 
 Support for the 'p' packet on newly created remote targets is "disabled".

Note the
 "Use of" vs "Support for", 
and the
 "for future remote targets", vs "on newly created remote targets".

Also note that "disabled" is not accepted by the "set command", while printing
it in quotes suggests that it would.  I mean, note:

Current master:

 (gdb) show remote X-packet
 Support for the `p' packet is currently disabled.

(no quotes around disabled)

vs your patches:

 (gdb) show remote X-packet 
 Support for the 'p' packet on newly created remote targets is "disabled".

and of course:

 (gdb) set remote X-packet disabled
 "on", "off" or "auto" expected.


Also, BTW, err, why do we talk about the 'p' packet if the command is about
the X packet?  That's busted.  Seems like a preexisting issue in current master.

> +/* Convert the target type (future remote target or currently connected target)
> +   to a name used for gdb printing.  */
> +
> +static const char *
> +get_target_type_name (bool target_connected)
> +{
> +  if (target_connected)
> +    return "on the current remote target";
> +  else
> +    return "on newly created remote targets";
> +}

Note this is not i18n friendly.

Pedro Alves