[PATCH 2/4] gdbserver: Add debug-file option

[PATCH 2/4] gdbserver: Add debug-file option

[Alan Hayward]

Add command line option to send all debug output to a given file.
Always default back to stderr.

Add matching monitor command. Add documentation.

gdb/doc/ChangeLog:

2019-04-16  Alan Hayward  <alan.hayward@arm.com>

	* gdb.texinfo
	(Other Command-Line Arguments for gdbserver): Add debug-file
	option.
	(Monitor Commands for gdbserver): Likewise.
	(gdbserver man): Likewise.

gdb/gdbserver/ChangeLog:

2019-04-16  Alan Hayward  <alan.hayward@arm.com>

	* debug.c (debug_set_output): New function.
	(debug_vprintf): Send output to debug_file.
	(debug_flush): Likewise.
	* debug.h (debug_set_output): New declaration.
	* server.c (handle_monitor_command): Add debug-file option.
	(captured_main): Likewise.
---
 gdb/doc/gdb.texinfo    | 16 ++++++++++++++--
 gdb/gdbserver/debug.c  | 42 +++++++++++++++++++++++++++++++++++++++---
 gdb/gdbserver/debug.h  |  5 +++++
 gdb/gdbserver/server.c |  6 ++++++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f410d026b8..1716466609 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -21332,8 +21332,11 @@ The @option{--debug} option tells @code{gdbserver} to display extra
 status information about the debugging process.
 @cindex @option{--remote-debug}, @code{gdbserver} option
 The @option{--remote-debug} option tells @code{gdbserver} to display
-remote protocol debug output.  These options are intended for
-@code{gdbserver} development and for bug reports to the developers.
+remote protocol debug output.
+@cindex @option{--debug-file}, @code{gdbserver} option
+The @option{--debug-file=filename} option tells @code{gdbserver} to
+write any debug output to the given file.  These options are intended
+for @code{gdbserver} development and for bug reports to the developers.
 
 @cindex @option{--debug-format}, @code{gdbserver} option
 The @option{--debug-format=option1[,option2,...]} option tells
@@ -21433,6 +21436,10 @@ Disable or enable general debugging messages.
 Disable or enable specific debugging messages associated with the remote
 protocol (@pxref{Remote Protocol}).
 
+@item monitor set debug-file filename
+@itemx monitor set debug-file
+Send any debug output to the given file, or to stderr.
+
 @item monitor set debug-format option1@r{[},option2,...@r{]}
 Specify additional text to add to debugging messages.
 Possible options are:
@@ -44563,6 +44570,11 @@ Instruct @code{gdbserver} to display remote protocol debug output.
 This option is intended for @code{gdbserver} development and for bug reports to
 the developers.
 
+@item --debug-file=filename
+Instruct @code{gdbserver} to send any debug output to the given file.
+This option is intended for @code{gdbserver} development and for bug reports to
+the developers.
+
 @item --debug-format=option1@r{[},option2,...@r{]}
 Instruct @code{gdbserver} to include extra information in each line
 of debugging output.
diff --git a/gdb/gdbserver/debug.c b/gdb/gdbserver/debug.c
index 7c4c77afe2..53647c1ea6 100644
--- a/gdb/gdbserver/debug.c
+++ b/gdb/gdbserver/debug.c
@@ -23,6 +23,9 @@
 int remote_debug = 0;
 #endif
 
+/* Output file for debugging.  Default to standard error.  */
+FILE *debug_file = stderr;
+
 /* Enable miscellaneous debugging output.  The name is historical - it
    was originally used to debug LinuxThreads support.  */
 int debug_threads;
@@ -30,6 +33,39 @@ int debug_threads;
 /* Include timestamps in debugging output.  */
 int debug_timestamp;
 
+#if !defined (IN_PROCESS_AGENT)
+
+/* See debug.h.  */
+
+void
+debug_set_output (const char *new_debug_file)
+{
+  /* Close any existing file and reset to standard error.  */
+  if (debug_file != stderr)
+    {
+      fclose (debug_file);
+    }
+  debug_file = stderr;
+
+  /* Catch empty filenames.  */
+  if (new_debug_file == nullptr || strlen (new_debug_file) == 0)
+    return;
+
+  FILE *fptr = fopen (new_debug_file, "w");
+
+  if (fptr == nullptr)
+    {
+      debug_printf ("Cannot open %s for writing. %s. Switching to stderr.\n",
+		    new_debug_file, strerror (errno));
+      return;
+    }
+
+  debug_file = fptr;
+  return;
+}
+
+#endif
+
 /* Print a debugging message.
    If the text begins a new line it is preceded by a timestamp.
    We don't get fancy with newline checking, we just check whether the
@@ -50,11 +86,11 @@ debug_vprintf (const char *format, va_list ap)
       seconds s = duration_cast<seconds> (now.time_since_epoch ());
       microseconds us = duration_cast<microseconds> (now.time_since_epoch ()) - s;
 
-      fprintf (stderr, "%ld.%06ld ", (long) s.count (), (long) us.count ());
+      fprintf (debug_file, "%ld.%06ld ", (long) s.count (), (long) us.count ());
     }
 #endif
 
-  vfprintf (stderr, format, ap);
+  vfprintf (debug_file, format, ap);
 
 #if !defined (IN_PROCESS_AGENT)
   if (*format)
@@ -69,7 +105,7 @@ debug_vprintf (const char *format, va_list ap)
 void
 debug_flush (void)
 {
-  fflush (stderr);
+  fflush (debug_file);
 }
 
 /* Notify the user that the code is entering FUNCTION_NAME.
diff --git a/gdb/gdbserver/debug.h b/gdb/gdbserver/debug.h
index c8d5e3365e..f65c91c9eb 100644
--- a/gdb/gdbserver/debug.h
+++ b/gdb/gdbserver/debug.h
@@ -21,6 +21,11 @@
 
 #if !defined (IN_PROCESS_AGENT)
 extern int remote_debug;
+
+/* Switch all debug output to DEBUG_FILE.  If DEBUG_FILE is nullptr or an
+   empty string, or if the file cannot be opened, then debug output is sent to
+   stderr.  */
+void debug_set_output (const char *debug_file);
 #endif
 
 extern int debug_threads;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3f6c849dbc..36510ad1b2 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1403,6 +1403,10 @@ handle_monitor_command (char *mon, char *own_buf)
 	  write_enn (own_buf);
 	}
     }
+  else if (strcmp (mon, "set debug-file") == 0)
+    debug_set_output (nullptr);
+  else if (startswith (mon, "set debug-file "))
+    debug_set_output (mon + sizeof ("set debug-file ") - 1);
   else if (strcmp (mon, "help") == 0)
     monitor_show_help ();
   else if (strcmp (mon, "exit") == 0)
@@ -3649,6 +3653,8 @@ captured_main (int argc, char *argv[])
 	}
       else if (strcmp (*next_arg, "--remote-debug") == 0)
 	remote_debug = 1;
+      else if (startswith (*next_arg, "--debug-file="))
+	debug_set_output ((*next_arg) + sizeof ("--debug-file=") -1);
       else if (strcmp (*next_arg, "--disable-packet") == 0)
 	{
 	  gdbserver_show_disableable (stdout);
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 2/4] gdbserver: Add debug-file option

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> Add command line option to send all debug output to a given file.
Alan> Always default back to stderr.

Alan> Add matching monitor command. Add documentation.

Thanks.  The code parts of this are ok.

One little nit:

Alan> +  debug_file = fptr;
Alan> +  return;
Alan> +}

...this "return;" can be removed.

Tom

Re: [PATCH 2/4] gdbserver: Add debug-file option

[Alan Hayward]

> 
> On 16 Apr 2019, at 15:43, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> CC: nd <nd@arm.com>, Alan Hayward <Alan.Hayward@arm.com>
>> Date: Tue, 16 Apr 2019 10:17:41 +0000
>> 
>> gdb/doc/ChangeLog:
>> 
>> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* gdb.texinfo
>> 	(Other Command-Line Arguments for gdbserver): Add debug-file
> 
> The node name in parentheses should be on the same line as
> gdb.texinfo.

Fixed.

> 
>> 	(Monitor Commands for gdbserver): Likewise.
>> 	(gdbserver man): Likewise.
> 
> It is better to have a list of node names in parentheses, with only
> one description, than having separate entries that say "Likewise”.

Wasn’t aware it could be done that way. Fixed.

> 
>> +@cindex @option{--debug-file}, @code{gdbserver} option
> 
> I think it would be good to have here an additional index entry:
> 
> @cindex @code{gdbserver}, send all debug output to a single file

Makes sense. Added.

> 
>> +The @option{--debug-file=filename} option tells @code{gdbserver} to
>                           ^^^^^^^^
> "filename" should be in @var here, as it is a parameter.

Done.

> 
>> +write any debug output to the given file.  These options are intended
>                         ^^^^^^^^^^^^^^^^^
> "to the given @var{filename}", so as to reference the parameter.

Done.

> 
>> +@item --debug-file=filename
>> +Instruct @code{gdbserver} to send any debug output to the given file.
> 
> Same here.

Done.

> 
> The documentation changes are okay with these nits fixed.
> 
> Thanks.

> 
> On 16 Apr 2019, at 20:39, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> Add command line option to send all debug output to a given file.
> Alan> Always default back to stderr.
> 
> Alan> Add matching monitor command. Add documentation.
> 
> Thanks.  The code parts of this are ok.
> 
> One little nit:
> 
> Alan> +  debug_file = fptr;
> Alan> +  return;
> Alan> +}
> 
> ...this "return;" can be removed.
> 

Done.



Thanks!

Pushed the following:




diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 27e65ea56c..64073278fa 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,9 @@
+2019-04-17  Alan Hayward  <alan.hayward@arm.com>
+
+       * gdb.texinfo (Other Command-Line Arguments for gdbserver)
+       (Monitor Commands for gdbserver)
+       (gdbserver man): Add debug-file option.
+
2019-04-08  Kevin Buettner  <kevinb@redhat.com>

       * python.texi (Inferiors In Python): Rename
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f410d026b8..a3a5f3e28c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -21332,8 +21332,12 @@ The @option{--debug} option tells @code{gdbserver} to display extra
status information about the debugging process.
@cindex @option{--remote-debug}, @code{gdbserver} option
The @option{--remote-debug} option tells @code{gdbserver} to display
-remote protocol debug output.  These options are intended for
-@code{gdbserver} development and for bug reports to the developers.
+remote protocol debug output.
+@cindex @option{--debug-file}, @code{gdbserver} option
+@cindex @code{gdbserver}, send all debug output to a single file
+The @option{--debug-file=@var{filename}} option tells @code{gdbserver} to
+write any debug output to the given @var{filename}.  These options are intended
+for @code{gdbserver} development and for bug reports to the developers.

@cindex @option{--debug-format}, @code{gdbserver} option
The @option{--debug-format=option1[,option2,...]} option tells
@@ -21433,6 +21437,10 @@ Disable or enable general debugging messages.
Disable or enable specific debugging messages associated with the remote
protocol (@pxref{Remote Protocol}).

+@item monitor set debug-file filename
+@itemx monitor set debug-file
+Send any debug output to the given file, or to stderr.
+
@item monitor set debug-format option1@r{[},option2,...@r{]}
Specify additional text to add to debugging messages.
Possible options are:
@@ -44563,6 +44571,11 @@ Instruct @code{gdbserver} to display remote protocol debug output.
This option is intended for @code{gdbserver} development and for bug reports to
the developers.

+@item --debug-file=@var{filename}
+Instruct @code{gdbserver} to send any debug output to the given @var{filename}.
+This option is intended for @code{gdbserver} development and for bug reports to
+the developers.
+
@item --debug-format=option1@r{[},option2,...@r{]}
Instruct @code{gdbserver} to include extra information in each line
of debugging output.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 0581f59b5d..d3380d6145 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,12 @@
+2019-04-17  Alan Hayward  <alan.hayward@arm.com>
+
+       * debug.c (debug_set_output): New function.
+       (debug_vprintf): Send output to debug_file.
+       (debug_flush): Likewise.
+       * debug.h (debug_set_output): New declaration.
+       * server.c (handle_monitor_command): Add debug-file option.
+       (captured_main): Likewise.
+
2019-04-17  Alan Hayward  <alan.hayward@arm.com>

       * debug.c (remote_debug): Add definition.
diff --git a/gdb/gdbserver/debug.c b/gdb/gdbserver/debug.c
index 7c4c77afe2..d80cd52540 100644
--- a/gdb/gdbserver/debug.c
+++ b/gdb/gdbserver/debug.c
@@ -23,6 +23,9 @@
int remote_debug = 0;
#endif

+/* Output file for debugging.  Default to standard error.  */
+FILE *debug_file = stderr;
+
/* Enable miscellaneous debugging output.  The name is historical - it
   was originally used to debug LinuxThreads support.  */
int debug_threads;
@@ -30,6 +33,38 @@ int debug_threads;
/* Include timestamps in debugging output.  */
int debug_timestamp;

+#if !defined (IN_PROCESS_AGENT)
+
+/* See debug.h.  */
+
+void
+debug_set_output (const char *new_debug_file)
+{
+  /* Close any existing file and reset to standard error.  */
+  if (debug_file != stderr)
+    {
+      fclose (debug_file);
+    }
+  debug_file = stderr;
+
+  /* Catch empty filenames.  */
+  if (new_debug_file == nullptr || strlen (new_debug_file) == 0)
+    return;
+
+  FILE *fptr = fopen (new_debug_file, "w");
+
+  if (fptr == nullptr)
+    {
+      debug_printf ("Cannot open %s for writing. %s. Switching to stderr.\n",
+                   new_debug_file, strerror (errno));
+      return;
+    }
+
+  debug_file = fptr;
+}
+
+#endif
+
/* Print a debugging message.
   If the text begins a new line it is preceded by a timestamp.
   We don't get fancy with newline checking, we just check whether the
@@ -50,11 +85,11 @@ debug_vprintf (const char *format, va_list ap)
      seconds s = duration_cast<seconds> (now.time_since_epoch ());
      microseconds us = duration_cast<microseconds> (now.time_since_epoch ()) - s;

-      fprintf (stderr, "%ld.%06ld ", (long) s.count (), (long) us.count ());
+      fprintf (debug_file, "%ld.%06ld ", (long) s.count (), (long) us.count ());
    }
#endif

-  vfprintf (stderr, format, ap);
+  vfprintf (debug_file, format, ap);

#if !defined (IN_PROCESS_AGENT)
  if (*format)
@@ -69,7 +104,7 @@ debug_vprintf (const char *format, va_list ap)
void
debug_flush (void)
{
-  fflush (stderr);
+  fflush (debug_file);
}

/* Notify the user that the code is entering FUNCTION_NAME.
diff --git a/gdb/gdbserver/debug.h b/gdb/gdbserver/debug.h
index c8d5e3365e..f65c91c9eb 100644
--- a/gdb/gdbserver/debug.h
+++ b/gdb/gdbserver/debug.h
@@ -21,6 +21,11 @@

#if !defined (IN_PROCESS_AGENT)
extern int remote_debug;
+
+/* Switch all debug output to DEBUG_FILE.  If DEBUG_FILE is nullptr or an
+   empty string, or if the file cannot be opened, then debug output is sent to
+   stderr.  */
+void debug_set_output (const char *debug_file);
#endif

extern int debug_threads;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3f6c849dbc..36510ad1b2 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1403,6 +1403,10 @@ handle_monitor_command (char *mon, char *own_buf)
         write_enn (own_buf);
       }
    }
+  else if (strcmp (mon, "set debug-file") == 0)
+    debug_set_output (nullptr);
+  else if (startswith (mon, "set debug-file "))
+    debug_set_output (mon + sizeof ("set debug-file ") - 1);
  else if (strcmp (mon, "help") == 0)
    monitor_show_help ();
  else if (strcmp (mon, "exit") == 0)
@@ -3649,6 +3653,8 @@ captured_main (int argc, char *argv[])
       }
      else if (strcmp (*next_arg, "--remote-debug") == 0)
       remote_debug = 1;
+      else if (startswith (*next_arg, "--debug-file="))
+       debug_set_output ((*next_arg) + sizeof ("--debug-file=") -1);
      else if (strcmp (*next_arg, "--disable-packet") == 0)
       {
         gdbserver_show_disableable (stdout);

Re: [PATCH 0/4] gdbserver/testsuite : Capture gdbserver debug output during testing

[Eli Zaretskii]

> From: Alan Hayward <Alan.Hayward@arm.com>
> CC: nd <nd@arm.com>, Alan Hayward <Alan.Hayward@arm.com>
> Date: Tue, 16 Apr 2019 10:17:35 +0000
> 
> Not sure if I should add a statement to the NEWS file (can add in a follow
> on patch).

I think you should.

Re: [PATCH 1/4] gdbserver: Move remote_debug to a single place

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> Also removes the unused gdb_stdlog variable.

I looked and this is referenced from common/observable.h; but I suppose
that could be converted to use debug_vprintf or similar if it's ever
used in gdbserver.

Alan> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>
Alan> 	* debug.c (remote_debug): Add definition.
Alan> 	* debug.h (remote_debug): Add declaration.
Alan> 	* hostio.c (remote_debug): Remove declaration.
Alan> 	* remote-utils.c (struct ui_file): Likewise.
Alan> 	(remote_debug): Likewise.
Alan> 	* remote-utils.h (remote_debug): Likewise,
Alan> 	* server.c (remote_debug): Remove definition.

Thanks, this is ok.

Tom

Re: [PATCH 3/4] gdbserver: Ensure all debug output uses debug functions

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> All debug output needs to go via debug functions to ensure it writes to the
Alan> correct output stream.

Alan> gdb/ChangeLog:

Alan> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>

Alan> 	* nat/linux-waitpid.c (linux_debug): Call debug_vprintf.

Alan> gdb/gdbserver/ChangeLog:

Alan> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>

Alan> 	* ax.c (ax_vdebug): Call debug_printf.
Alan> 	* debug.c (debug_write): New function.
Alan> 	* debug.h (debug_write): New declaration.
Alan> 	* linux-low.c (sigchld_handler): Call debug_write.

Thanks, this is ok.

Tom

Re: [PATCH 3/4] gdbserver: Ensure all debug output uses debug functions

[Tom de Vries]

On 16-04-19 21:43, Tom Tromey wrote:
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> All debug output needs to go via debug functions to ensure it writes to the
> Alan> correct output stream.
> 
> Alan> gdb/ChangeLog:
> 
> Alan> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>
> 
> Alan> 	* nat/linux-waitpid.c (linux_debug): Call debug_vprintf.
> 
> Alan> gdb/gdbserver/ChangeLog:
> 
> Alan> 2019-04-16  Alan Hayward  <alan.hayward@arm.com>
> 
> Alan> 	* ax.c (ax_vdebug): Call debug_printf.
> Alan> 	* debug.c (debug_write): New function.
> Alan> 	* debug.h (debug_write): New declaration.
> Alan> 	* linux-low.c (sigchld_handler): Call debug_write.
> 

Building gdb with clang, I run into:
...
src/gdb/gdbserver/linux-low.c:6190:41: error: comparison of unsigned
expression < 0 is always false [-Werror,-Wtautological-compare]
                           sizeof ("sigchld_handler\n") - 1) < 0)
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~
...

This seems to fix it.

Thanks,
- Tom

diff --git a/gdb/gdbserver/debug.c b/gdb/gdbserver/debug.c
index a1cf5dbf7a..19f11fc17c 100644
--- a/gdb/gdbserver/debug.c
+++ b/gdb/gdbserver/debug.c
@@ -133,7 +133,7 @@ do_debug_exit (const char *function_name)

 /* See debug.h.  */

-size_t
+ssize_t
 debug_write (const void *buf, size_t nbyte)
 {
   int fd = fileno (debug_file);
diff --git a/gdb/gdbserver/debug.h b/gdb/gdbserver/debug.h
index 29e58ad8a4..07e94eac6e 100644
--- a/gdb/gdbserver/debug.h
+++ b/gdb/gdbserver/debug.h
@@ -36,7 +36,7 @@ void do_debug_enter (const char *function_name);
 void do_debug_exit (const char *function_name);

 /* Async signal safe debug output function that calls write directly.  */
-size_t debug_write (const void *buf, size_t nbyte);
+ssize_t debug_write (const void *buf, size_t nbyte);

 /* These macros are for use in major functions that produce a lot of
    debugging output.  They help identify in the mass of debugging output

Re: [PATCH 3/4] gdbserver: Ensure all debug output uses debug functions

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This seems to fix it.

This needs a ChangeLog entry but is otherwise ok.  Thanks.

Tom

Re: [PATCH 4/4] testsuite: Add option to capture gdbserver debug

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> Add board option which enables gdbserver debug and sends it to the
Alan> file gdbserver.log, located in the output directory for the current
Alan> test.  Document this.

Alan> Add debug versions of the native gdbserver board files.

Alan> Disable tspeed.exp when debugging to prevent the log file filling
Alan> many gigabytes then timing out.

Thanks.

Alan> +gdbserver,debug
Alan> +
Alan> +  When set gdbserver debug is outputed to the file gdbserver.log in the test

I think it should say "is sent" rather than "is outputed".

Alan> diff --git a/gdb/testsuite/boards/native-extended-gdbserver-debug.exp b/gdb/testsuite/boards/native-extended-gdbserver-debug.exp

I wonder if a new board is needed for this?
Could it be done some other way, like a command-line setting?

TBH I'm not sure what the typical approach is for something like this.

Alan>  	if {![info exists gdbserver_reconnect_p] || !$gdbserver_reconnect_p} {
Alan>  	    # GDB client could accidentally connect to a stale server.
Alan> -	    # append gdbserver_command " --debug --once"
Alan>  	    append gdbserver_command " --once"

Was this intentional?


One random thought I had about this series is that it would be nice to
have a way to get "set remotelogfile" output in the test directory.
That way a failing test could be re-run with gdbreplay without much
trouble.

Tom

Re: [PATCH 4/4] testsuite: Add option to capture gdbserver debug

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2478 bytes --]

(Patches 1 to 3 pushed).

> On 16 Apr 2019, at 20:49, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> Add board option which enables gdbserver debug and sends it to the
> Alan> file gdbserver.log, located in the output directory for the current
> Alan> test.  Document this.
> 
> Alan> Add debug versions of the native gdbserver board files.
> 
> Alan> Disable tspeed.exp when debugging to prevent the log file filling
> Alan> many gigabytes then timing out.
> 
> Thanks.
> 
> Alan> +gdbserver,debug
> Alan> +
> Alan> +  When set gdbserver debug is outputed to the file gdbserver.log in the test
> 
> I think it should say "is sent" rather than "is outputed”.

Done.

> Alan> diff --git a/gdb/testsuite/boards/native-extended-gdbserver-debug.exp b/gdb/testsuite/boards/native-extended-gdbserver-debug.exp
> 
> I wonder if a new board is needed for this?
> Could it be done some other way, like a command-line setting?

Agreed, it’s a little awkward. I’ve been playing round with an alternative
implementation and have added it an environment variable (in addition to the
board setting).

So, now you can do:
	make check GDBSERVER_DEBUG=all

Which is much nicer. However, see final comment.

> 
> TBH I'm not sure what the typical approach is for something like this.
> 
> Alan>  	if {![info exists gdbserver_reconnect_p] || !$gdbserver_reconnect_p} {
> Alan>  	    # GDB client could accidentally connect to a stale server.
> Alan> -	    # append gdbserver_command " --debug --once"
> Alan>  	    append gdbserver_command " --once"
> 
> Was this intentional?

Yes, I removed this commented out line. I suspect it was there so that you could
quickly switch over to using "—debug". With my changes you shouldn’t need it.

> 
> 
> One random thought I had about this series is that it would be nice to
> have a way to get "set remotelogfile" output in the test directory.
> That way a failing test could be re-run with gdbreplay without much
> trouble.

Yes, I think it’d be possible using a similar approach.  You’d end up with another
environment variable, REPLAY_LOG=1 or something.  My only concern is that it’s
starting down the road of adding more environment vars to the make line - which
people might not want?


I’ll post the updated version as a V2 so that I can use git send-email.


Alan.\x16º&Öéj×!zÊÞ¶êçמw÷Ib²Ö«r\x18\x1dn–­r\x17¬

[PATCH 4/4] testsuite: Add option to capture gdbserver debug

[Alan Hayward]

Add both board option and environment variable which enables gdbserver
debug and sends it to the file gdbserver.debug, located in the output
directory for the current test.  Document this.

Add support for the environment variable in the Makefile.

The testsuite can be run with gdbserver debug enabled in the following way:

	make check GDBSERVER_DEBUG=all

Disable tspeed.exp when debugging to prevent the log file filling
many gigabytes then timing out.

gdb/testsuite/ChangeLog:

2019-04-23  Alan Hayward  <alan.hayward@arm.com>

	* Makefile.in: Pass through GDBSERVER_DEBUG.
        * README (Testsuite Parameters): Add GDBSERVER_DEBUG.
        (gdbserver,debug): Add board setting.
        * gdb.trace/tspeed.exp: Skip when debugging.
	* lib/gdb.exp (gdbserver_debug_enabled): New procedure.
	* lib/gdbserver-support.exp: Likewise
---
 gdb/testsuite/Makefile.in               |  4 ++-
 gdb/testsuite/README                    | 19 +++++++++++++
 gdb/testsuite/gdb.trace/tspeed.exp      |  5 ++++
 gdb/testsuite/lib/gdb.exp               |  7 +++++
 gdb/testsuite/lib/gdbserver-support.exp | 38 ++++++++++++++++++++++++-
 5 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 6625512d50..40225e3b91 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -53,6 +53,7 @@ RUNTESTFLAGS =
 FORCE_PARALLEL =
 
 GDB_DEBUG =
+GDBSERVER_DEBUG =
 
 # Default number of iterations that we will use to run the testsuite
 # if the user does not specify the RACY_ITER environment variable
@@ -165,6 +166,7 @@ check-read1:
 TIMESTAMP = $(if $(TS),| $(srcdir)/print-ts.py $(if $(TS_FORMAT),$(TS_FORMAT),),)
 
 gdb_debug = $(if $(GDB_DEBUG),GDB_DEBUG=$(GDB_DEBUG) ; export GDB_DEBUG ;,)
+gdbserver_debug = $(if $(GDBSERVER_DEBUG),GDBSERVER_DEBUG=$(GDBSERVER_DEBUG) ; export GDBSERVER_DEBUG ;,)
 
 # All the hair to invoke dejagnu.  A given invocation can just append
 # $(RUNTESTFLAGS)
@@ -172,7 +174,7 @@ DO_RUNTEST = \
 	rootme=`pwd`; export rootme; \
 	srcdir=${srcdir} ; export srcdir ; \
 	EXPECT=${EXPECT} ; export EXPECT ; \
-	EXEEXT=${EXEEXT} ; export EXEEXT ; $(gdb_debug) \
+	EXEEXT=${EXEEXT} ; export EXEEXT ; $(gdb_debug) $(gdbserver_debug) \
         $(RPATH_ENVVAR)=$$rootme/../../expect:$$rootme/../../libstdc++:$$rootme/../../tk/unix:$$rootme/../../tcl/unix:$$rootme/../../bfd:$$rootme/../../opcodes:$$$(RPATH_ENVVAR); \
 	export $(RPATH_ENVVAR); \
 	if [ -f $${rootme}/../../expect/expect ] ; then  \
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index e17f218da8..43f35a9d4b 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -302,6 +302,17 @@ For example, to turn on debugging for infrun and target, you can do:
 
 	make check GDB_DEBUG="infrun,target"
 
+GDBSERVER_DEBUG
+
+When set gdbserver debug is sent to the file gdbserver.debug in the test
+output directory.  Valid values are:
+	debug  - turn on gdbserver debug.
+	remote - turn on gdbserver remote debug.
+	all - turn on all the above debug options.
+For example, to turn on all gdbserver debugging, you can do:
+
+	make check GDBSERVER_DEBUG=all
+
 Race detection
 **************
 
@@ -513,6 +524,14 @@ gdb,debug
   components. For example, to turn on debugging for infrun and target, set to
   "infrun,target".
 
+gdbserver,debug
+
+  When set gdbserver debug is sent to the file gdbserver.debug in the test
+  output directory.  Valid values are:
+  "debug"  - turn on gdbserver debug.
+  "remote" - turn on gdbserver remote debug.
+  "all" - turn on all the above debug options.
+
 Testsuite Organization
 **********************
 
diff --git a/gdb/testsuite/gdb.trace/tspeed.exp b/gdb/testsuite/gdb.trace/tspeed.exp
index 6fd812e4f6..9afec64acf 100644
--- a/gdb/testsuite/gdb.trace/tspeed.exp
+++ b/gdb/testsuite/gdb.trace/tspeed.exp
@@ -19,6 +19,11 @@ if {[skip_shlib_tests]} {
     return 0
 }
 
+# Do not run if gdbsever debug is enabled - the output file is many Gb.
+if [gdbserver_debug_enabled] {
+    return 0
+}
+
 standard_testfile
 set executable $testfile
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 896cd80741..35d65d0e43 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6410,5 +6410,12 @@ proc gdb_debug_init { } {
   gdb_test "set logging on" ".*" "Enable logging"
 }
 
+# Check if debugging is enabled for gdbserver.
+
+proc gdbserver_debug_enabled { } {
+    # Always disabled for GDB only setups.
+    return 0
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 2cb64f7d2f..164a1d1d3c 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -283,12 +283,26 @@ proc gdbserver_start { options arguments } {
 	# If gdbserver_reconnect will be called $gdbserver_reconnect_p must be
 	# set to true already during gdbserver_start.
 	global gdbserver_reconnect_p
+	global srcdir
+	global subdir
 	if {![info exists gdbserver_reconnect_p] || !$gdbserver_reconnect_p} {
 	    # GDB client could accidentally connect to a stale server.
-	    # append gdbserver_command " --debug --once"
 	    append gdbserver_command " --once"
 	}
 
+	# Enable debug if set.
+	if [gdbserver_debug_enabled] {
+	    global gdbserverdebug
+	    set debugfile [standard_output_file gdbserver.debug]
+	    if { $gdbserverdebug == "debug" } {
+		append gdbserver_command " --debug --debug-file=$debugfile"
+	    } elseif { $gdbserverdebug == "remote" } {
+		append gdbserver_command " --remote-debug --debug-file=$debugfile"
+	    } elseif { $gdbserverdebug == "all" } {
+		append gdbserver_command " --debug --remote-debug --debug-file=$debugfile"
+	    }
+	}
+
 	if { $options != "" } {
 	    append gdbserver_command " $options"
 	}
@@ -561,3 +575,25 @@ proc mi_gdbserver_start_multi { } {
 
     return [mi_gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
 }
+
+# Check if debugging is enabled for gdbserver.
+
+proc gdbserver_debug_enabled { } {
+    global gdbserverdebug
+
+    # If not already read, get the debug setting from environment or board setting.
+    if ![info exists gdbserverdebug] {
+	global env
+	if [info exists env(GDBSERVER_DEBUG)] {
+	    set gdbserverdebug $env(GDBSERVER_DEBUG)
+	} elseif [target_info exists gdbserver,debug] {
+	    set gdbserverdebug [target_info gdbserver,debug]
+	} else {
+	    return 0
+	}
+    }
+
+    # Only return success on valid values.
+    return [expr { $gdbserverdebug == "debug" || $gdbserverdebug == "remote"
+		   || $gdbserverdebug == "all" }]
+}
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 4/4] testsuite: Add option to capture gdbserver debug

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> 2019-04-23  Alan Hayward  <alan.hayward@arm.com>

Alan> 	* Makefile.in: Pass through GDBSERVER_DEBUG.
Alan>         * README (Testsuite Parameters): Add GDBSERVER_DEBUG.
Alan>         (gdbserver,debug): Add board setting.
Alan>         * gdb.trace/tspeed.exp: Skip when debugging.
Alan> 	* lib/gdb.exp (gdbserver_debug_enabled): New procedure.
Alan> 	* lib/gdbserver-support.exp: Likewise

Thanks, this is ok.

Tom