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

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

[Ciaran Woodward]

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).

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.
 There is no trailing NUL byte in a remote protocol packet; if the stub
 stores packets in a NUL-terminated format, it should allow an extra
 byte in its buffer for the NUL.  If this stub feature is not supported,
-- 
2.25.1

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

[Eli Zaretskii]

> From: Ciaran Woodward <ciaranwoodward@xmos.com>
> Cc: Ciaran Woodward <ciaranwoodward@xmos.com>
> Date: Mon, 22 Apr 2024 16:35:02 +0100
> 
> 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).
> 
> 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(-)

Thanks.  The patch is, of course, okay Texinfo-wise, but I'd like
someone who knows about the subject matter to confirm your
conclusions.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

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

[Luis Machado]

On 4/22/24 16:40, Eli Zaretskii wrote:
>> From: Ciaran Woodward <ciaranwoodward@xmos.com>
>> Cc: Ciaran Woodward <ciaranwoodward@xmos.com>
>> Date: Mon, 22 Apr 2024 16:35:02 +0100
>>
>> 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).
>>
>> 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(-)
> 
> Thanks.  The patch is, of course, okay Texinfo-wise, but I'd like
> someone who knows about the subject matter to confirm your
> conclusions.
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Looks correct to me. From gdbserver's definition of PBUFSIZ:

"...This value must be at least as large as the largest
 register set supported by gdbserver."

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

[Pedro Alves]

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>

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

[Ciaran Woodward]

> > 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.

Ah, yes - not sure where that '32' came from, since it seems like it
could guess the remote packet size too high. In combination with the
fact that many remotes won't return *all* of the registers in the g
response either, so it has 2 mechanisms to over-estimate.

Thankfully I don't think that really matters since modern remotes
should always specify PacketSize explicitly.

I think the rename is a good idea - I don’t know how I feel about
'fixing' things though, since the RSP is very sensitive to backwards
compatibility issues and its impossible to check all the potential
stubs. It looks like the remote_write_packet_aux underestimates
the size of the buffer at least, so we won't overflow any remote
server in the current state.