Fix gdbreplay checksum calculation
Checks
Commit Message
I needed to use gdbreplay today. It didn't work quite right, and
using "set debug remote 1" showed that gdb was rejecting some
responses like:
[remote] Sending packet: $vCont?#49
[remote] Junk: #vCont?
[remote] Junk: 8vCont?
[remote] Junk: 3vCont?
[remote] Received Ack
The checksum recalculation seems to have gone wrong. Looking at the
code, it seems like 'where_csum' is calculated inconsistently: in the
main loop it is after the '#' but in the "== 0" case it is before the
'#'.
This patch fixes the problem and also avoids a string copy.
CC: Alexandra Hájková <ahajkova@redhat.com>
---
gdbserver/gdbreplay.cc | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
Comments
On Mon, Dec 9, 2024 at 8:07 PM Tom Tromey <tromey@adacore.com> wrote:
> I needed to use gdbreplay today. It didn't work quite right, and
> using "set debug remote 1" showed that gdb was rejecting some
> responses like:
>
> [remote] Sending packet: $vCont?#49
> [remote] Junk: #vCont?
> [remote] Junk: 8vCont?
> [remote] Junk: 3vCont?
> [remote] Received Ack
>
> The checksum recalculation seems to have gone wrong. Looking at the
> code, it seems like 'where_csum' is calculated inconsistently: in the
> main loop it is after the '#' but in the "== 0" case it is before the
> '#'.
>
> This patch fixes the problem and also avoids a string copy.
>
> CC: Alexandra Hájková <ahajkova@redhat.com>
> ---
>
>
Hi Tom,
thank you for fixing this mistake of mine.
Tom Tromey <tromey@adacore.com> writes:
> I needed to use gdbreplay today. It didn't work quite right, and
> using "set debug remote 1" showed that gdb was rejecting some
> responses like:
>
> [remote] Sending packet: $vCont?#49
> [remote] Junk: #vCont?
> [remote] Junk: 8vCont?
> [remote] Junk: 3vCont?
> [remote] Received Ack
>
> The checksum recalculation seems to have gone wrong. Looking at the
> code, it seems like 'where_csum' is calculated inconsistently: in the
> main loop it is after the '#' but in the "== 0" case it is before the
> '#'.
>
> This patch fixes the problem and also avoids a string copy.
Thanks for fixing this:
Approved-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
>
> CC: Alexandra Hájková <ahajkova@redhat.com>
> ---
> gdbserver/gdbreplay.cc | 31 +++++++++++--------------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/gdbserver/gdbreplay.cc b/gdbserver/gdbreplay.cc
> index a9a55924ca9..951f50ea54e 100644
> --- a/gdbserver/gdbreplay.cc
> +++ b/gdbserver/gdbreplay.cc
> @@ -405,11 +405,12 @@ expect (FILE *fp)
> /* Calculate checksum for the packet stored in buffer buf. Store
> the checksum in a hexadecimal format in a checksum_hex variable. */
> static void
> -recalculate_csum (const std::string &buf, int cnt, unsigned char *checksum_hex)
> +recalculate_csum (const std::string &buf, int off, unsigned char *checksum_hex)
> {
> unsigned char csum = 0;
>
> - for (int i = 0; i < cnt; i++)
> + int len = buf.length ();
> + for (int i = off; i < len; ++i)
> csum += buf[i];
>
> checksum_hex[0] = tohex ((csum >> 4) & 0xf);
> @@ -435,9 +436,9 @@ play (FILE *fp)
> }
> while ((fromlog = logchar (fp, false)) != EOL)
> {
> - line.push_back (fromlog);
> - if (line[line.length ()] == '#')
> + if (fromlog == '#')
> where_csum = line.length ();
> + line.push_back (fromlog);
> }
>
> /* Packet starts with '+$' or '$', we don't want to calculate those
> @@ -446,23 +447,13 @@ play (FILE *fp)
> if (line[0] == '+')
> offset = 2;
>
> - /* If '#' is missing at the end of the line, add it and adjust the line
> - length. */
> - if (where_csum == 0)
> - {
> - where_csum = line.length ();
> - line.push_back ('#');
> - }
> - recalculate_csum (line.substr (offset), where_csum - offset, checksum);
> -
> - /* Check if the checksum is missing and adjust the line length to be able
> - to fit the checksum. */
> - if (where_csum + 1 >= line.length ())
> - line.resize (where_csum + 3);
> + if (where_csum > 0)
> + line.resize (where_csum);
> + recalculate_csum (line, offset, checksum);
>
> - /* Replace what is at the end of the packet with the checksum. */
> - line[where_csum + 1] = checksum[0];
> - line[where_csum + 2] = checksum[1];
> + line.push_back ('#');
> + line.push_back (checksum[0]);
> + line.push_back (checksum[1]);
>
> if (write (remote_desc_out, line.data (), line.size ()) != line.size ())
> remote_error ("Error during write to gdb");
> --
> 2.47.0
@@ -405,11 +405,12 @@ expect (FILE *fp)
/* Calculate checksum for the packet stored in buffer buf. Store
the checksum in a hexadecimal format in a checksum_hex variable. */
static void
-recalculate_csum (const std::string &buf, int cnt, unsigned char *checksum_hex)
+recalculate_csum (const std::string &buf, int off, unsigned char *checksum_hex)
{
unsigned char csum = 0;
- for (int i = 0; i < cnt; i++)
+ int len = buf.length ();
+ for (int i = off; i < len; ++i)
csum += buf[i];
checksum_hex[0] = tohex ((csum >> 4) & 0xf);
@@ -435,9 +436,9 @@ play (FILE *fp)
}
while ((fromlog = logchar (fp, false)) != EOL)
{
- line.push_back (fromlog);
- if (line[line.length ()] == '#')
+ if (fromlog == '#')
where_csum = line.length ();
+ line.push_back (fromlog);
}
/* Packet starts with '+$' or '$', we don't want to calculate those
@@ -446,23 +447,13 @@ play (FILE *fp)
if (line[0] == '+')
offset = 2;
- /* If '#' is missing at the end of the line, add it and adjust the line
- length. */
- if (where_csum == 0)
- {
- where_csum = line.length ();
- line.push_back ('#');
- }
- recalculate_csum (line.substr (offset), where_csum - offset, checksum);
-
- /* Check if the checksum is missing and adjust the line length to be able
- to fit the checksum. */
- if (where_csum + 1 >= line.length ())
- line.resize (where_csum + 3);
+ if (where_csum > 0)
+ line.resize (where_csum);
+ recalculate_csum (line, offset, checksum);
- /* Replace what is at the end of the packet with the checksum. */
- line[where_csum + 1] = checksum[0];
- line[where_csum + 2] = checksum[1];
+ line.push_back ('#');
+ line.push_back (checksum[0]);
+ line.push_back (checksum[1]);
if (write (remote_desc_out, line.data (), line.size ()) != line.size ())
remote_error ("Error during write to gdb");