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

[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

Re: [PATCH 1/4] Add debug redirect option

[Tom Tromey]

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

Alan> Add the option to redirect debug output seperately to normal
Alan> output, using the cli command:

Alan>   set logging debugredirect on

[...]

Alan> [
Alan> An alternative would be to add an extra option to the original
Alan> redirect command:
Alan>   set logging redirect on|off|debug
Alan> This method is less flexible, but may be more intuative - setting to
Alan> debug would cause just debug to be redirected.  It would break
Alan> existing scripts that enable using "set logging redirect 1" instead
Alan> of "set logging redirect on".
Alan> ]

I think the choice you implemented here is fine.

Alan> [ I'm also a little unsure if I have the "release" logic correct. ]

One way to improve this code would be to change tee_file to use
ui_file_up.  That way the call to the constructor wouldn't need to use
release(), just std::move.  See the appended.

Tom

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 3a5e14de3c7..8e34bfc12bc 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -97,13 +97,7 @@ make_logging_output (ui_file *curr_output, ui_file_up logfile,
   if (logging_redirect)
     return logfile.release ();
   else
-    {
-      /* Note that the "tee" takes ownership of the log file.  */
-      ui_file *out = new tee_file (curr_output, false,
-				   logfile.get (), true);
-      logfile.release ();
-      return out;
-    }
+    return new tee_file (curr_output, std::move (logfile));
 }
 
 /* This is a helper for the `set logging' command.  */
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 77f6b31ce4b..f18738af624 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -283,20 +283,13 @@ stderr_file::stderr_file (FILE *stream)
 
 \f
 
-tee_file::tee_file (ui_file *one, bool close_one,
-		    ui_file *two, bool close_two)
+tee_file::tee_file (ui_file *one, ui_file_up &&two)
   : m_one (one),
-    m_two (two),
-    m_close_one (close_one),
-    m_close_two (close_two)
+    m_two (std::move (two))
 {}
 
 tee_file::~tee_file ()
 {
-  if (m_close_one)
-    delete m_one;
-  if (m_close_two)
-    delete m_two;
 }
 
 void
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 6e6ca1c9cdc..a932f215416 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -243,11 +243,9 @@ public:
 class tee_file : public ui_file
 {
 public:
-  /* Create a file which writes to both ONE and TWO.  CLOSE_ONE and
-     CLOSE_TWO indicate whether the original files should be closed
-     when the new file is closed.  */
-  tee_file (ui_file *one, bool close_one,
-	    ui_file *two, bool close_two);
+  /* Create a file which writes to both ONE and TWO.  ONE will remain
+     open when this object is destroyed; but TWO will be closed.  */
+  tee_file (ui_file *one, ui_file_up &&two);
   ~tee_file () override;
 
   void write (const char *buf, long length_buf) override;
@@ -258,10 +256,9 @@ public:
   void flush () override;
 
 private:
-  /* The two underlying ui_files, and whether they should each be
-     closed on destruction.  */
-  ui_file *m_one, *m_two;
-  bool m_close_one, m_close_two;
+  /* The two underlying ui_files.  */
+  ui_file *m_one;
+  ui_file_up m_two;
 };
 
 #endif

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

[Tom Tromey]

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

Alan> @@ -1716,6 +1716,8 @@ proc default_gdb_start { } {
Alan>  	    warning "Couldn't set the width to 0."
Alan>  	}
Alan>      }
Alan> +
Alan> +    gdb_debug_init

One question I have is how this interacts with tests that run gdb
multiple times.  Is there just one log?

Alan> +    if ![info exists gdbdebug] {

I think it's preferable to brace expressions, like "if {![info exists...]}".
I guess it's just stylistic though.

Alan> +  # First ensure logging is off.
Alan> +  gdb_test_no_output "set logging off"

Related to the multiple gdb invocations question -- I think this will
result in duplicate test names, so something extra should probably be
done here.

thanks,
Tom

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

[Alan Hayward]

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

Thanks for the reviews. I’ll push patch 4, and repost the others
with the suggested changes.


> On 25 Apr 2019, at 15:31, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> @@ -1716,6 +1716,8 @@ proc default_gdb_start { } {
> Alan>  	    warning "Couldn't set the width to 0."
> Alan>  	}
> Alan>      }
> Alan> +
> Alan> +    gdb_debug_init
> 
> One question I have is how this interacts with tests that run gdb
> multiple times.  Is there just one log?

By default the log files are opened in append mode. So everything should
happily go to a single file. I’ll double check.

> 
> Alan> +    if ![info exists gdbdebug] {
> 
> I think it's preferable to brace expressions, like "if {![info exists...]}".
> I guess it's just stylistic though.

Ok.

> 
> Alan> +  # First ensure logging is off.
> Alan> +  gdb_test_no_output "set logging off"
> 
> Related to the multiple gdb invocations question -- I think this will
> result in duplicate test names, so something extra should probably be
> done here.
> 

I can switch this to use “send_gdb” in the same way "set height 0” is used.
It’ll also stop the number of passes increasing when debug is enabled.


> thanks,
> Tom

Oh, and earlier I got a mailbox full bounce back from your email when I sent
the reply to the PIE email:
"550 5.0.350 Remote server returned an error -> 550 Mailbox is full / Blocks limit exceeded / Inode limit exceeded”


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

Re: [PATCH 3/4] testsuite: Disable tests when logging

[Tom Tromey]

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

Alan> 	* gdb.base/breakpoint-in-ro-region.exp: Disable when debugging.
[...]

My first reaction to this was negative, but after thinking about it a
little I came around.  I think my view is that enabling debug logging
for the test suite is primarily a debugging tool, and so it's fine --
especially for tests that are already enabling debugging -- for it not
to work universally.

Alan> +# Do not run if gdb debug is enabled

Normally comments should have a "." at the end.
It would perhaps be good if the comments explained why.

Alan> +# Do not run if gdb debug is enabled - it interferes with the command history.

... like this one.  Is that reasonable?

thanks,
Tom

Re: [PATCH 0/4] Capture GDB debug when running the testsuite

[Tom Tromey]

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

Alan> Maybe something needs adding to the gdb wiki too?

I think probably this page:

https://sourceware.org/gdb/wiki/TestingGDB

thanks,
Tom