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

[Pedro Alves <pedro@palves.net>]

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)