[patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

This patch fixes another batch of testsuite bugs I found, this time 
related to handling of the gdb testglue wrapper that is needed on some 
targets where the simulator (or whatever) does not properly report 
program exit status to the test harness.  More details in the commit 
message.

I tested this on nios2-elf.  OK to commit?

-Sandra


[-- Attachment #2: gdb-wrapper.patch --]
[-- Type: text/x-patch, Size: 5059 bytes --]

commit 266cd05dfb396288fbde156e5ac4ccdc23bf3b62
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Mon Jul 6 17:09:18 2020 -0700

    Fix more bugs in gdb testglue wrapper handling
    
    In commit 24ac169ac5a918cd82b7485935f0c40a094c625e, this patch:
    
    	2020-02-21  Shahab Vahedi  <shahab@synopsys.com>
    
    	* lib/gdb.exp (gdb_wrapper_init): Reset
    	"gdb_wrapper_initialized" to 0 if "wrapper_file" does
    	not exist.
    
    attempted to fix problems finding the gdb test wrapper gdb_tg.o in
    some tests that cd to some non-default directory by rebuilding also
    the test wrapper in that directory.  This had the side-effect of
    leaving these .o files in various places in the GDB source directory
    tree.
    
    Furthermore, while the tests that cd to some non-default directory
    cannot run on remote host, the code that was added to probe for the
    presence of the wrapper file was also specific to host == build.
    
    This patch reverts the problematic parts of that commit and replaces
    it with forcing use of an absolute (rather than relative) pathname to
    the .o file for linking when host == build.
    
    While debugging this patch, I also observed that use of the construct
    "[info exists gdb_wrapper_file]" was not reliable for detecting when
    that variable had been initialized by gdb_wrapper_init.  I rewrote
    that so that the variable is always initialized and has a value of an
    empty string when no wrapper file is needed.
    
    2020-07-06  Sandra Loosemore  <sandra@codesourcery.com>
    
    	gdb/testsuite/
    	* lib/gdb.exp (gdb_wrapper_file, gdb_wrapper_flags):
    	Initialize to empty string at top level.
    	(gdb_wrapper_init): Revert check for file existence on build.
    	Build the wrapper in its default place, not a build-specific
    	location.  When host == build, make the pathname absolute.
    	(gdb_compile): Delete leftover declaration of
    	gdb_wrapper_initialized.  Check gdb_wrapper_file being an empty
    	string instead of uninitialized.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8eae1ab..efab440 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,14 @@
+2020-07-06  Sandra Loosemore  <sandra@codesourcery.com>
+
+	* lib/gdb.exp (gdb_wrapper_file, gdb_wrapper_flags):
+	Initialize to empty string at top level.
+	(gdb_wrapper_init): Revert check for file existence on build.
+	Build the wrapper in its default place, not a build-specific
+	location.  When host == build, make the pathname absolute.
+	(gdb_compile): Delete leftover declaration of
+	gdb_wrapper_initialized.  Check gdb_wrapper_file being an empty
+	string instead of uninitialized.
+
 2020-07-06  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR python/22748
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index b0faf62..9f7881e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3694,6 +3694,8 @@ proc current_target_name { } {
 
 set gdb_wrapper_initialized 0
 set gdb_wrapper_target ""
+set gdb_wrapper_file ""
+set gdb_wrapper_flags ""
 
 proc gdb_wrapper_init { args } {
     global gdb_wrapper_initialized
@@ -3701,27 +3703,25 @@ 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 [standard_output_file "testglue.o"]]
+	set result [build_wrapper "testglue.o"]
 	if { $result != "" } {
 	    set gdb_wrapper_file [lindex $result 0]
+	    if ![is_remote host] {
+		set gdb_wrapper_file [file join [pwd] $gdb_wrapper_file]
+	    }
 	    set gdb_wrapper_flags [lindex $result 1]
 	} else {
 	    warning "Status wrapper failed to build."
 	}
+    } else {
+	set gdb_wrapper_file ""
+	set gdb_wrapper_flags ""
     }
+    verbose "set gdb_wrapper_file = $gdb_wrapper_file"
     set gdb_wrapper_initialized 1
     set gdb_wrapper_target [current_target_name]
 }
@@ -3857,7 +3857,6 @@ proc gdb_compile {source dest type options} {
     global GDB_TESTCASE_OPTIONS
     global gdb_wrapper_file
     global gdb_wrapper_flags
-    global gdb_wrapper_initialized
     global srcdir
     global objdir
     global gdb_saved_set_unbuffered_mode_obj
@@ -3994,7 +3993,7 @@ proc gdb_compile {source dest type options} {
 
     if {[target_info exists needs_status_wrapper] && \
 	    [target_info needs_status_wrapper] != "0" && \
-	    [info exists gdb_wrapper_file]} {
+	    $gdb_wrapper_file != "" } {
 	lappend options "libs=${gdb_wrapper_file}"
 	lappend options "ldflags=${gdb_wrapper_flags}"
     }

[ping] Re: [patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[Sandra Loosemore]

Ping?

https://sourceware.org/pipermail/gdb-patches/2020-July/170224.html

On 7/6/20 6:33 PM, Sandra Loosemore wrote:
> This patch fixes another batch of testsuite bugs I found, this time 
> related to handling of the gdb testglue wrapper that is needed on some 
> targets where the simulator (or whatever) does not properly report 
> program exit status to the test harness.  More details in the commit 
> message.
> 
> I tested this on nios2-elf.  OK to commit?
> 
> -Sandra
> 

Re: [patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[Tom Tromey]

>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:

Sandra>      if {[target_info exists needs_status_wrapper] && \
Sandra>  	    [target_info needs_status_wrapper] != "0"} {
Sandra> -	set result [build_wrapper [standard_output_file "testglue.o"]]
Sandra> +	set result [build_wrapper "testglue.o"]
Sandra>  	if { $result != "" } {
Sandra>  	    set gdb_wrapper_file [lindex $result 0]
Sandra> +	    if ![is_remote host] {
Sandra> +		set gdb_wrapper_file [file join [pwd] $gdb_wrapper_file]

If "make -j check" is used, isn't it possible that multiple runs will
use the same file name and then clash?

Tom

Re: [patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[Sandra Loosemore]

On 7/21/20 12:13 PM, Tom Tromey wrote:
>>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:
> 
> Sandra>      if {[target_info exists needs_status_wrapper] && \
> Sandra>  	    [target_info needs_status_wrapper] != "0"} {
> Sandra> -	set result [build_wrapper [standard_output_file "testglue.o"]]
> Sandra> +	set result [build_wrapper "testglue.o"]
> Sandra>  	if { $result != "" } {
> Sandra>  	    set gdb_wrapper_file [lindex $result 0]
> Sandra> +	    if ![is_remote host] {
> Sandra> +		set gdb_wrapper_file [file join [pwd] $gdb_wrapper_file]
> 
> If "make -j check" is used, isn't it possible that multiple runs will
> use the same file name and then clash?

Well, the first part of this patch hunk is undoing an incorrect change 
from commit 24ac169ac5a918cd82b7485935f0c40a094c625e -- incorrect 
because standard_output_file returns a pathname relative to build, which 
cannot generally work on remote host.  Things were apparently working 
fine for years up until February.  :-S

The second part is just the usual idiom to convert a relative pathname 
to absolute in TCL, so that we can refer to it correctly when [pwd] is 
something else.  That was actually the problem the previous commit was 
trying to fix (incorrectly, by trying to rebuild the wrapper in [pwd], 
and leaving the .o files polluting the source directory in some cases).

-Sandra

Re: [patch] gdb/testsuite: Fix more bugs in gdb testglue wrapper handling

[Tom Tromey]

>>>>> "Sandra" == Sandra Loosemore <sandra@codesourcery.com> writes:

Sandra> Well, the first part of this patch hunk is undoing an incorrect change
Sandra> from commit 24ac169ac5a918cd82b7485935f0c40a094c625e -- incorrect 
Sandra> because standard_output_file returns a pathname relative to build,
Sandra> which cannot generally work on remote host.  Things were apparently
Sandra> working fine for years up until February.  :-S

Maybe nobody ever tried parallel tests in this scenario?

On the one hand, I'd hate to reintroduce a latent bug.
On the other hand, few people seem to use this code and I'd rather not
stand in the way of progress for some scenario that's not relevant.

So, the patch is OK with me.  Thanks.

Tom