gdbreplay: Add --debug-logging option

Message ID 20241002102539.3541666-1-ahajkova@khirnov.net
State New
Headers
Series gdbreplay: Add --debug-logging option |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
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_check--master-aarch64 success Test passed

Commit Message

Alexandra Hájková Oct. 2, 2024, 10:24 a.m. UTC
  From: Alexandra Hájková <ahajkova@redhat.com>

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

Eli Zaretskii Oct. 5, 2024, 6:17 a.m. UTC | #1
> From: Alexandra Hájková <ahajkova@khirnov.net>
> Cc: ahajkova@redhat.com
> Date: Wed,  2 Oct 2024 12:24:04 +0200
> 
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
> 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.

Shouldn't this be the other way around: keep the previous behavior by
default and add an opt-in switch to disable that (for use in tests
etc.)?  Why change the default behavior for the benefit of a single
use case?

> Other motivation is making communication between GDB
> and gdbreplay faster as printing bigger remotelogfile takes
> considerable amount of time.

Did someone complain about that?

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

Our conventions are to leave two spaces between sentences, so please
leave one more.  Otherwise, the documentation part of this is
approved.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  
Alexandra Petlanova Hajkova Oct. 5, 2024, 9:59 a.m. UTC | #2
On Sat, Oct 5, 2024 at 8:17 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Alexandra Hájková <ahajkova@khirnov.net>
> > Cc: ahajkova@redhat.com
> > Date: Wed,  2 Oct 2024 12:24:04 +0200
> >
> > From: Alexandra Hájková <ahajkova@redhat.com>
> >
> > 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.
>
> Shouldn't this be the other way around: keep the previous behavior by
> default and add an opt-in switch to disable that (for use in tests
> etc.)?  Why change the default behavior for the benefit of a single
> use case?
>
> > Other motivation is making communication between GDB
> > and gdbreplay faster as printing bigger remotelogfile takes
> > considerable amount of time.
>
> Did someone complain about that?
>

Having to wait for gdbreplay to print all the remote protocol packets till
it reaches the next GDB command
annoys me a lot. I expected the other people may feel the same. If not,
let's keep the original behaviour.
I'd really like to get an opinion from the other gdbreplay users.

Thank you,
Alexandra
  
Eli Zaretskii Oct. 5, 2024, 11:32 a.m. UTC | #3
> From: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
> Date: Sat, 5 Oct 2024 11:59:20 +0200
> Cc: Alexandra Hájková <ahajkova@khirnov.net>, 
> 	gdb-patches@sourceware.org
> 
> On Sat, Oct 5, 2024 at 8:17 AM Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  > From: Alexandra Hájková <ahajkova@khirnov.net>
>  > Cc: ahajkova@redhat.com
>  > Date: Wed,  2 Oct 2024 12:24:04 +0200
>  > 
>  > From: Alexandra Hájková <ahajkova@redhat.com>
>  > 
>  > 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.
> 
>  Shouldn't this be the other way around: keep the previous behavior by
>  default and add an opt-in switch to disable that (for use in tests
>  etc.)?  Why change the default behavior for the benefit of a single
>  use case?
> 
>  > Other motivation is making communication between GDB
>  > and gdbreplay faster as printing bigger remotelogfile takes
>  > considerable amount of time.
> 
>  Did someone complain about that?
> 
> Having to wait for gdbreplay to print all the remote protocol packets till it reaches the next GDB command
> annoys me a lot. I expected the other people may feel the same. If not, let's keep the original behaviour.
> I'd really like to get an opinion from the other gdbreplay users. 

Let's hear what others think about this change, and take it from
there.

Thanks.
  
Tom Tromey Oct. 7, 2024, 5:08 p.m. UTC | #4
>>>>> "Alexandra" == Alexandra Petlanova Hajkova <ahajkova@redhat.com> writes:

Alexandra> Having to wait for gdbreplay to print all the remote protocol
Alexandra> packets till it reaches the next GDB command annoys me a
Alexandra> lot. I expected the other people may feel the same. If not,
Alexandra> let's keep the original behaviour.  I'd really like to get an
Alexandra> opinion from the other gdbreplay users.

FWIW I think it's fine to change the default, the main reason being that
gdbreplay isn't installed and so is only used by gdb developers.

If the change proves to be a problem we can always flip it again later.

Tom
  
Andrew Burgess Oct. 11, 2024, 9:51 a.m. UTC | #5
Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
>> Date: Sat, 5 Oct 2024 11:59:20 +0200
>> Cc: Alexandra Hájková <ahajkova@khirnov.net>, 
>> 	gdb-patches@sourceware.org
>> 
>> On Sat, Oct 5, 2024 at 8:17 AM Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>  > From: Alexandra Hájková <ahajkova@khirnov.net>
>>  > Cc: ahajkova@redhat.com
>>  > Date: Wed,  2 Oct 2024 12:24:04 +0200
>>  > 
>>  > From: Alexandra Hájková <ahajkova@redhat.com>
>>  > 
>>  > 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.
>> 
>>  Shouldn't this be the other way around: keep the previous behavior by
>>  default and add an opt-in switch to disable that (for use in tests
>>  etc.)?  Why change the default behavior for the benefit of a single
>>  use case?
>> 
>>  > Other motivation is making communication between GDB
>>  > and gdbreplay faster as printing bigger remotelogfile takes
>>  > considerable amount of time.
>> 
>>  Did someone complain about that?
>> 
>> Having to wait for gdbreplay to print all the remote protocol packets till it reaches the next GDB command
>> annoys me a lot. I expected the other people may feel the same. If not, let's keep the original behaviour.
>> I'd really like to get an opinion from the other gdbreplay users. 
>
> Let's hear what others think about this change, and take it from
> there.

My thoughts on this: as Tom said, gdbreplay is a maintainer tool so I
think we have more flexibility to experiment with the defaults.

The current default of printing everything is pretty useless really as
it spams the terminal with some huge packets (which can easily be read
from the log file if we really want to see them), all the terminal
output slows things down.

If maintainers do complain then we can always flip the default back
again later.


Thanks,
Andrew
  

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