* [PATCH] gdb/doc: Fix incorrect information in RSP doc @ 2024-04-22 15:35 Ciaran Woodward 2024-04-22 15:40 ` Eli Zaretskii 2024-04-23 18:58 ` Pedro Alves 0 siblings, 2 replies; 5+ messages in thread From: Ciaran Woodward @ 2024-04-22 15:35 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb/doc: Fix incorrect information in RSP doc 2024-04-22 15:35 [PATCH] gdb/doc: Fix incorrect information in RSP doc Ciaran Woodward @ 2024-04-22 15:40 ` Eli Zaretskii 2024-04-23 16:04 ` Luis Machado 2024-04-23 18:58 ` Pedro Alves 1 sibling, 1 reply; 5+ messages in thread From: Eli Zaretskii @ 2024-04-22 15:40 UTC (permalink / raw) To: Ciaran Woodward; +Cc: gdb-patches > 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb/doc: Fix incorrect information in RSP doc 2024-04-22 15:40 ` Eli Zaretskii @ 2024-04-23 16:04 ` Luis Machado 0 siblings, 0 replies; 5+ messages in thread From: Luis Machado @ 2024-04-23 16:04 UTC (permalink / raw) To: Eli Zaretskii, Ciaran Woodward; +Cc: gdb-patches 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." ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb/doc: Fix incorrect information in RSP doc 2024-04-22 15:35 [PATCH] gdb/doc: Fix incorrect information in RSP doc Ciaran Woodward 2024-04-22 15:40 ` Eli Zaretskii @ 2024-04-23 18:58 ` Pedro Alves 2024-04-24 16:28 ` Ciaran Woodward 1 sibling, 1 reply; 5+ messages in thread From: Pedro Alves @ 2024-04-23 18:58 UTC (permalink / raw) To: Ciaran Woodward, gdb-patches 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] gdb/doc: Fix incorrect information in RSP doc 2024-04-23 18:58 ` Pedro Alves @ 2024-04-24 16:28 ` Ciaran Woodward 0 siblings, 0 replies; 5+ messages in thread From: Ciaran Woodward @ 2024-04-24 16:28 UTC (permalink / raw) To: Pedro Alves, gdb-patches > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-24 16:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-22 15:35 [PATCH] gdb/doc: Fix incorrect information in RSP doc Ciaran Woodward 2024-04-22 15:40 ` Eli Zaretskii 2024-04-23 16:04 ` Luis Machado 2024-04-23 18:58 ` Pedro Alves 2024-04-24 16:28 ` Ciaran Woodward
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).