[binutils-gdb] testsuite: Don't use expect_background to reap gdbserver

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6423214fcb912a5d402ffcb84d6f61f2b900a900

commit 6423214fcb912a5d402ffcb84d6f61f2b900a900
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Apr 7 18:19:30 2015 +0100

    testsuite: Don't use expect_background to reap gdbserver
    
    I adjusted a test to do 'expect -i $server_spawn_id -re ...', and saw
    really strange behavior.  Whether that expect would work, depended on
    whether GDB would also send output and the same expect matched it too
    (on $gdb_spawn_id).  I was perplexed until I noticed that
    gdbserver_spawn spawns gdbserver and then uses expect_background to
    reap gdbserver.  That expect_background conflicts/races with any
    "expect -i $server_spawn_id" done anywhere else in parallel...
    
    In order to make it possible for tests to read inferior I/O out of
    $server_spawn_id, we to get rid of that expect_background.  This patch
    makes us instead reap gdbserver's spawn id when GDB exits.  If GDB is
    still around, this gives a chance for gdbserver to exit cleanly.  The
    current code in gdb_finish uses "kill", but that doesn't work with
    extended-remote (gdbserver doesn't exit).  We now use "monitor exit"
    instead which works in both remote and extended-remote modes.
    
    gdb/testsuite/ChangeLog:
    2015-04-07  Pedro Alves  <palves@redhat.com>
    
    	* lib/gdb.exp (gdb_finish): Delete persistent gdbserver handling.
    	* lib/gdbserver-support.exp (gdbserver_start): Make
    	$server_spawn_id global.
    	(gdbserver_start): Don't wait for gdbserver's spawn id with
    	expect_background.
    	(close_gdbserver): New procedure.
    	(gdb_exit): Rename the default version and reimplement.

Diff:
---
 gdb/testsuite/ChangeLog                 | 10 ++++++
 gdb/testsuite/lib/gdb.exp               | 14 --------
 gdb/testsuite/lib/gdbserver-support.exp | 61 ++++++++++++++++++++++++++-------
 3 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 694b1c8..eb084d2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,15 @@
 2015-04-07  Pedro Alves  <palves@redhat.com>
 
+	* lib/gdb.exp (gdb_finish): Delete persistent gdbserver handling.
+	* lib/gdbserver-support.exp (gdbserver_start): Make
+	$server_spawn_id global.
+	(gdbserver_start): Don't wait for gdbserver's spawn id with
+	expect_background.
+	(close_gdbserver): New procedure.
+	(gdb_exit): Rename the default version and reimplement.
+
+2015-04-07  Pedro Alves  <palves@redhat.com>
+
         * lib/gdb.exp (gdb_test_multiple): When processing an argument,
 	append the substituted item, not the original item.
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 203b8cf..1669f45 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4000,20 +4000,6 @@ proc gdb_finish { } {
     global gdb_prompt
     global cleanfiles
 
-    # Give persistent gdbserver a chance to terminate before GDB is killed.
-    if {[info exists gdbserver_reconnect_p] && $gdbserver_reconnect_p
-	&& [info exists gdb_spawn_id]} {
-	send_gdb "kill\n";
-	gdb_expect 10 {
-	    -re "y or n" {
-		send_gdb "y\n";
-		exp_continue;
-	    }
-	    -re "$gdb_prompt $" {
-	    }
-	}
-    }
-
     # Exit first, so that the files are no longer in use.
     gdb_exit
 
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 4a59154..f19b796 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -270,6 +270,7 @@ proc gdbserver_start { options arguments } {
 	    append gdbserver_command " $arguments"
 	}
 
+	global server_spawn_id
 	set server_spawn_id [remote_spawn target $gdbserver_command]
 
 	# Wait for the server to open its TCP socket, so that GDB can connect.
@@ -294,19 +295,6 @@ proc gdbserver_start { options arguments } {
 	break
     }
 
-    # We can't just call close, because if gdbserver is local then that means
-    # that it will get a SIGHUP.  Doing it this way could also allow us to
-    # get at the inferior's input or output if necessary, and means that we
-    # don't need to redirect output.
-    expect_background {
-	-i $server_spawn_id
-	full_buffer { }
-	eof {
-	    # The spawn ID is already closed now (but not yet waited for).
-	    wait -i $expect_out(spawn_id)
-	}
-    }
-
     return [list $protocol [$get_remote_address $debughost $portnum]]
 }
 
@@ -328,6 +316,53 @@ proc gdbserver_spawn { child_args } {
     return [gdbserver_start "" $arguments]
 }
 
+# Close the GDBserver connection.
+
+proc close_gdbserver {} {
+    global server_spawn_id
+
+    # We can't just call close, because if gdbserver is local then that means
+    # that it will get a SIGHUP.  Doing it this way could also allow us to
+    # get at the inferior's input or output if necessary, and means that we
+    # don't need to redirect output.
+
+    if {![info exists server_spawn_id]} {
+	return
+    }
+
+    verbose "Quitting GDBserver"
+
+    catch "close -i $server_spawn_id"
+    catch "wait -i $server_spawn_id"
+    unset server_spawn_id
+}
+
+# Hook into GDB exit, and close GDBserver.
+
+if { [info procs gdbserver_gdb_exit] == "" } {
+    rename gdb_exit gdbserver_orig_gdb_exit
+}
+proc gdb_exit {} {
+    global gdb_spawn_id server_spawn_id
+    global gdb_prompt
+
+    if {[info exists gdb_spawn_id] && [info exists server_spawn_id]} {
+	send_gdb "monitor exit\n";
+	gdb_expect {
+	    -re "$gdb_prompt $" {
+		exp_continue
+	    }
+	    -i "$server_spawn_id" eof {
+		wait -i $expect_out(spawn_id)
+		unset server_spawn_id
+	    }
+	}
+    }
+    close_gdbserver
+
+    gdbserver_orig_gdb_exit
+}
+
 # Start a gdbserver process running HOST_EXEC and pass CHILD_ARGS
 # to it.  Return 0 on success, or non-zero on failure: 2 if gdbserver
 # failed to start or 1 if we couldn't connect to it.