[PATCH] gdb/testsuite: Regenerate the testglue if it is not in path

[PATCH] gdb/testsuite: Regenerate the testglue if it is not in path

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

For running the  DejaGnu  tests,  some  esoteric  configurations
may require a testglue.   This,  for  instance,  is  true  about
testing ARC  targets  which  uses  its  own  DejaGnu  board  and
a simulator which does not support returning the  pass  or  fail
through the exit code.  Therefore,  for  those  tests  that  use
"gdb_compile" a "gdb_tg.o" file is compiled and  linked  to  the
final executable.

There  are  tests  that  invoke  "gdb_compile"  from   different
directories.   Let's  take  a   look   at   an   example   test:
gdb.base/fullname.exp.  The purpose of this  test  is  to  build
the executable from different directories (absolute vs. relative
vs.  other) and then check if gdb can handle setting breakpoints
accordingly.

When  "gdb_compile"  generates  the  "gdb_tg.o",  it  does   not
do it again  for  the  same  test.   Although  this  might  seem
efficient, it can lead to  problems  when  changing  directories
before the next compile:

  gdb compile failed, arc-elf32-gcc: error: gdb_tg.o:
  No such file or directory

This patch checks if the wrapper file ("gdb_tg.o") is  still  in
reach and if it is not, it will stimulate the generation of  the
wrapper again.

It is worth mentioning that GCC's  DejaGnu  tests  handle  these
scenarios as well and they seem to be more efficient in doing so
by saving the library paths and manipulating them  if  necessary
[1].  However, for GDB tests, that  require  less  compilations,
I think the proposed solution should be fine compared to a  more
full fledged solution from GCC.  The glue file in  our  case  is
only 2 KiB.

Last but not least, I ran the x86_64 tests on an x86_64 host and
found no regression.

[1]
Avid  coders  may  look  for  "set_ld_library_path_env_vars"  in
gcc/testsuite/lib/target-libpath.exp.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_compile): Reset
	"gdb_wrapper_initialized" to 0 if "wrapper_file" does
	not exist.
---
 gdb/testsuite/lib/gdb.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index d5e22957039..8e91c91e715 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3812,6 +3812,15 @@ proc gdb_compile {source dest type options} {
     verbose "options are $options"
     verbose "source is $source $dest $type $options"
 
+    # if the wrapper is initialized but the wrapper file cannot be
+    # found anymore, the wrapper file must be built again.
+    if { $gdb_wrapper_initialized == 1 && \
+	    [info exists gdb_wrapper_file] && \
+	    ![file exists $gdb_wrapper_file] } {
+	verbose "reinitializing the wrapper"
+	set gdb_wrapper_initialized 0
+    }
+
     if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
 
     if {[target_info exists needs_status_wrapper] && \
-- 
2.25.0

Re: [PATCH] gdb/testsuite: Regenerate the testglue if it is not in path

[Tom Tromey]

>>>>> "Shahab" == Shahab Vahedi <shahab.vahedi@gmail.com> writes:

Shahab> For running the  DejaGnu  tests,  some  esoteric  configurations
Shahab> may require a testglue.

Thank you for the patch.  I wasn't aware of this code at all :-)

Shahab> This patch checks if the wrapper file ("gdb_tg.o") is  still  in
Shahab> reach and if it is not, it will stimulate the generation of  the
Shahab> wrapper again.

I wonder if it would make sense to instead set gdb_wrapper_file to the
absolute path.

It could perhaps be put in the cache directory.  This might help avoid
recompiling it many times when running the tests in parallel.

What do you think of that?

Shahab> +    # if the wrapper is initialized but the wrapper file cannot be

GNU style is to start a comment with an upper-case letter, so "If".

Shahab> +    # found anymore, the wrapper file must be built again.
Shahab> +    if { $gdb_wrapper_initialized == 1 && \
Shahab> +	    [info exists gdb_wrapper_file] && \
Shahab> +	    ![file exists $gdb_wrapper_file] } {
Shahab> +	verbose "reinitializing the wrapper"
Shahab> +	set gdb_wrapper_initialized 0
Shahab> +    }
Shahab> +
Shahab>      if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }

If you don't like the absolute path idea, maybe just having all this
logic be in the wrapper code would be better, and this could just call
gdb_wrapper_init unconditionally.

Tom

Re: [PATCH] gdb/testsuite: Regenerate the testglue if it is not in path

[Shahab Vahedi]

On Tue, Feb 11, 2020 at 08:39:03AM -0700, Tom Tromey wrote:
> >>>>> "Shahab" == Shahab Vahedi <shahab.vahedi@gmail.com> writes:
> 
> Shahab> For running the  DejaGnu  tests,  some  esoteric  configurations
> Shahab> may require a testglue.
> 
> Thank you for the patch.  I wasn't aware of this code at all :-)
> 
> Shahab> This patch checks if the wrapper file ("gdb_tg.o") is  still  in
> Shahab> reach and if it is not, it will stimulate the generation of  the
> Shahab> wrapper again.
> 
> I wonder if it would make sense to instead set gdb_wrapper_file to the
> absolute path.

This is how "gdb_wrapper_file" is set in gdb/testsuite/lib/gdb.exp:

  proc gdb_wrapper_init { args } {
  ...
  global gdb_wrapper_file
  ...
      set result [build_wrapper "testglue.o"]
      if { $result != "" } {
          set gdb_wrapper_file [lindex $result 0]
          ...
  }

The "build_wrapper" is a procedure form DejaGnu  itself  (libgloss.exp),
and it does not provide the full path.  Maybe we  can  get  the  current
working directory from somewhere else and append it here?

> It could perhaps be put in the cache directory.  This might help avoid
> recompiling it many times when running the tests in parallel.
> 
> What do you think of that?

I absolutely like this idea and believe this is how it should  be  done,
but am not sure where to begin. Any pointers?

> 
> Shahab> +    # if the wrapper is initialized but the wrapper file cannot be
> 
> GNU style is to start a comment with an upper-case letter, so "If".

I will fix this in next patch (whatever approach we take).

> 
> Shahab> +    # found anymore, the wrapper file must be built again.
> Shahab> +    if { $gdb_wrapper_initialized == 1 && \
> Shahab> +	    [info exists gdb_wrapper_file] && \
> Shahab> +	    ![file exists $gdb_wrapper_file] } {
> Shahab> +	verbose "reinitializing the wrapper"
> Shahab> +	set gdb_wrapper_initialized 0
> Shahab> +    }
> Shahab> +
> Shahab>      if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
> 
> If you don't like the absolute path idea, maybe just having all this
> logic be in the wrapper code would be better, and this could just call
> gdb_wrapper_init unconditionally.

I agree with your suggestion and if we opt for  this  approach,  I  will
adapt it as such.

> 
> Tom

Cheers,
Shahab

[PATCH v2] gdb/testsuite: Regenerate the testglue if it is not in path

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

For running the  DejaGnu  tests,  some  esoteric  configurations
may require a testglue.   This,  for  instance,  is  true  about
testing ARC  targets  which  uses  its  own  DejaGnu  board  and
a simulator which does not support returning the  pass  or  fail
through the exit code.  Therefore,  for  those  tests  that  use
"gdb_compile" a "gdb_tg.o" file is compiled and  linked  to  the
final executable.

There  are  tests  that  invoke  "gdb_compile"  from   different
directories.   Let's  take  a   look   at   an   example   test:
gdb.base/fullname.exp.  The purpose of this  test  is  to  build
the executable from different directories (absolute vs. relative
vs.  other) and then check if gdb can handle setting breakpoints
accordingly.

When  "gdb_compile"  generates  the  "gdb_tg.o",  it  does   not
do it again  for  the  same  test.   Although  this  might  seem
efficient, it can lead to  problems  when  changing  directories
before the next compile:

  gdb compile failed, arc-elf32-gcc: error: gdb_tg.o:
  No such file or directory

This patch checks if the wrapper file ("gdb_tg.o") is  still  in
reach and if it is not, it will stimulate the generation of  the
wrapper again.

It is worth mentioning that GCC's  DejaGnu  tests  handle  these
scenarios as well and they seem to be more efficient in doing so
by saving the library paths and manipulating them  if  necessary
[1].  However, for GDB tests, that  require  less  compilations,
I think the proposed solution should be fine compared to a  more
full fledged solution from GCC.  The glue file in  our  case  is
only 2 KiB.

Last but not least, I ran the x86_64 tests on an x86_64 host and
found no regression.

[1]
Avid  coders  may  look  for  "set_ld_library_path_env_vars"  in
gcc/testsuite/lib/target-libpath.exp.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_wrapper_init): Reset
	"gdb_wrapper_initialized" to 0 if "wrapper_file" does
	not exist.
---
 gdb/testsuite/lib/gdb.exp | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index d5e22957039..8f55c4fcb2d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3565,6 +3565,15 @@ proc gdb_wrapper_init { args } {
     global gdb_wrapper_flags
     global gdb_wrapper_target
 
+    # if the wrapper is initialized but the wrapper file cannot be
+    # found anymore, the wrapper file must be built again.
+    if { $gdb_wrapper_initialized == 1 && \
+	    [info exists gdb_wrapper_file] && \
+	    ![file exists $gdb_wrapper_file] } {
+	verbose "reinitializing the wrapper"
+	set gdb_wrapper_initialized 0
+    }
+
     if { $gdb_wrapper_initialized == 1 } { return; }
 
     if {[target_info exists needs_status_wrapper] && \
@@ -3812,7 +3821,7 @@ proc gdb_compile {source dest type options} {
     verbose "options are $options"
     verbose "source is $source $dest $type $options"
 
-    if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
+    gdb_wrapper_init
 
     if {[target_info exists needs_status_wrapper] && \
 	    [target_info needs_status_wrapper] != "0" && \
-- 
2.25.0

Re: [PATCH v2] gdb/testsuite: Regenerate the testglue if it is not in path

[Simon Marchi]

On 2020-02-18 6:51 a.m., Shahab Vahedi wrote:
> From: Shahab Vahedi <shahab@synopsys.com>
> 
> For running the  DejaGnu  tests,  some  esoteric  configurations
> may require a testglue.   This,  for  instance,  is  true  about
> testing ARC  targets  which  uses  its  own  DejaGnu  board  and
> a simulator which does not support returning the  pass  or  fail
> through the exit code.  Therefore,  for  those  tests  that  use

When you say "returning the pass or fail through the exit code", do
you mean the exit codes of the test programs (the value returned
by main)?  If so, it does not really mean "pass" or "fail", in a
testsuite sense.  In which case, it would be clearer to just say
"does not support returning the program's exit code".

I don't know what Tom was referring to when he talked about the cache
directory.  I see that the testsuite Makefile cleans a cache directory,
but I have no idea how things can end up there.  Tom, can you clarify?

It's great if you can manage to have it built only once, but just make
sure it's not racy, like:

1. Test 1 notices testglue.o is not there
2. Test 2 notices testglue.o is not there
3. Test 1 generates testglue.o
4. Test 2 starts to write testglue.o, deleting what was done in step 3,
   gets interrupted half-way
5. Test 1 tries to link the half-baked testglue.o in its executable and fails

An option to have it at a predictable absolute place would be to pass
the result of [standard_output_file testglue.o] to build_wrapper.
It would be rebuilt for each test, but it would make sure that tests
don't interfere with each other.

Simon

[PATCH v3] gdb/testsuite: Regenerate the testglue if it is not in

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

For running the  DejaGnu  tests,  some  esoteric  configurations
may require a testglue.   This,  for  instance,  is  true  about
testing ARC  targets  which  uses  its  own  DejaGnu  board  and
a simulator which does not support returning the program's  exit
code.  Therefore, for those  tests  that  use  "gdb_compile",  a
"gdb_tg.o"  file  is  compiled  and  linked   into   the   final
executable.

There  are  tests  that  invoke  "gdb_compile"  from   different
directories.   Let's  take  a   look   at   an   example   test:
gdb.base/fullname.exp.  The purpose of this  test  is  to  build
the executable from different directories (absolute vs. relative
vs.  other) and then check if gdb can handle setting breakpoints
accordingly.

When  "gdb_compile"  generates  the  "gdb_tg.o",  it  does   not
do it again  for  the  same  test.   Although  this  might  seem
efficient, it can lead to  problems  when  changing  directories
before the next compile:

  gdb compile failed, arc-elf32-gcc: error: gdb_tg.o:
  No such file or directory

This patch checks if the wrapper file ("gdb_tg.o") is  still  in
reach and if it is not, it will stimulate  the  regeneration  of
the wrapper.

It is worth mentioning that GCC's  DejaGnu  tests  handle  these
scenarios as well and they seem to be more efficient in doing so
by saving the library paths and manipulating them  if  necessary
[1].  However, for GDB tests, that  require  less  compilations,
I think the proposed solution should be fine compared to a  more
full fledged solution from GCC.  The glue file in  our  case  is
only 2 KiB.

Last but not least, I ran the x86_64 tests on an x86_64 host and
found no regression.

[1]
Avid  coders  may  look  for  "set_ld_library_path_env_vars"  in
gcc/testsuite/lib/target-libpath.exp.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_wrapper_init): Reset
	"gdb_wrapper_initialized" to 0 if "wrapper_file" does
	not exist.
---
 gdb/testsuite/lib/gdb.exp | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 4376e08ca1b..2826b1bfe4c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3575,11 +3575,20 @@ proc gdb_wrapper_init { args } {
     global gdb_wrapper_flags
     global gdb_wrapper_target
 
+    # If the wrapper is initialized but the wrapper file cannot be
+    # found anymore, the wrapper file must be built again.
+    if { $gdb_wrapper_initialized == 1 && \
+	    [info exists gdb_wrapper_file] && \
+	    ![file exists $gdb_wrapper_file] } {
+	verbose "reinitializing the wrapper"
+	set gdb_wrapper_initialized 0
+    }
+
     if { $gdb_wrapper_initialized == 1 } { return; }
 
     if {[target_info exists needs_status_wrapper] && \
 	    [target_info needs_status_wrapper] != "0"} {
-	set result [build_wrapper "testglue.o"]
+	set result [build_wrapper [standard_output_file "testglue.o"]]
 	if { $result != "" } {
 	    set gdb_wrapper_file [lindex $result 0]
 	    set gdb_wrapper_flags [lindex $result 1]
@@ -3822,7 +3831,7 @@ proc gdb_compile {source dest type options} {
     verbose "options are $options"
     verbose "source is $source $dest $type $options"
 
-    if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
+    gdb_wrapper_init
 
     if {[target_info exists needs_status_wrapper] && \
 	    [target_info needs_status_wrapper] != "0" && \
-- 
2.25.1

Re: [PATCH v2] gdb/testsuite: Regenerate the testglue if it is not in path

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I don't know what Tom was referring to when he talked about the cache
Simon> directory.  I see that the testsuite Makefile cleans a cache directory,
Simon> but I have no idea how things can end up there.  Tom, can you clarify?

If you write a gdb_caching_proc, it caches its result in the cache directory.
This can speed up parallel builds by avoiding excess work.

However in this case, I'm inclined to just let the patch in as-is, since
it's a specialty thing not affecting most test runs.

Tom

Re: [PATCH v3] gdb/testsuite: Regenerate the testglue if it is not in

[Tom Tromey]

>>>>> "Shahab" == Shahab Vahedi <shahab.vahedi@gmail.com> writes:

Shahab> gdb/testsuite/ChangeLog:

Shahab> 	* lib/gdb.exp (gdb_wrapper_init): Reset
Shahab> 	"gdb_wrapper_initialized" to 0 if "wrapper_file" does
Shahab> 	not exist.

Thank you.  I changed my mind -- I think whether or not to cache this is
up to you, as most builds don't need this bit.

So, this is ok, please check it in.

Tom