Re: [PATCH] gdb/doc: Fix incorrect information in RSP doc

[Pedro Alves <pedro@palves.net>]

On 2024-04-22 16:35, Ciaran Woodward wrote:
> The 'PacketSize' attribute of the qSupported packet was
> documented to be the maximum size of the packet including
> the frame and checksum bytes, however this is not how it
> was treated in the code. In reality, PacketSize is the
> maximum size of the data in the RSP packets, not including
> the framing or checksum bytes.
> 
> For instance, GDB's remote.c treats it as the maximum
> number of data bytes.  See remote_read_bytes_1, where the
> size of the request is capped at PacketSize/2 (for
> hex-encoding).


OTOH, we have code like this:


  /* Should rsa->sizeof_g_packet needs more space than the
     default, adjust the size accordingly.  Remember that each byte is
     encoded as two characters.  32 is the overhead for the packet
     header / footer.  NOTE: cagney/1999-10-26: I suspect that 8
     (``$NN:G...#NN'') is a better guess, the below has been padded a
     little.  */
  if (this->sizeof_g_packet > ((this->remote_packet_size - 32) / 2))
    this->remote_packet_size = (this->sizeof_g_packet * 2 + 32);


and this:

 remote_target::remote_write_bytes_aux ()
 ...
   payload_capacity_bytes = get_memory_write_packet_size ();
 
   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */
   rs->buf[0] = '\0';
 
   /* Compute the size of the actual payload by subtracting out the
      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */
 
   payload_capacity_bytes -= strlen ("$,:#NN");


So looks like we have a mess?  Most code in remote.c seems to assume
get_remote_packet_size() returns the max payload size, except, not always.

I think it would be good to rename remote_packet_size to remote_packet_data_size
or remote_packet_payload_size and similarly rename the few functions around this,
like get_remote_packet_size and get_memory_write_packet_size, and fix those
cases above accordingly.

> 
> Also see gdbserver's server.cc, where the internal buffer
> is sized as PBUFSIZ and PBUFSIZ-1 is used as PacketSize.
> In gdbserver's case, the buffer is not used for any of the
> framing or checksum characters. (I am not certain where the -1
> comes from. I think it comes from back when there were no
> binary packets, so packets were treated as strings with
> null terminators).
> 
> It also seems like gdbservers in the wild treat it in
> this way:
> 
> Embocosm doc:
> https://www.embecosm.com/appnotes/ean4/embecosm-howto-rsp-server-ean4-issue-2.html#id3078000
> 
> A quick glance over openocd's gdb_server.c gdb_put_packet_inner()
> function shows that the internal buffer also excludes the framing
> and checksum.
> 
> Likewise, qEmu's gdbstub.c allocates PacketSize bytes for
> the internal packet contents, and PacketSize+4 for the
> full frame.
> ---
>  gdb/doc/gdb.texinfo | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 31a531ee992..b2e9faac82d 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -44953,7 +44953,7 @@ These are the currently defined stub features, in more detail:
>  The remote stub can accept packets up to at least @var{bytes} in
>  length.  @value{GDBN} will send packets up to this size for bulk
>  transfers, and will never send larger packets.  This is a limit on the
> -data characters in the packet, including the frame and checksum.
> +data characters in the packet, not including the frame and checksum.

Yes, to me, this just looks like a typo.  "data characters in the packet" reads to me
like talking about what the docs refer to as packet-data (i.e., the payload), and the
"not including the frame and checksum." would be just there to try say redundant things to
be clear.  

The sentence just doesn't make sense as is -- the frame and checksum are not
part of the data characters, so "including" is a strange word to use.  If it really was
trying to say the whole set of characters in the packet, it would have been phrased
differently IMO, like saying "plus" instead of "including, perhaps.

With the "not", it now serves the redundancy purpose I'm referring.


Approved-By: Pedro Alves <pedro@palves.net>