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

["Schimpe, Christina" <christina.schimpe@intel.com>]

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