gdbreplay: Add --debug-logging option

Message ID 20241004185732.153001-1-ahajkova@redhat.com
State New
Headers
Series gdbreplay: Add --debug-logging option |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Alexandra Petlanova Hajkova Oct. 4, 2024, 6:57 p.m. UTC
  As gdbreplay communicates with GDB, it outputs all the remote
protocol communication it reads from the remotelogfile to stderr.
This patch disables this behavior by default but adds the new
--debug-logging option which turns printing the packets
to stderr on again.

The motivation for this change is to make it possible to use
gdbreplay with TCL tests. Printing the whole remotelog file out
seems to overflow the expect cache wich causes gdbreplay to not
to get the packet its expects and results in going out of sync
with GDB. Other motivation is making communication between GDB
and gdbreplay faster as printing bigger remotelogfile takes
considerable amount of time.
---
 gdbserver/README       |  11 ++---
 gdbserver/gdbreplay.cc | 100 +++++++++++++++++++++++++++++------------
 2 files changed, 78 insertions(+), 33 deletions(-)
  

Comments

Tom Tromey Oct. 4, 2024, 7:33 p.m. UTC | #1
>>>>> "Alexandra" == Alexandra Hájková <ahajkova@redhat.com> writes:

Alexandra> As gdbreplay communicates with GDB, it outputs all the remote
Alexandra> protocol communication it reads from the remotelogfile to stderr.

Yeah.  I think this is done primarily so the 'c' lines are echoed, so
you can see what to type next.  Not really sure though.

I see you left these enabled by default.

Alexandra> The motivation for this change is to make it possible to use
Alexandra> gdbreplay with TCL tests.

Sounds intriguing.  Like, how would this be useful?

Alexandra> +bool debug_logging = 0;
 
 = false

Alexandra> -  if (argc >= 2 && strcmp (argv[1], "--version") == 0)
Alexandra> +  while (*next_arg != NULL && **next_arg == '-')
Alexandra>      {
Alexandra> -      gdbreplay_version ();
Alexandra> -      exit (0);
Alexandra> -    }
Alexandra> -  if (argc >= 2 && strcmp (argv[1], "--help") == 0)
Alexandra> -    {
Alexandra> -      gdbreplay_usage (stdout);
Alexandra> -      exit (0);
Alexandra> +      if (argc >= 3 && strcmp (*next_arg, "--version") == 0)
Alexandra> +	{
Alexandra> +	  gdbreplay_version ();
Alexandra> +	  exit (0);
Alexandra> +	}

I don't understand the ">= 3" here.

On my list for years now has been to convert gdbserver (and gdbreplay)
to getopt_long.

Alexandra> +	debug_logging = 1;

 = true

Tom
  
Alexandra Petlanova Hajkova Oct. 4, 2024, 7:59 p.m. UTC | #2
On Fri, Oct 4, 2024 at 9:33 PM Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Alexandra" == Alexandra Hájková <ahajkova@redhat.com> writes:
>
> Alexandra> As gdbreplay communicates with GDB, it outputs all the remote
> Alexandra> protocol communication it reads from the remotelogfile to
> stderr.
>
> Yeah.  I think this is done primarily so the 'c' lines are echoed, so
> you can see what to type next.  Not really sure though.
>
> I see you left these enabled by default.
>
> Alexandra> The motivation for this change is to make it possible to use
> Alexandra> gdbreplay with TCL tests.
>
> Sounds intriguing.  Like, how would this be useful?
>

I wrote a TCL test which starts a communication with gdbsever setting the
remotelog file.
Then, it modifies the remotelog, injects an error message instead of the
expected replay
 to the vMustReplyEmpty packet in order  to test GDB reacts to the error
response properly.
 After the remotelog modification, this test restarts GDB and starts
communication with gdbreply
 instead of the gdbserver using the remotelog. A similar test could be
written with any other packet.
We know that when gdbreplay is communicating with GDB and is interrupted
with Ctrl-C, GDB crashes
in some cases - depending on which packet is going out. I'd like to start
solving these crashes. This test
would be a great way how to test such problems was really fixed and also I
think, it would be great to
test this way any new packet which is being added to the remote protocol.

>
>
  
Tom Tromey Oct. 4, 2024, 8:02 p.m. UTC | #3
>>>>> "Alexandra" == Alexandra Petlanova Hajkova <ahajkova@redhat.com> writes:

Alexandra> Then, it modifies the remotelog, injects an error message
Alexandra> instead of the expected replay to the vMustReplyEmpty packet
Alexandra> in order to test GDB reacts to the error response properly.

That's awesome.

Tom
  

Patch

diff --git a/gdbserver/README b/gdbserver/README
index 5b47510c3e3..e7e99ec7988 100644
--- a/gdbserver/README
+++ b/gdbserver/README
@@ -125,8 +125,9 @@  Then start GDB (preferably in a different screen or window) and use the
 
 Repeat the same sequence of user commands to GDB that you gave in the
 original debug session.  GDB should not be able to tell that it is talking
-to GDBreplay rather than a real target, all other things being equal.  Note
-that GDBreplay echos the command lines to stderr, as well as the contents of
-the packets it sends and receives.  The last command echoed by GDBreplay is
-the next command that needs to be typed to GDB to continue the session in
-sync with the original session.
+to GDBreplay rather than a real target, all other things being equal.
+
+As GDBreplay communicates with GDB, it outputs only the commands
+it expects from GDB. The --debug-logging option turns printing the
+remotelogfile to stderr on. GDBreplay then echos the command lines
+to stderr, as well as the contents of the packets it sends and receives.
diff --git a/gdbserver/gdbreplay.cc b/gdbserver/gdbreplay.cc
index c2359e4ab43..cc8dc22c0fb 100644
--- a/gdbserver/gdbreplay.cc
+++ b/gdbserver/gdbreplay.cc
@@ -66,6 +66,7 @@  typedef int socklen_t;
 
 static int remote_desc_in;
 static int remote_desc_out;
+bool debug_logging = 0;
 
 static void
 sync_error (FILE *fp, const char *desc, int expect, int got)
@@ -259,13 +260,13 @@  remote_open (const char *name)
 }
 
 static int
-logchar (FILE *fp)
+logchar (FILE *fp, bool print)
 {
   int ch;
   int ch2;
 
   ch = fgetc (fp);
-  if (ch != '\r')
+  if (ch != '\r' && (print || debug_logging))
     {
       fputc (ch, stderr);
       fflush (stderr);
@@ -282,16 +283,22 @@  logchar (FILE *fp)
 	  ungetc (ch, fp);
 	  ch = '\r';
 	}
-      fputc (ch == EOL ? '\n' : '\r', stderr);
-      fflush (stderr);
+      if (print || debug_logging)
+	{
+	  fputc (ch == EOL ? '\n' : '\r', stderr);
+	  fflush (stderr);
+	}
       break;
     case '\n':
       ch = EOL;
       break;
     case '\\':
       ch = fgetc (fp);
-      fputc (ch, stderr);
-      fflush (stderr);
+      if (print || debug_logging)
+	{
+	  fputc (ch, stderr);
+	  fflush (stderr);
+	}
       switch (ch)
 	{
 	case '\\':
@@ -316,14 +323,28 @@  logchar (FILE *fp)
 	  break;
 	case 'x':
 	  ch2 = fgetc (fp);
-	  fputc (ch2, stderr);
-	  fflush (stderr);
+	  if (print || debug_logging)
+	    {
+	      fputc (ch2, stderr);
+	      fflush (stderr);
+	    }
 	  ch = fromhex (ch2) << 4;
 	  ch2 = fgetc (fp);
-	  fputc (ch2, stderr);
-	  fflush (stderr);
+	  if (print || debug_logging)
+	    {
+	      fputc (ch2, stderr);
+	      fflush (stderr);
+	    }
 	  ch |= fromhex (ch2);
 	  break;
+	case 'c':
+	  fputc (ch, stderr);
+	  fflush (stderr);
+	  break;
+	case 'E':
+	  fputc (ch, stderr);
+	  fflush (stderr);
+	  break;
 	default:
 	  /* Treat any other char as just itself */
 	  break;
@@ -354,14 +375,14 @@  expect (FILE *fp)
   int fromlog;
   int fromgdb;
 
-  if ((fromlog = logchar (fp)) != ' ')
+  if ((fromlog = logchar (fp, false)) != ' ')
     {
       sync_error (fp, "Sync error during gdb read of leading blank", ' ',
 		  fromlog);
     }
   do
     {
-      fromlog = logchar (fp);
+      fromlog = logchar (fp, false);
       if (fromlog == EOL)
 	break;
       fromgdb = gdbchar (remote_desc_in);
@@ -386,12 +407,12 @@  play (FILE *fp)
   int fromlog;
   char ch;
 
-  if ((fromlog = logchar (fp)) != ' ')
+  if ((fromlog = logchar (fp, false)) != ' ')
     {
       sync_error (fp, "Sync error skipping blank during write to gdb", ' ',
 		  fromlog);
     }
-  while ((fromlog = logchar (fp)) != EOL)
+  while ((fromlog = logchar (fp, false)) != EOL)
     {
       ch = fromlog;
       if (write (remote_desc_out, &ch, 1) != 1)
@@ -426,16 +447,27 @@  captured_main (int argc, char *argv[])
 {
   FILE *fp;
   int ch;
+  char **next_arg = &argv[1];
 
-  if (argc >= 2 && strcmp (argv[1], "--version") == 0)
+  while (*next_arg != NULL && **next_arg == '-')
     {
-      gdbreplay_version ();
-      exit (0);
-    }
-  if (argc >= 2 && strcmp (argv[1], "--help") == 0)
-    {
-      gdbreplay_usage (stdout);
-      exit (0);
+      if (argc >= 3 && strcmp (*next_arg, "--version") == 0)
+	{
+	  gdbreplay_version ();
+	  exit (0);
+	}
+
+      else if (argc >= 3 && strcmp (*next_arg, "--help") == 0)
+	{
+	  gdbreplay_usage (stdout);
+	  exit (0);
+	}
+      else if (argc == 4 && strcmp (*next_arg, "--debug-logging") == 0)
+	debug_logging = 1;
+      else if (argc == 4)
+	  fprintf (stderr, "Invalid option %s\n", *next_arg);
+
+      next_arg++;
     }
 
   if (argc < 3)
@@ -443,13 +475,14 @@  captured_main (int argc, char *argv[])
       gdbreplay_usage (stderr);
       exit (1);
     }
-  fp = fopen (argv[1], "r");
+  fp = fopen (*next_arg, "r");
   if (fp == NULL)
     {
-      perror_with_name (argv[1]);
+      perror_with_name (*next_arg);
     }
-  remote_open (argv[2]);
-  while ((ch = logchar (fp)) != EOF)
+  next_arg++;
+  remote_open (*next_arg);
+  while ((ch = logchar (fp, false)) != EOF)
     {
       switch (ch)
 	{
@@ -462,10 +495,21 @@  captured_main (int argc, char *argv[])
 	  play (fp);
 	  break;
 	case 'c':
-	  /* Command executed by gdb */
-	  while ((ch = logchar (fp)) != EOL);
+	  /* We want to always print the command executed by GDB. */
+	  if (!debug_logging)
+	    {
+	      fprintf (stderr, "\n");
+	      fprintf (stderr, "Command expected from GDB:\n");
+	    }
+	  while ((ch = logchar (fp, true)) != EOL);
+	  break;
+	case 'E':
+	  if (!debug_logging)
+	    fprintf (stderr, "E");
+	  while ((ch = logchar (fp, true)) != EOL);
 	  break;
 	}
+
     }
   remote_close ();
   exit (0);