[PATCH] Implement ${board}_{exec,spawn} for native-extended-gdbserver

[PATCH] Implement ${board}_{exec,spawn} for native-extended-gdbserver

[Sergio Durigan Junior]

Hi,

Mark Wielaard is kindly running two Debian buildslaves in our
buildbot, and he noticed that, when testing things under the
native-extended-gdbserver board, there is a huge slowndown due to
several timeouts that are happening.

If you look at the following log:

  <http://gdb-build.sergiodj.net/builders/Debian-i686-native-extended-gdbserver/builds/48/steps/test%20gdb/logs/stdio>

You will notice that this happens because dejagnu is trying to open a
rsh connection to test native-extended-gdbserver, which of course is
wrong, and we end up not testing things properly.

After some investigation (motivated by an idea from Pedro), I found
that our native-extended-gdbserver board does not set the "isremote"
flag properly.  I fixed that by doing two things:

- Defining the ${board}_spawn procedure inside the board file, and

- Moving the ${board}_exec procedure (present at the native-gdbserver
  and native-stdio-gdbserver boards) to the gdbserver-base file.

After that, I am not seeing this failure anymore.

Is this OK to check in?

gdb/testsuite/ChangeLog:
2015-02-03  Sergio Durigan Junior  <sergiodj@redhat.com>

	* boards/gdbserver-base.exp (${board}_exec): New procedure, moved
	from native-gdbserver.exp and native-stdio-gdbserver.exp.
	* boards/native-extended-gdbserver.exp: Do not set isremote using
	board_info.
	(${board}_spawn): New procedure.
	* boards/native-gdbserver.exp (${board}_exec): Moved to
	gdbserver-base.exp.
	* boards/native-stdio-gdbserver.exp (${board}_exec): Likewise.
---
 gdb/testsuite/boards/gdbserver-base.exp            | 12 ++++++++++++
 gdb/testsuite/boards/native-extended-gdbserver.exp | 18 ++++++++++++------
 gdb/testsuite/boards/native-gdbserver.exp          | 12 ------------
 gdb/testsuite/boards/native-stdio-gdbserver.exp    | 12 ------------
 4 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
index c77dc6e..1a52915 100644
--- a/gdb/testsuite/boards/gdbserver-base.exp
+++ b/gdb/testsuite/boards/gdbserver-base.exp
@@ -50,3 +50,15 @@ proc ${board}_download { board host dest } {
 proc ${board}_upload {dest srcfile args} {
     return $srcfile
 }
+
+proc ${board}_exec { hostname program args } {
+    global board_info
+
+    set baseboard [lindex [split $hostname "/"] 0]
+
+    set board_info($baseboard,isremote) 0
+    set result [remote_exec $hostname $program $args]
+    set board_info($baseboard,isremote) 1
+
+    return $result
+}
diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 57503db..0059c4e 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -23,12 +23,6 @@
 load_generic_config "extended-gdbserver"
 load_board_description "gdbserver-base"
 
-# By default, dejagnu makes the board remote unless the board name
-# matches localhost.  Force it to be NOT remote.
-global board
-global board_info
-set board_info($board,isremote) 0
-
 set_board_info sockethost "localhost:"
 
 # We will be using the extended GDB remote protocol.
@@ -119,3 +113,15 @@ proc mi_gdb_load { arg } {
 
     return 0
 }
+
+proc ${board}_spawn { board cmd } {
+    global board_info
+
+    set baseboard [lindex [split $board "/"] 0]
+
+    set board_info($baseboard,isremote) 0
+    set result [remote_spawn $board $cmd]
+    set board_info($baseboard,isremote) 1
+
+    return $result
+}
diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index 7738b2e..3d972ba 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -47,15 +47,3 @@ proc ${board}_spawn { board cmd } {
 
     return $result
 }
-
-proc ${board}_exec { hostname program args } {
-    global board_info
-
-    set baseboard [lindex [split $hostname "/"] 0]
-
-    set board_info($baseboard,isremote) 0
-    set result [remote_exec $hostname $program $args]
-    set board_info($baseboard,isremote) 1
-
-    return $result
-}
diff --git a/gdb/testsuite/boards/native-stdio-gdbserver.exp b/gdb/testsuite/boards/native-stdio-gdbserver.exp
index 2ec2a47..b8d0949 100644
--- a/gdb/testsuite/boards/native-stdio-gdbserver.exp
+++ b/gdb/testsuite/boards/native-stdio-gdbserver.exp
@@ -75,15 +75,3 @@ proc ${board}_spawn { board cmd } {
 
     return $result
 }
-
-proc ${board}_exec { hostname program args } {
-    global board_info
-
-    set baseboard [lindex [split $hostname "/"] 0]
-
-    set board_info($baseboard,isremote) 0
-    set result [remote_exec $hostname $program $args]
-    set board_info($baseboard,isremote) 1
-
-    return $result
-}
-- 
1.9.3

Re: [PATCH] Implement ${board}_{exec,spawn} for native-extended-gdbserver

[Pedro Alves]

On 02/04/2015 12:20 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
> index 57503db..0059c4e 100644
> --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
> +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
> @@ -23,12 +23,6 @@
>  load_generic_config "extended-gdbserver"
>  load_board_description "gdbserver-base"
>  
> -# By default, dejagnu makes the board remote unless the board name
> -# matches localhost.  Force it to be NOT remote.
> -global board
> -global board_info
> -set board_info($board,isremote) 0

I don't think we can/should remove this.  That'll make
"is_remote" checks all over the testsuite return true,
and thus skip tests that weren't skipped before.  E.g.,
solib-display.exp.

I think we need to instead do here what ${board}_spawn
is already doing.  That is, remove any target variant
specifications from $board, like:

 --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
 +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
 @@ -27,7 +27,9 @@ load_board_description "gdbserver-base"
  # matches localhost.  Force it to be NOT remote.
  global board
  global board_info
 -set board_info($board,isremote) 0
 +# Remove any target variant specifications from the name.
 +set baseboard [lindex [split $board "/"] 0]
 +set board_info($baseboard,isremote) 0

I'm testing that.

Thanks,
Pedro Alves

Re: [PATCH] Implement ${board}_{exec,spawn} for native-extended-gdbserver

[Pedro Alves]

On 02/04/2015 02:05 PM, Pedro Alves wrote:
> On 02/04/2015 12:20 AM, Sergio Durigan Junior wrote:

>> -# By default, dejagnu makes the board remote unless the board name
>> -# matches localhost.  Force it to be NOT remote.
>> -global board
>> -global board_info
>> -set board_info($board,isremote) 0
> 
> I don't think we can/should remove this.  That'll make
> "is_remote" checks all over the testsuite return true,
> and thus skip tests that weren't skipped before.  E.g.,
> solib-display.exp.

I've confirmed this.

> I think we need to instead do here what ${board}_spawn
> is already doing.  That is, remove any target variant
> specifications from $board, like:
> 
>  --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
>  +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
>  @@ -27,7 +27,9 @@ load_board_description "gdbserver-base"
>   # matches localhost.  Force it to be NOT remote.
>   global board
>   global board_info
>  -set board_info($board,isremote) 0
>  +# Remove any target variant specifications from the name.
>  +set baseboard [lindex [split $board "/"] 0]
>  +set board_info($baseboard,isremote) 0
> 
> I'm testing that.

It works.  I've pushed the patch below now, to unblock the builder.

More could/should be done though.  A number of boards do "set_board_info isremote 0",
so should be failing in the same way.  Sounds like it'd be good to have a
shared file all those boards would source, and we'd move
native-extended-gdbserver.exp's version there.  (TBC, I'm not planning on
doing that myself.)

--- 
From 42d9e5288b6576b56f3f803901433b88ee863bc6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 4 Feb 2015 14:53:24 +0100
Subject: [PATCH] Fix '--target_board=native-extended-gdbserver/-m32'

Running the testsuite with the native-extended-gdbserver.exp board and
passing a variant spec, like

  make check RUNTESTFLAGS="--target_board=native-extended-gdbserver/-m32"

results in dejagnu trying to open a rsh connection to
"native-extended-gdbserver", which of course is wrong.  The point of
this board is running things locally.

The issue is that the native-extended-gdbserver board does not clear
the "isremote" flag properly.

Reported by Sergio at:
  https://sourceware.org/ml/gdb-patches/2015-02/msg00067.html

testsuite/
2015-02-04  Pedro Alves  <palves@redhat.com>

	* boards/native-extended-gdbserver.exp: Remove any target variant
	specifications from the board name before clearing the isremote
	flag from board_info.
---
 gdb/testsuite/ChangeLog                            | 6 ++++++
 gdb/testsuite/boards/native-extended-gdbserver.exp | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 2657987..fcc16cb 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-02-04  Pedro Alves  <palves@redhat.com>
+
+	* boards/native-extended-gdbserver.exp: Remove any target variant
+	specifications from the board name before clearing the isremote
+	flag from board_info.
+
 2015-01-31  Doug Evans  <xdje42@gmail.com>
 
 	* gdb.base/maint.exp <maint print type argc>: Update expected output.
diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 57503db..8d299ef 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -27,7 +27,9 @@ load_board_description "gdbserver-base"
 # matches localhost.  Force it to be NOT remote.
 global board
 global board_info
-set board_info($board,isremote) 0
+# Remove any target variant specifications from the name.
+set baseboard [lindex [split $board "/"] 0]
+set board_info($baseboard,isremote) 0
 
 set_board_info sockethost "localhost:"
 
-- 
1.9.3

Re: [PATCH] Implement ${board}_{exec,spawn} for native-extended-gdbserver

[Doug Evans]

Pedro Alves <palves@redhat.com> writes:
> On 02/04/2015 12:20 AM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
>> index 57503db..0059c4e 100644
>> --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
>> +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
>> @@ -23,12 +23,6 @@
>>  load_generic_config "extended-gdbserver"
>>  load_board_description "gdbserver-base"
>>  
>> -# By default, dejagnu makes the board remote unless the board name
>> -# matches localhost.  Force it to be NOT remote.
>> -global board
>> -global board_info
>> -set board_info($board,isremote) 0
>
> I don't think we can/should remove this.  That'll make
> "is_remote" checks all over the testsuite return true,
> and thus skip tests that weren't skipped before.  E.g.,
> solib-display.exp.
>
> I think we need to instead do here what ${board}_spawn
> is already doing.  That is, remove any target variant
> specifications from $board, like:
>
>  --- a/gdb/testsuite/boards/native-extended-gdbserver.exp
>  +++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
>  @@ -27,7 +27,9 @@ load_board_description "gdbserver-base"
>   # matches localhost.  Force it to be NOT remote.
>   global board
>   global board_info
>  -set board_info($board,isremote) 0
>  +# Remove any target variant specifications from the name.
>  +set baseboard [lindex [split $board "/"] 0]
>  +set board_info($baseboard,isremote) 0
>
> I'm testing that.

Eewwww....

[not your patch, just the fact that $board ends up with entire string,
including, e.g., /-m32]

We should document this somewhere, as I can imagine it being
all too easy to continue to just use $board without realizing this.
A little grepping makes me wonder.
E.g., all the procs with ${board} as a prefix.
bash$ grep -F '${board}' */*.exp
proc ${board}_file { dest op args } {
...

and sure enough, those procs get "/-m32" in the name too.
I added
verbose -log "[info procs *${board}*]" to gdbserver-base.exp
and got
native-gdbserver/-m32_file native-gdbserver/-m32_download native-gdbserver/-m32_upload

Yikes.

btw, The name "baseboard" is a bit confusing because I immediately
think of /usr/share/dejagnu/baseboards, which is a different thing.