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

Pedro Alves
2022-05-25T14:27:31Z

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)