[PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[Andrew Burgess]

Changes in DejaGnu 1.6.2 mean that our testsuite will no longer run.
This is because of some confusion over how the gdb.exp file is
handled.

The gdb.exp file is really the tool init file, which is loaded from
within the DejaGnu core, and it should not be loaded directly from any
other file in the testsuite.

DejaGnu tries to prevent the same library being loaded twice by
remembering the names of library files as they are loaded.  Until
recently loading the tool init file in DejaGnu was very similar to
loading a library file, as a result, loading the gdb.exp tool init
file simply recorded 'gdb.exp' as having been loaded, future attempts
to load 'gdb.exp' as a library would then be ignored (as the file was
marked as already loaded).

DejaGnu has now changed so that it supports having both a tool init
file and a library with the same name, something that was not possible
before.  What this means however is that when the core loads the
'gdb.exp' tool init file it no longer marks the library 'gdb.exp' as
having been loaded.  When we then execute 'load_lib gdb.exp' we then
try to reload the 'gdb.exp' file.

Unfortunately our gdb.exp file can only be loaded once.  It use of
'rename cd builtin_cd' means that a second attempt to load this file
will fail.

This was discussed on the DejaGnu list here:
   http://lists.gnu.org/archive/html/dejagnu/2019-03/msg00000.html

and the suggested advice is that, unless we have some real requirement
to load the tool init file twice, we should remove calls to 'load_lib
gdb.exp' and rely on DejaGnu to load the file for us, which is what
this patch does.

I've tested with native X86-64/GNU Linux and see no regressions.

gdb/testsuite/ChangeLog:

	* config/default.exp: Remove 'load_lib gdb.exp'.
	* config/monitor.exp: Likewise.
	* config/sid.exp: Likewise.
	* config/sim.exp: Likewise.
	* config/slite.exp: Likewise.
	* config/unix.exp: Likewise.
	* gdb.base/default.exp: Remove unhelpful comment.
---
 gdb/testsuite/ChangeLog            | 10 ++++++++++
 gdb/testsuite/config/default.exp   |  2 +-
 gdb/testsuite/config/monitor.exp   |  3 ---
 gdb/testsuite/config/sid.exp       |  2 --
 gdb/testsuite/config/sim.exp       |  2 --
 gdb/testsuite/config/slite.exp     |  2 +-
 gdb/testsuite/config/unix.exp      |  2 --
 gdb/testsuite/gdb.base/default.exp |  2 --
 8 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/testsuite/config/default.exp b/gdb/testsuite/config/default.exp
index 8b70ee4b507..325a58851ec 100644
--- a/gdb/testsuite/config/default.exp
+++ b/gdb/testsuite/config/default.exp
@@ -13,4 +13,4 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-load_lib gdb.exp
+# Nothing extra is needed here.
diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
index be8d8429b8d..48a01feb706 100644
--- a/gdb/testsuite/config/monitor.exp
+++ b/gdb/testsuite/config/monitor.exp
@@ -14,9 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-load_lib gdb.exp
-# puts "***** DID USE MONITOR ******"
-
 #
 # gdb_target_cmd
 # Send gdb the "target" command
diff --git a/gdb/testsuite/config/sid.exp b/gdb/testsuite/config/sid.exp
index 3c92a4fe9f5..17a3ad568d9 100644
--- a/gdb/testsuite/config/sid.exp
+++ b/gdb/testsuite/config/sid.exp
@@ -14,8 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-load_lib gdb.exp
-
 proc sid_start {} {
     global verbose
 
diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp
index dafb1a26063..8d87e3089b0 100644
--- a/gdb/testsuite/config/sim.exp
+++ b/gdb/testsuite/config/sim.exp
@@ -14,8 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-load_lib gdb.exp
-
 #
 # gdb_target_sim
 # Set gdb to target the simulator
diff --git a/gdb/testsuite/config/slite.exp b/gdb/testsuite/config/slite.exp
index 0fa472ee0cb..ffbd9c811e8 100644
--- a/gdb/testsuite/config/slite.exp
+++ b/gdb/testsuite/config/slite.exp
@@ -27,7 +27,7 @@
 # rather than being displayed by gdb.
 
 load_lib remote.exp
-load_lib gdb.exp
+
 set gdb_prompt "\\(gdb\\)"
 
 #
diff --git a/gdb/testsuite/config/unix.exp b/gdb/testsuite/config/unix.exp
index 2ab1d9abf0c..770e69b4c32 100644
--- a/gdb/testsuite/config/unix.exp
+++ b/gdb/testsuite/config/unix.exp
@@ -21,5 +21,3 @@
 # does not seem to be enough.  Try starting with 60.
 set timeout 60
 verbose "Timeout is now $timeout seconds" 2
-
-load_lib gdb.exp
diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
index 8a11401c79f..ece1428e617 100644
--- a/gdb/testsuite/gdb.base/default.exp
+++ b/gdb/testsuite/gdb.base/default.exp
@@ -25,8 +25,6 @@ set timeout 60
 # test default actions of gdb commands
 #
 
-#load_lib gdb.exp
-
 gdb_test "add-symbol-file" "add-symbol-file takes a file name and an address" "add-symbol-file"
 
 # test append
-- 
2.14.5

Re: [PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[Sergio Durigan Junior]

On Thursday, March 07 2019, Andrew Burgess wrote:

> Changes in DejaGnu 1.6.2 mean that our testsuite will no longer run.
> This is because of some confusion over how the gdb.exp file is
> handled.
>
> The gdb.exp file is really the tool init file, which is loaded from
> within the DejaGnu core, and it should not be loaded directly from any
> other file in the testsuite.
>
> DejaGnu tries to prevent the same library being loaded twice by
> remembering the names of library files as they are loaded.  Until
> recently loading the tool init file in DejaGnu was very similar to
> loading a library file, as a result, loading the gdb.exp tool init
> file simply recorded 'gdb.exp' as having been loaded, future attempts
> to load 'gdb.exp' as a library would then be ignored (as the file was
> marked as already loaded).
>
> DejaGnu has now changed so that it supports having both a tool init
> file and a library with the same name, something that was not possible
> before.  What this means however is that when the core loads the
> 'gdb.exp' tool init file it no longer marks the library 'gdb.exp' as
> having been loaded.  When we then execute 'load_lib gdb.exp' we then
> try to reload the 'gdb.exp' file.
>
> Unfortunately our gdb.exp file can only be loaded once.  It use of
> 'rename cd builtin_cd' means that a second attempt to load this file
> will fail.
>
> This was discussed on the DejaGnu list here:
>    http://lists.gnu.org/archive/html/dejagnu/2019-03/msg00000.html
>
> and the suggested advice is that, unless we have some real requirement
> to load the tool init file twice, we should remove calls to 'load_lib
> gdb.exp' and rely on DejaGnu to load the file for us, which is what
> this patch does.
>
> I've tested with native X86-64/GNU Linux and see no regressions.

FWIW, I've always found these "load_lib gdb.exp" to be remnants of an
old past, and not needed anymore (or even buggy).  Thanks for the
investigation; IMO, the patch is fine could almost be considered an
"obvious" cleanup.  I'm not a global maintainer, though.

Thanks,

> gdb/testsuite/ChangeLog:
>
> 	* config/default.exp: Remove 'load_lib gdb.exp'.
> 	* config/monitor.exp: Likewise.
> 	* config/sid.exp: Likewise.
> 	* config/sim.exp: Likewise.
> 	* config/slite.exp: Likewise.
> 	* config/unix.exp: Likewise.
> 	* gdb.base/default.exp: Remove unhelpful comment.
> ---
>  gdb/testsuite/ChangeLog            | 10 ++++++++++
>  gdb/testsuite/config/default.exp   |  2 +-
>  gdb/testsuite/config/monitor.exp   |  3 ---
>  gdb/testsuite/config/sid.exp       |  2 --
>  gdb/testsuite/config/sim.exp       |  2 --
>  gdb/testsuite/config/slite.exp     |  2 +-
>  gdb/testsuite/config/unix.exp      |  2 --
>  gdb/testsuite/gdb.base/default.exp |  2 --
>  8 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/testsuite/config/default.exp b/gdb/testsuite/config/default.exp
> index 8b70ee4b507..325a58851ec 100644
> --- a/gdb/testsuite/config/default.exp
> +++ b/gdb/testsuite/config/default.exp
> @@ -13,4 +13,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -load_lib gdb.exp
> +# Nothing extra is needed here.
> diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
> index be8d8429b8d..48a01feb706 100644
> --- a/gdb/testsuite/config/monitor.exp
> +++ b/gdb/testsuite/config/monitor.exp
> @@ -14,9 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -load_lib gdb.exp
> -# puts "***** DID USE MONITOR ******"
> -
>  #
>  # gdb_target_cmd
>  # Send gdb the "target" command
> diff --git a/gdb/testsuite/config/sid.exp b/gdb/testsuite/config/sid.exp
> index 3c92a4fe9f5..17a3ad568d9 100644
> --- a/gdb/testsuite/config/sid.exp
> +++ b/gdb/testsuite/config/sid.exp
> @@ -14,8 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -load_lib gdb.exp
> -
>  proc sid_start {} {
>      global verbose
>  
> diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp
> index dafb1a26063..8d87e3089b0 100644
> --- a/gdb/testsuite/config/sim.exp
> +++ b/gdb/testsuite/config/sim.exp
> @@ -14,8 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -load_lib gdb.exp
> -
>  #
>  # gdb_target_sim
>  # Set gdb to target the simulator
> diff --git a/gdb/testsuite/config/slite.exp b/gdb/testsuite/config/slite.exp
> index 0fa472ee0cb..ffbd9c811e8 100644
> --- a/gdb/testsuite/config/slite.exp
> +++ b/gdb/testsuite/config/slite.exp
> @@ -27,7 +27,7 @@
>  # rather than being displayed by gdb.
>  
>  load_lib remote.exp
> -load_lib gdb.exp
> +
>  set gdb_prompt "\\(gdb\\)"
>  
>  #
> diff --git a/gdb/testsuite/config/unix.exp b/gdb/testsuite/config/unix.exp
> index 2ab1d9abf0c..770e69b4c32 100644
> --- a/gdb/testsuite/config/unix.exp
> +++ b/gdb/testsuite/config/unix.exp
> @@ -21,5 +21,3 @@
>  # does not seem to be enough.  Try starting with 60.
>  set timeout 60
>  verbose "Timeout is now $timeout seconds" 2
> -
> -load_lib gdb.exp
> diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
> index 8a11401c79f..ece1428e617 100644
> --- a/gdb/testsuite/gdb.base/default.exp
> +++ b/gdb/testsuite/gdb.base/default.exp
> @@ -25,8 +25,6 @@ set timeout 60
>  # test default actions of gdb commands
>  #
>  
> -#load_lib gdb.exp
> -
>  gdb_test "add-symbol-file" "add-symbol-file takes a file name and an address" "add-symbol-file"
>  
>  # test append
> -- 
> 2.14.5

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> FWIW, I've always found these "load_lib gdb.exp" to be remnants of an
Sergio> old past, and not needed anymore (or even buggy).  Thanks for the
Sergio> investigation; IMO, the patch is fine could almost be considered an
Sergio> "obvious" cleanup.  I'm not a global maintainer, though.

Thanks for looking at this.
This is ok.

Tom

Re: [PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[Pedro Franco de Carvalho]

Hello,

Thanks for this fix.

I noticed that the 8.3 branch has the commit that redefines "cd" that
triggered the issue fixed here, but this fix is not present there.

Would it make sense to include this in the 8.3 branch?

--
Pedro Franco de Carvalho

Andrew Burgess <andrew.burgess@embecosm.com> writes:

> Changes in DejaGnu 1.6.2 mean that our testsuite will no longer run.
> This is because of some confusion over how the gdb.exp file is
> handled.
>
> The gdb.exp file is really the tool init file, which is loaded from
> within the DejaGnu core, and it should not be loaded directly from any
> other file in the testsuite.
>
> DejaGnu tries to prevent the same library being loaded twice by
> remembering the names of library files as they are loaded.  Until
> recently loading the tool init file in DejaGnu was very similar to
> loading a library file, as a result, loading the gdb.exp tool init
> file simply recorded 'gdb.exp' as having been loaded, future attempts
> to load 'gdb.exp' as a library would then be ignored (as the file was
> marked as already loaded).
>
> DejaGnu has now changed so that it supports having both a tool init
> file and a library with the same name, something that was not possible
> before.  What this means however is that when the core loads the
> 'gdb.exp' tool init file it no longer marks the library 'gdb.exp' as
> having been loaded.  When we then execute 'load_lib gdb.exp' we then
> try to reload the 'gdb.exp' file.
>
> Unfortunately our gdb.exp file can only be loaded once.  It use of
> 'rename cd builtin_cd' means that a second attempt to load this file
> will fail.
>
> This was discussed on the DejaGnu list here:
>    http://lists.gnu.org/archive/html/dejagnu/2019-03/msg00000.html
>
> and the suggested advice is that, unless we have some real requirement
> to load the tool init file twice, we should remove calls to 'load_lib
> gdb.exp' and rely on DejaGnu to load the file for us, which is what
> this patch does.
>
> I've tested with native X86-64/GNU Linux and see no regressions.
>
> gdb/testsuite/ChangeLog:
>
> 	* config/default.exp: Remove 'load_lib gdb.exp'.
> 	* config/monitor.exp: Likewise.
> 	* config/sid.exp: Likewise.
> 	* config/sim.exp: Likewise.
> 	* config/slite.exp: Likewise.
> 	* config/unix.exp: Likewise.
> 	* gdb.base/default.exp: Remove unhelpful comment.
> ---
>  gdb/testsuite/ChangeLog            | 10 ++++++++++
>  gdb/testsuite/config/default.exp   |  2 +-
>  gdb/testsuite/config/monitor.exp   |  3 ---
>  gdb/testsuite/config/sid.exp       |  2 --
>  gdb/testsuite/config/sim.exp       |  2 --
>  gdb/testsuite/config/slite.exp     |  2 +-
>  gdb/testsuite/config/unix.exp      |  2 --
>  gdb/testsuite/gdb.base/default.exp |  2 --
>  8 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/testsuite/config/default.exp b/gdb/testsuite/config/default.exp
> index 8b70ee4b507..325a58851ec 100644
> --- a/gdb/testsuite/config/default.exp
> +++ b/gdb/testsuite/config/default.exp
> @@ -13,4 +13,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> -load_lib gdb.exp
> +# Nothing extra is needed here.
> diff --git a/gdb/testsuite/config/monitor.exp b/gdb/testsuite/config/monitor.exp
> index be8d8429b8d..48a01feb706 100644
> --- a/gdb/testsuite/config/monitor.exp
> +++ b/gdb/testsuite/config/monitor.exp
> @@ -14,9 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> -load_lib gdb.exp
> -# puts "***** DID USE MONITOR ******"
> -
>  #
>  # gdb_target_cmd
>  # Send gdb the "target" command
> diff --git a/gdb/testsuite/config/sid.exp b/gdb/testsuite/config/sid.exp
> index 3c92a4fe9f5..17a3ad568d9 100644
> --- a/gdb/testsuite/config/sid.exp
> +++ b/gdb/testsuite/config/sid.exp
> @@ -14,8 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> -load_lib gdb.exp
> -
>  proc sid_start {} {
>      global verbose
>
> diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp
> index dafb1a26063..8d87e3089b0 100644
> --- a/gdb/testsuite/config/sim.exp
> +++ b/gdb/testsuite/config/sim.exp
> @@ -14,8 +14,6 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> -load_lib gdb.exp
> -
>  #
>  # gdb_target_sim
>  # Set gdb to target the simulator
> diff --git a/gdb/testsuite/config/slite.exp b/gdb/testsuite/config/slite.exp
> index 0fa472ee0cb..ffbd9c811e8 100644
> --- a/gdb/testsuite/config/slite.exp
> +++ b/gdb/testsuite/config/slite.exp
> @@ -27,7 +27,7 @@
>  # rather than being displayed by gdb.
>
>  load_lib remote.exp
> -load_lib gdb.exp
> +
>  set gdb_prompt "\\(gdb\\)"
>
>  #
> diff --git a/gdb/testsuite/config/unix.exp b/gdb/testsuite/config/unix.exp
> index 2ab1d9abf0c..770e69b4c32 100644
> --- a/gdb/testsuite/config/unix.exp
> +++ b/gdb/testsuite/config/unix.exp
> @@ -21,5 +21,3 @@
>  # does not seem to be enough.  Try starting with 60.
>  set timeout 60
>  verbose "Timeout is now $timeout seconds" 2
> -
> -load_lib gdb.exp
> diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
> index 8a11401c79f..ece1428e617 100644
> --- a/gdb/testsuite/gdb.base/default.exp
> +++ b/gdb/testsuite/gdb.base/default.exp
> @@ -25,8 +25,6 @@ set timeout 60
>  # test default actions of gdb commands
>  #
>
> -#load_lib gdb.exp
> -
>  gdb_test "add-symbol-file" "add-symbol-file takes a file name and an address" "add-symbol-file"
>
>  # test append
> -- 
> 2.14.5

Re: [PATCH] gdb/testsuite: Prepare for DejaGnu 1.6.2

[Joel Brobecker]

> Thanks for this fix.
> 
> I noticed that the 8.3 branch has the commit that redefines "cd" that
> triggered the issue fixed here, but this fix is not present there.
> 
> Would it make sense to include this in the 8.3 branch?

I looked at the patch, and it looked safe to me. I also ran
the testsuite with an older version of dejagnu (1.5.1), and
it ran fine. So I backported the patch to the 8.3 branch.

> > gdb/testsuite/ChangeLog:
> >
> > 	* config/default.exp: Remove 'load_lib gdb.exp'.
> > 	* config/monitor.exp: Likewise.
> > 	* config/sid.exp: Likewise.
> > 	* config/sim.exp: Likewise.
> > 	* config/slite.exp: Likewise.
> > 	* config/unix.exp: Likewise.
> > 	* gdb.base/default.exp: Remove unhelpful comment.

-- 
Joel