[RFC][gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

[RFC][gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

[Tom de Vries]

Hi,

While running test-case gdb.multi/multi-target.exp, I observed a silent
timeout related to "monitor exit".

By making the timeout explicit in an expect clause in gdbserver_gdb_exit:
...
+  timeout {
+    warning "Timed out waiting for EOF in server after $monitor_exit"
+  }
...
we get in the log:
...
monitor exit^M
"monitor" command not supported by this target.^M
(gdb) WARNING: Timed out waiting for EOF in server after monitor exit
...

What happens is the following:
- the inferior 5 is selected
- a breakpoint is set in inferior 1
- the breakpoint triggers and we switch to inferior 1
- setup is called by test_continue, which calls clean_restarts, which calls
  gdbserver_gdb_exit (due to load_lib gdbserver-support.exp)
- gdbserver_gdb_exit issues "monitor exit"
- gdb responds with "not supported by this target" because inferior 1 is
  native

Fix this by keeping a list of server_spawn_id, and cleaning those up before
calling gdbserver_gdb_exit.

This reduces testing time from 1m22s to 32s.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

gdb/testsuite/ChangeLog:

2020-03-16  Tom de Vries  <tdevries@suse.de>

	* lib/gdbserver-support.exp (gdbserver_exit): Factor out of ...
	(gdbserver_gdb_exit): ... here.  Add timeout warning.
	* gdb.multi/multi-target.exp (server_spawn_ids): New global var.
	(connect_target_extended_remote): Append new server_spawn_id to
	server_spawn_ids.
	(cleanup): New proc.
	(setup, <toplevel>): Call cleanup.

---
 gdb/testsuite/gdb.multi/multi-target.exp | 29 +++++++++++++++++++++++
 gdb/testsuite/lib/gdbserver-support.exp  | 40 +++++++++++++++++++++-----------
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/gdb/testsuite/gdb.multi/multi-target.exp b/gdb/testsuite/gdb.multi/multi-target.exp
index 6c727b5e3b..2e66cb55fa 100644
--- a/gdb/testsuite/gdb.multi/multi-target.exp
+++ b/gdb/testsuite/gdb.multi/multi-target.exp
@@ -33,8 +33,13 @@ if { [prepare_for_testing "failed to prepare" ${binfile} "${srcfile}" \
     return
 }
 
+# Keep list of server_spawn_id
+set server_spawn_ids [list]
+
 proc connect_target_extended_remote {binfile} {
     set res [gdbserver_start "--multi" ""]
+    global server_spawn_ids server_spawn_id
+    lappend server_spawn_ids $server_spawn_id
     set gdbserver_gdbport [lindex $res 1]
     return [gdb_target_cmd "extended-remote" $gdbserver_gdbport]
 }
@@ -99,12 +104,34 @@ proc next_live_inferior {inf} {
     return $inf
 }
 
+# Clean up the server_spawn_ids.
+proc cleanup_gdbservers { } {
+    global server_spawn_id
+    global server_spawn_ids
+    set n 1
+    foreach id $server_spawn_ids {
+	set server_spawn_id $id
+	if { $n == 1 } {
+	    set inferior 2
+	} elseif { $n == 2 } {
+	    set inferior 5
+	} else {
+	    gdb_assert 0 "Unexpected amount of gdb servers"
+	}
+	gdb_test "inferior $inferior"
+	gdbserver_exit 0
+	incr n
+    }
+    set server_spawn_ids [list]
+}
+
 # Return true on success, false otherwise.
 
 proc setup {non-stop} {
     global gcorefile gcore_created
     global binfile
 
+    cleanup_gdbservers
     clean_restart ${binfile}
 
     # multi-target depends on target running in non-stop mode.  Force
@@ -448,3 +475,5 @@ with_test_prefix "info-inferiors" {
 	test_info_inferiors $multi_process
     }
 }
+
+cleanup_gdbservers
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 706bbeb9df..a2cc80f28d 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -431,21 +431,11 @@ if { [info procs gdbserver_orig_gdb_exit] == "" } {
     rename mi_gdb_exit gdbserver_orig_mi_gdb_exit
 }
 
-proc gdbserver_gdb_exit { is_mi } {
+# Cleanup gdbserver $server_spawn_id
+
+proc gdbserver_exit { is_mi } {
     global gdb_spawn_id server_spawn_id
     global gdb_prompt
-    global gdbserver_reconnect_p
-
-    # Leave GDBserver running if we're exiting GDB in order to
-    # reconnect to the same instance of GDBserver again.
-    if {[info exists gdbserver_reconnect_p] && $gdbserver_reconnect_p} {
-	if { $is_mi } {
-	    gdbserver_orig_mi_gdb_exit
-	} else {
-	    gdbserver_orig_gdb_exit
-	}
-	return
-    }
 
     if {[info exists gdb_spawn_id] && [info exists server_spawn_id]} {
 	# GDB may be terminated in an expected way or an unexpected way,
@@ -469,10 +459,34 @@ proc gdbserver_gdb_exit { is_mi } {
 		    wait -i $expect_out(spawn_id)
 		    unset server_spawn_id
 		}
+               timeout {
+                   warning "Timed out waiting for EOF in server after $monitor_exit"
+               }
 	    }
 	}
     }
     close_gdbserver
+}
+
+# Local version of gdb_exit that also cleans up gdbserver $server_spawn_id.
+
+proc gdbserver_gdb_exit { is_mi } {
+    global gdb_spawn_id server_spawn_id
+    global gdb_prompt
+    global gdbserver_reconnect_p
+
+    # Leave GDBserver running if we're exiting GDB in order to
+    # reconnect to the same instance of GDBserver again.
+    if {[info exists gdbserver_reconnect_p] && $gdbserver_reconnect_p} {
+	if { $is_mi } {
+	    gdbserver_orig_mi_gdb_exit
+	} else {
+	    gdbserver_orig_gdb_exit
+	}
+	return
+    }
+
+    gdbserver_exit $is_mi
 
     if { $is_mi } {
 	gdbserver_orig_mi_gdb_exit

[PING][RFC][gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

[Tom de Vries]

On 16-03-2020 18:01, Tom de Vries wrote:
> Hi,
> 
> While running test-case gdb.multi/multi-target.exp, I observed a silent
> timeout related to "monitor exit".
> 
> By making the timeout explicit in an expect clause in gdbserver_gdb_exit:
> ...
> +  timeout {
> +    warning "Timed out waiting for EOF in server after $monitor_exit"
> +  }
> ...
> we get in the log:
> ...
> monitor exit^M
> "monitor" command not supported by this target.^M
> (gdb) WARNING: Timed out waiting for EOF in server after monitor exit
> ...
> 
> What happens is the following:
> - the inferior 5 is selected
> - a breakpoint is set in inferior 1
> - the breakpoint triggers and we switch to inferior 1
> - setup is called by test_continue, which calls clean_restarts, which calls
>   gdbserver_gdb_exit (due to load_lib gdbserver-support.exp)
> - gdbserver_gdb_exit issues "monitor exit"
> - gdb responds with "not supported by this target" because inferior 1 is
>   native
> 
> Fix this by keeping a list of server_spawn_id, and cleaning those up before
> calling gdbserver_gdb_exit.
> 
> This reduces testing time from 1m22s to 32s.
> 
> Any comments?
> 

Ping.

Thanks,
- Tom

> [gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-03-16  Tom de Vries  <tdevries@suse.de>
> 
> 	* lib/gdbserver-support.exp (gdbserver_exit): Factor out of ...
> 	(gdbserver_gdb_exit): ... here.  Add timeout warning.
> 	* gdb.multi/multi-target.exp (server_spawn_ids): New global var.
> 	(connect_target_extended_remote): Append new server_spawn_id to
> 	server_spawn_ids.
> 	(cleanup): New proc.
> 	(setup, <toplevel>): Call cleanup.
> 
> ---
>  gdb/testsuite/gdb.multi/multi-target.exp | 29 +++++++++++++++++++++++
>  gdb/testsuite/lib/gdbserver-support.exp  | 40 +++++++++++++++++++++-----------
>  2 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.multi/multi-target.exp b/gdb/testsuite/gdb.multi/multi-target.exp
> index 6c727b5e3b..2e66cb55fa 100644
> --- a/gdb/testsuite/gdb.multi/multi-target.exp
> +++ b/gdb/testsuite/gdb.multi/multi-target.exp
> @@ -33,8 +33,13 @@ if { [prepare_for_testing "failed to prepare" ${binfile} "${srcfile}" \
>      return
>  }
>  
> +# Keep list of server_spawn_id
> +set server_spawn_ids [list]
> +
>  proc connect_target_extended_remote {binfile} {
>      set res [gdbserver_start "--multi" ""]
> +    global server_spawn_ids server_spawn_id
> +    lappend server_spawn_ids $server_spawn_id
>      set gdbserver_gdbport [lindex $res 1]
>      return [gdb_target_cmd "extended-remote" $gdbserver_gdbport]
>  }
> @@ -99,12 +104,34 @@ proc next_live_inferior {inf} {
>      return $inf
>  }
>  
> +# Clean up the server_spawn_ids.
> +proc cleanup_gdbservers { } {
> +    global server_spawn_id
> +    global server_spawn_ids
> +    set n 1
> +    foreach id $server_spawn_ids {
> +	set server_spawn_id $id
> +	if { $n == 1 } {
> +	    set inferior 2
> +	} elseif { $n == 2 } {
> +	    set inferior 5
> +	} else {
> +	    gdb_assert 0 "Unexpected amount of gdb servers"
> +	}
> +	gdb_test "inferior $inferior"
> +	gdbserver_exit 0
> +	incr n
> +    }
> +    set server_spawn_ids [list]
> +}
> +
>  # Return true on success, false otherwise.
>  
>  proc setup {non-stop} {
>      global gcorefile gcore_created
>      global binfile
>  
> +    cleanup_gdbservers
>      clean_restart ${binfile}
>  
>      # multi-target depends on target running in non-stop mode.  Force
> @@ -448,3 +475,5 @@ with_test_prefix "info-inferiors" {
>  	test_info_inferiors $multi_process
>      }
>  }
> +
> +cleanup_gdbservers
> diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
> index 706bbeb9df..a2cc80f28d 100644
> --- a/gdb/testsuite/lib/gdbserver-support.exp
> +++ b/gdb/testsuite/lib/gdbserver-support.exp
> @@ -431,21 +431,11 @@ if { [info procs gdbserver_orig_gdb_exit] == "" } {
>      rename mi_gdb_exit gdbserver_orig_mi_gdb_exit
>  }
>  
> -proc gdbserver_gdb_exit { is_mi } {
> +# Cleanup gdbserver $server_spawn_id
> +
> +proc gdbserver_exit { is_mi } {
>      global gdb_spawn_id server_spawn_id
>      global gdb_prompt
> -    global gdbserver_reconnect_p
> -
> -    # Leave GDBserver running if we're exiting GDB in order to
> -    # reconnect to the same instance of GDBserver again.
> -    if {[info exists gdbserver_reconnect_p] && $gdbserver_reconnect_p} {
> -	if { $is_mi } {
> -	    gdbserver_orig_mi_gdb_exit
> -	} else {
> -	    gdbserver_orig_gdb_exit
> -	}
> -	return
> -    }
>  
>      if {[info exists gdb_spawn_id] && [info exists server_spawn_id]} {
>  	# GDB may be terminated in an expected way or an unexpected way,
> @@ -469,10 +459,34 @@ proc gdbserver_gdb_exit { is_mi } {
>  		    wait -i $expect_out(spawn_id)
>  		    unset server_spawn_id
>  		}
> +               timeout {
> +                   warning "Timed out waiting for EOF in server after $monitor_exit"
> +               }
>  	    }
>  	}
>      }
>      close_gdbserver
> +}
> +
> +# Local version of gdb_exit that also cleans up gdbserver $server_spawn_id.
> +
> +proc gdbserver_gdb_exit { is_mi } {
> +    global gdb_spawn_id server_spawn_id
> +    global gdb_prompt
> +    global gdbserver_reconnect_p
> +
> +    # Leave GDBserver running if we're exiting GDB in order to
> +    # reconnect to the same instance of GDBserver again.
> +    if {[info exists gdbserver_reconnect_p] && $gdbserver_reconnect_p} {
> +	if { $is_mi } {
> +	    gdbserver_orig_mi_gdb_exit
> +	} else {
> +	    gdbserver_orig_gdb_exit
> +	}
> +	return
> +    }
> +
> +    gdbserver_exit $is_mi
>  
>      if { $is_mi } {
>  	gdbserver_orig_mi_gdb_exit
> 

Re: [PING][RFC][gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

[Tom Tromey]

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

>> Fix this by keeping a list of server_spawn_id, and cleaning those up before
>> calling gdbserver_gdb_exit.
>> 
>> This reduces testing time from 1m22s to 32s.
>> 
>> Any comments?
>> 

Tom> Ping.

Thank you.  This looks good to me.

Tom

Re: [RFC][gdb/testsuite] Fix silent timeout in gdb.multi/multi-target.exp

[Pedro Alves]

Hi,

Thanks for addressing this, and sorry for the delay.

On 3/16/20 5:01 PM, Tom de Vries wrote:
> Hi,
> 
> While running test-case gdb.multi/multi-target.exp, I observed a silent
> timeout related to "monitor exit".
> 
> By making the timeout explicit in an expect clause in gdbserver_gdb_exit:
> ...
> +  timeout {
> +    warning "Timed out waiting for EOF in server after $monitor_exit"
> +  }
> ...
> we get in the log:
> ...
> monitor exit^M
> "monitor" command not supported by this target.^M
> (gdb) WARNING: Timed out waiting for EOF in server after monitor exit
> ...
> 
> What happens is the following:
> - the inferior 5 is selected
> - a breakpoint is set in inferior 1
> - the breakpoint triggers and we switch to inferior 1
> - setup is called by test_continue, which calls clean_restarts, which calls

Typo: clean_restarts -> clean_restart

>   gdbserver_gdb_exit (due to load_lib gdbserver-support.exp)
> - gdbserver_gdb_exit issues "monitor exit"
> - gdb responds with "not supported by this target" because inferior 1 is
>   native
> 
> Fix this by keeping a list of server_spawn_id, and cleaning those up before
> calling gdbserver_gdb_exit.
> 
> This reduces testing time from 1m22s to 32s.
> 
> Any comments?

Looks mostly good to me, though I have a comment below.

> diff --git a/gdb/testsuite/gdb.multi/multi-target.exp b/gdb/testsuite/gdb.multi/multi-target.exp
> index 6c727b5e3b..2e66cb55fa 100644
> --- a/gdb/testsuite/gdb.multi/multi-target.exp
> +++ b/gdb/testsuite/gdb.multi/multi-target.exp
> @@ -33,8 +33,13 @@ if { [prepare_for_testing "failed to prepare" ${binfile} "${srcfile}" \
>      return
>  }
>  
> +# Keep list of server_spawn_id
> +set server_spawn_ids [list]

It'd not a huge deal, but how about making this a list of pairs
of (inferior ID, spawn ID)?  That would make cleanup_gdbservers not have
to hardcode the inferior numbers.  add_inferior, which is
connect_target_extended_remote's caller, has the inferior number
handy already in the NUM parameter, so it'd just need to pass it down
to connect_target_extended_remote.

Thanks,
Pedro Alves