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

[Schimpe, Christina]

Hi Pedro, 

Thanks for the review. 

I will go ahead now and work on a v3 of this series with the new suggestion for the
"set remote foo" packet configuration  and the logging suggested in the email thread
below.

Best Regards,
Christina

> -----Original Message-----
> From: Pedro Alves <pedro@palves.net>
> Sent: Wednesday, May 25, 2022 4:27 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; Andrew Burgess
> <aburgess@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 1/3] gdb: Make global feature array a per-remote target
> array
> 
> Hi Christina,
> 
> On 2022-04-27 14:55, Schimpe, Christina wrote:
> > Hi Pedro and Andrew,
> >
> > Thanks a lot for identifying and fixing the regression and thank you for the
> review.
> >
> > I added my comments for Pedro's review below.  It would be great if
> > you could briefly review my new suggestion for the logging.
> >
> >
> >> 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.
> >>
> >
> > I think the reason why I did not investigate this approach further is because
> these "set remote"
> > commands applied to future connections before and  I did not want to change
> that.
> > My initial patch did not add any logging for the set remote commands
> > and the user would not have noticed that the configuration does not apply to
> future targets anymore (if connected).
> > But with the appropriate logging it should be clear and the user
> > should be warned.  So I don't have any preferences anymore and would
> > go ahead to adapt the patch to Pedro's suggestion, if there are no further
> arguments against it.
> >
> >> 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.
> >>
> >
> > Yes, you are right.
> >
> > So I now would suggest the following logging for the packet
> > configuration commands (based on Pedro's suggestion for the new packet
> configuration):
> >
> > ~~~
> > (gdb) set remote kill-packet
> > "on", "off" or "auto" expected.
> > (gdb) show remote kill-packet
> > Support for the 'vKill' packet on future remote targets is "auto", currently
> unknown.
> > (gdb) set remote kill-packet off
> > Support for the 'vKill' packet on future remote targets is set to "off".
> > (gdb) show remote kill-packet
> > Support for the 'vKill' packet on future remote targets is "off".
> > (gdb) target extended-remote :1234
> > Remote debugging using :1234
> > (gdb) set remote kill-packet on
> > Support for the 'vKill' packet on the current remote target is set to "on".
> > (gdb) show remote kill-packet
> > Support for the 'vKill' packet on the current remote target is "on".
> > ~~~~
> > So the only difference between the logging for the show and the set
> > commands, is that for set we log "is set to" instead of "is".
> >
> 
> Seems fine.
> 
> >
> > And for the memory read and write configuration of patch (2/3):
> >
> > ~~~~
> > (gdb) set remote memory-read-packet-size Argument required (integer,
> > "fixed" or "limited').
> > (gdb) show remote memory-read-packet-size The memory-read-packet-size
> > on future remote targets is 0 (default). The actual limit will be further reduced
> dependent on the target.
> > (gdb) set remote memory-read-packet-size fixed Future targets may not
> > be able to correctly handle a memory-read-packet-size
> 
> You say "future remote targets" in the other messages, so I guess here you'd say
> "Future remote targets" instead of "Future targets" too.
> 
> > of 16384 bytes.  Change the packet size for future remote targets? (y
> > or n) y The memory-read-packet-size on future remote targets is set to "fixed".
> > (gdb) show remote memory-read-packet-size The memory-read-packet-size
> > on future remote targets is 0 (default). Packets are fixed at 16384 bytes.
> > (gdb) target extended-remote :1234
> > Remote debugging using :1234
> > (gdb) set remote memory-read-packet-size 16300 The
> > memory-read-packet-size on the current remote targets is set to 16300.
> 
> "remote targets" -> "remote target" (singular).
> 
> > (gdb) show remote memory-read-packet-size The memory-read-packet-size
> > on the current remote target is 16300. Packets are fixed at 16300 bytes.
> > ~~~~
> >
> 
> Seems all fine to me.
> 
> > Note that the configuration options before were shown as ~~~~
> > (gdb) set remote memory-read-packet-size Argument required (integer,
> > `fixed' or `limited').
> > ~~~~
> >
> > I also noted a small issue in the configuration for the "limited" option:
> > ~~~~
> > (gdb) set remote memory-read-packet-size Argument required (integer,
> > `fixed' or `limited').
> > (gdb) set remote memory-read-packet-size limited Invalid
> > memory-read-packet-size (bad syntax).
> > (gdb) set remote memory-read-packet-size limit
> > (gdb)
> > ~~~~
> > So currently you need to specify "limit" although "limited" is suggested. I
> would adapt it to "limited" in the v3.
> 
> Sounds like that may break someone's scripts?  I'd go for tweaking the
> suggestion to say "limit" instead.
> 
> I went to look what does the manual say, but I wasn't able to find where this
> comment is documented in the manual...  Odd.
> 
> The online help does say "limit", though:
> 
>  (gdb) help set remote memory-read-packet-size  Set the maximum number of
> bytes per memory-read packet.
>  Specify the number of bytes in a packet or 0 (zero) for the  default packet size.
> The actual limit is further reduced  dependent on the target.  Specify ``fixed'' to
> disable the  further restriction and ``limit'' to enable that restriction.
>  (gdb)
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