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

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

[Schimpe, Christina]

Kindly pinging.

Thanks,
Christina

> -----Original Message-----
> From: Schimpe, Christina
> Sent: Wednesday, May 18, 2022 12:24 PM
> To: 'Pedro Alves' <pedro@palves.net>; 'Andrew Burgess'
> <aburgess@redhat.com>; 'gdb-patches@sourceware.org' <gdb-
> patches@sourceware.org>
> Subject: RE: [PING^2] [PATCH v2 1/3] gdb: Make global feature array a per-
> remote target array
> 
> Kindly pinging.
> 
> Thanks,
> Christina
> 
> > -----Original Message-----
> > From: Schimpe, Christina
> > Sent: Wednesday, May 11, 2022 9:31 AM
> > To: 'Pedro Alves' <pedro@palves.net>; 'Andrew Burgess'
> > <aburgess@redhat.com>; 'gdb-patches@sourceware.org' <gdb-
> > patches@sourceware.org>
> > Subject: RE: [PING] [PATCH v2 1/3] gdb: Make global feature array a
> > per- remote target array
> >
> > Kindly pinging for thoughts.
> >
> > Thanks,
> > Christina
> >
> > > -----Original Message-----
> > > From: Schimpe, Christina
> > > Sent: Wednesday, April 27, 2022 3:55 PM
> > > To: Pedro Alves <pedro@palves.net>; 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 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".
> > >
> > >
> > > 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 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.
> > > (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.
> > > ~~~~
> > >
> > > 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.
> > >
> > > >
> > > > > +/* 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.
> > > >
> > >
> > > Ah yes, I will adapt it in the v3, thanks.
> > >
> > >
> > > 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