[PATCH] testsuite: Treat empty string in needs_status_wrapper as a false value

[PATCH] testsuite: Treat empty string in needs_status_wrapper as a false value

[Anton Kolesov]

GCC testsuite treats empty string in [target_info needs_status_wrapper] the
same way as "0" - as a false value. GDB testsuite on the other hand recognizes
only zero as false. This patch changes this to improve compatibilty of board files.

gdb/testsuite/ChangeLog:

2013-10-17  Anton Kolesov <Anton.Kolesov@synopsys.com>

    * lib/gdb.exp (gdb_compile, gdb_wrapper_init): Treat empty string in
    target_info needs_status_wrapper the same way as zero - as a false value.
---
 gdb/testsuite/lib/gdb.exp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3efd539..f5f1555 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2511,6 +2511,7 @@ proc gdb_wrapper_init { args } {
     if { $gdb_wrapper_initialized == 1 } { return; }
 
     if {[target_info exists needs_status_wrapper] && \
+	    [target_info needs_status_wrapper] != "" && \
 	    [target_info needs_status_wrapper] != "0"} {
 	set result [build_wrapper "testglue.o"]
 	if { $result != "" } {
@@ -2610,6 +2611,7 @@ proc gdb_compile {source dest type options} {
     if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
 
     if {[target_info exists needs_status_wrapper] && \
+	    [target_info needs_status_wrapper] != "" && \
 	    [target_info needs_status_wrapper] != "0" && \
 	    [info exists gdb_wrapper_file]} {
 	lappend options "libs=${gdb_wrapper_file}"
-- 
1.8.4.1

RE: [PATCH] testsuite: Treat empty string in needs_status_wrapper as a false value

[Anton Kolesov]

Hi all,

It seems that I should have removed call to [target_info exists] in this patch as well, as it is redundant, since if variable is not set, then an empty string will be returned. However if needs_status_wrapper was explicitly set to an empty string, then [target_info exists] will have a different result than comparison with an empty string. Dejagnu itself in remote.exp compares needs_status_wrapper with an empty string, so I think that the same should be done in GDB test suite. If there are no other objections I will send an updated patch.

--
Anton Kolesov


> -----Original Message-----
> From: Anton Kolesov [mailto:Anton.Kolesov@synopsys.com]
> Sent: 17 October 2013 16:59
> To: gdb-patches@sourceware.org
> Cc: Anton Kolesov; Jeremy.bennett@embecosm.com
> Subject: [PATCH] testsuite: Treat empty string in needs_status_wrapper as
> a false value
> 
> GCC testsuite treats empty string in [target_info needs_status_wrapper]
> the
> same way as "0" - as a false value. GDB testsuite on the other hand
> recognizes
> only zero as false. This patch changes this to improve compatibilty of
> board files.
> 
> gdb/testsuite/ChangeLog:
> 
> 2013-10-17  Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
>     * lib/gdb.exp (gdb_compile, gdb_wrapper_init): Treat empty string in
>     target_info needs_status_wrapper the same way as zero - as a false
> value.
> ---
>  gdb/testsuite/lib/gdb.exp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 3efd539..f5f1555 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2511,6 +2511,7 @@ proc gdb_wrapper_init { args } {
>      if { $gdb_wrapper_initialized == 1 } { return; }
> 
>      if {[target_info exists needs_status_wrapper] && \
> +	    [target_info needs_status_wrapper] != "" && \
>  	    [target_info needs_status_wrapper] != "0"} {
>  	set result [build_wrapper "testglue.o"]
>  	if { $result != "" } {
> @@ -2610,6 +2611,7 @@ proc gdb_compile {source dest type options} {
>      if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
> 
>      if {[target_info exists needs_status_wrapper] && \
> +	    [target_info needs_status_wrapper] != "" && \
>  	    [target_info needs_status_wrapper] != "0" && \
>  	    [info exists gdb_wrapper_file]} {
>  	lappend options "libs=${gdb_wrapper_file}"
> --
> 1.8.4.1

[PATCH v2] testsuite: Treat an empty string in needs_status_wrapper as false

[Anton Kolesov]

GDB test suite considers [target_info needs_status_wrapper] to be false if
it unset or have a zero value. The former is achieved by using [target_info
exists needs_status_wrapper]. GCC test suite on the other hand do not use
"exists" but compares to an empty string. This doesn't make difference if
value is unset, as unset value is treated as an empty string, but makes a
difference if value was set to and empty string. In that case if
needs_status_wrapper was set to an empty string, then GCC test suite will
not use status wrapper, but GDB test suite will use it. Dejagnu's own
remote.exp uses a comparison with an empty string. Though for some reason
Dejagnu unlike GCC and GDB test suite doesn't treat a zero as a false.

This patch makes GDB test suite treat an empty string in
needs_status_wrapper the same way as it is done by GCC test suite and
Dejagnu.

gdb/testsuite/ChangeLog:

2013-10-21  Anton Kolesov <Anton.Kolesov@synopsys.com>

    * lib/gdb.exp (gdb_compile, gdb_wrapper_init): Treat empty string in
    target_info needs_status_wrapper the same way as zero - as a false value.
---
 gdb/testsuite/lib/gdb.exp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3efd539..1e5c34a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2510,7 +2510,7 @@ proc gdb_wrapper_init { args } {
 
     if { $gdb_wrapper_initialized == 1 } { return; }
 
-    if {[target_info exists needs_status_wrapper] && \
+    if {[target_info needs_status_wrapper] != "" && \
 	    [target_info needs_status_wrapper] != "0"} {
 	set result [build_wrapper "testglue.o"]
 	if { $result != "" } {
@@ -2609,7 +2609,7 @@ proc gdb_compile {source dest type options} {
 
     if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
 
-    if {[target_info exists needs_status_wrapper] && \
+    if {[target_info needs_status_wrapper] != "" && \
 	    [target_info needs_status_wrapper] != "0" && \
 	    [info exists gdb_wrapper_file]} {
 	lappend options "libs=${gdb_wrapper_file}"
-- 
1.8.4.1

Re: [PATCH v2] testsuite: Treat an empty string in needs_status_wrapper as false

[Pedro Alves]

On 10/21/2013 12:17 PM, Anton Kolesov wrote:
> GDB test suite considers [target_info needs_status_wrapper] to be false if
> it unset or have a zero value. The former is achieved by using [target_info
> exists needs_status_wrapper]. GCC test suite on the other hand do not use
> "exists" but compares to an empty string. This doesn't make difference if
> value is unset, as unset value is treated as an empty string, but makes a
> difference if value was set to and empty string. In that case if
> needs_status_wrapper was set to an empty string, then GCC test suite will
> not use status wrapper, but GDB test suite will use it. Dejagnu's own
> remote.exp uses a comparison with an empty string. Though for some reason
> Dejagnu unlike GCC and GDB test suite doesn't treat a zero as a false.

There's another reference to needs_status_wrapper in java.exp.

I think it'd be good to add a new procedure all these 3 places
call instead of inlining all these not-set/""/"0" checks.  It'd
also be great if the info quoted above was put in the code
itself (in the new procedure's documentation).

-- 
Pedro Alves

[PATCH v3] testsuite: Treat an empty string in needs_status_wrapper as false

[Anton Kolesov]

Updated according to feedback.

GDB test suite considers [target_info needs_status_wrapper] to be false if
it is unset or have a zero value. The former is achieved by using [target_info
exists needs_status_wrapper]. GCC test suite on the other hand do not use
"exists" but compares to an empty string. This doesn't make difference if
value is unset, as unset value is treated as an empty string, but makes a
difference if value was set to and empty string. In that case if
needs_status_wrapper was set to an empty string, then GCC test suite will
not use status wrapper, but GDB test suite will use it. Dejagnu's own
remote.exp uses a comparison with an empty string. Though for some reason
Dejagnu unlike GCC and GDB test suite doesn't treat a zero as a false.

This patch makes GDB test suite treat an empty string in
needs_status_wrapper the same way as it is done by GCC test suite and
Dejagnu. It also extracts those checks into separate function
'gdb_needs_status_wrapper'.

gdb/testsuite/ChangeLog:

2013-10-22  Anton Kolesov <Anton.Kolesov@synopsys.com>

	* lib/gdb.exp (gdb_needs_status_wrapper): New function.
	* lib/gdb.exp (gdb_compile, gdb_wrapper_init): Replace inline checks for
	needs_status_wrapper value with call to a new function.
	* lib/java.exp (java_init): Ditto.
---
 gdb/testsuite/lib/gdb.exp  | 25 +++++++++++++++++++++----
 gdb/testsuite/lib/java.exp |  2 +-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3efd539..7001c8d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2510,8 +2510,7 @@ proc gdb_wrapper_init { args } {
 
     if { $gdb_wrapper_initialized == 1 } { return; }
 
-    if {[target_info exists needs_status_wrapper] && \
-	    [target_info needs_status_wrapper] != "0"} {
+    if {[gdb_needs_status_wrapper] == 1} {
 	set result [build_wrapper "testglue.o"]
 	if { $result != "" } {
 	    set gdb_wrapper_file [lindex $result 0]
@@ -2609,8 +2608,7 @@ proc gdb_compile {source dest type options} {
 
     if { $gdb_wrapper_initialized == 0 } { gdb_wrapper_init }
 
-    if {[target_info exists needs_status_wrapper] && \
-	    [target_info needs_status_wrapper] != "0" && \
+    if {[gdb_needs_status_wrapper] == 1 && \
 	    [info exists gdb_wrapper_file]} {
 	lappend options "libs=${gdb_wrapper_file}"
 	lappend options "ldflags=${gdb_wrapper_flags}"
@@ -4492,5 +4490,24 @@ proc using_fission { } {
     return [regexp -- "-gsplit-dwarf" $debug_flags]
 }
 
+# There seems to be a lack of clear definition of what value of target_info
+# needs_status_wrapper should be considered as False and what as True. Dejagnu
+# in remote.exp makes a check comparing with an empty string. Which means that
+# False is an empty string or unset value. GCC test suite does the same, but it
+# also treats zero as a False. GDB previously was using zero and the unset
+# value as False, but unlike others it was treating an explicitly set empty
+# string as True. This function applies the same logic as GCC test suite for a
+# compatibility reasons and should be used instead of any direct checks for a
+# needs_status_wrapper value.
+# Return 1 if status wrapper is required, return 0 otherwise.
+proc gdb_needs_status_wrapper { } {
+	if {[target_info needs_status_wrapper] != "" && \
+	    [target_info needs_status_wrapper] != "0" } {
+		return 1
+	} else {
+		return 0
+	}
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdb/testsuite/lib/java.exp b/gdb/testsuite/lib/java.exp
index 19b1eee..8e6ff23 100644
--- a/gdb/testsuite/lib/java.exp
+++ b/gdb/testsuite/lib/java.exp
@@ -70,7 +70,7 @@ proc java_init { args } {
 
     set wrapper_file ""
     set wrap_compile_flags ""
-    if [target_info exists needs_status_wrapper] {
+    if {[gdb_needs_status_wrapper] == 1} {
 	set result [build_wrapper "testglue.o"]
 	if { $result != "" } {
 	    set wrapper_file [lindex $result 0]
-- 
1.8.4.1

Re: [PATCH v3] testsuite: Treat an empty string in needs_status_wrapper as false

[Pedro Alves]

On 10/22/2013 11:51 AM, Anton Kolesov wrote:
> Updated according to feedback.
> 
> GDB test suite considers [target_info needs_status_wrapper] to be false if
> it is unset or have a zero value. The former is achieved by using [target_info
> exists needs_status_wrapper]. GCC test suite on the other hand do not use
> "exists" but compares to an empty string. This doesn't make difference if
> value is unset, as unset value is treated as an empty string, but makes a
> difference if value was set to and empty string. In that case if
> needs_status_wrapper was set to an empty string, then GCC test suite will
> not use status wrapper, but GDB test suite will use it. Dejagnu's own
> remote.exp uses a comparison with an empty string. Though for some reason
> Dejagnu unlike GCC and GDB test suite doesn't treat a zero as a false.
> 
> This patch makes GDB test suite treat an empty string in
> needs_status_wrapper the same way as it is done by GCC test suite and
> Dejagnu. It also extracts those checks into separate function
> 'gdb_needs_status_wrapper'.

Yeah, this is all odd.

Looking at gcc's testsuite, even though lots of inconsistency
had been centralized in wrapper.exp years ago:

 http://lists.gnu.org/archive/html/dejagnu/2004-09/msg00001.html

they still remain largely inconsistent:

$ grep -rn needs_status_wrapper *
g++.old-deja/g++.other/init18.C:1:// Some targets (e.g. those with "set_board_info needs_status_wrapper 1"
g++.old-deja/g++.pt/static11.C:1:// Some targets (e.g. those with "set_board_info needs_status_wrapper 1"
lib/gfortran.exp:238:    if { [target_info needs_status_wrapper] != "" && [info exists gluefile] } {
lib/gnat.exp:152:    if { [target_info needs_status_wrapper]!="" && [info exists gluefile] } {
lib/wrapper.exp:27:    if { [target_info needs_status_wrapper] != "" \
lib/wrapper.exp:28:      && [target_info needs_status_wrapper] != "0" \
lib/go.exp:215:    if { [target_info needs_status_wrapper] != "" && [info exists gluefile] } {
lib/g++.exp:292:    if { [target_info needs_status_wrapper] != "" && [info exists gluefile] } {
lib/obj-c++.exp:343:    if { [target_info needs_status_wrapper] != "" && [info exists gluefile] } {
lib/gcc.exp:130:    if {[target_info needs_status_wrapper] != "" && \
lib/gcc.exp:131:            [target_info needs_status_wrapper] != "0" && \
lib/objc.exp:188:    if { [target_info needs_status_wrapper]!="" && [info exists gluefile] } {
lib/target-supports.exp:990:    if { [target_info needs_status_wrapper] != "" \
lib/target-supports.exp:991:         && [target_info needs_status_wrapper] != "0" } {

The check in DejaGNU looks like:

    # If all programs of this board have a wrapper that always outputs a
    # status message, then the absence of it means that the program
    # crashed, regardless of status found elsewhere (e.g. simulator exit
    # code).
    if { [target_info needs_status_wrapper] != "" } then {
	set nomatch_return 2
    } else {
	set nomatch_return -1
    }

Given all that, I don't think anyone will be setting target_info
needs_status_wrapper to "0" to say "false".  It just doesn't seem
it'd work properly anywhere.  I'd guess that "0" check was a little
mistake that ended blindly copy/pasted everywhere.

I also tried:

$ grep needs_status_wrapper * -rn in dejagnu's sources, and that hits
a bunch of boards like these:

 baseboards/m32r-linux-sim.exp:47:set_board_info needs_status_wrapper  1
 baseboards/mips-idt.exp:43:set_board_info needs_status_wrapper 1
 baseboards/sparclite-sim-le.exp:49:set_board_info needs_status_wrapper 1
 baseboards/v850-sim.exp:45:set_board_info needs_status_wrapper 1
 baseboards/mips-sim-idt64.exp:36:#set_board_info needs_status_wrapper 1
 baseboards/vr4100-ddb.exp:43:set_board_info needs_status_wrapper 1
 baseboards/sparclet-aout.exp:42:#set_board_info needs_status_wrapper 1

But there's only one that's odd... It looks like:

 baseboards/mn10300-sim.exp:49:set_board_info needs_status_wrapper ""

Sigh.  Someone used the empty string to enable the status wrapper...

Looking above that, to see if there's any comment that explains why
that one is different, we find:

 baseboards/mn10300-sim.exp:48:# The simulator doesn't return exit statuses and we need to indicate this.
 baseboards/mn10300-sim.exp-49-set_board_info needs_status_wrapper ""

Which is just the same comment that appears in all the
other boards.  So it just looks like nobody tried that
board with gcc?  I think we can just ignore that.

In any case, no board set needs_status_wrapper to 0.  I wonder
if we shouldn't stop looking for "0" explicitly too, like DejaGNU,
and clean up gcc's checks likewise...

But how did you notice this?  What value were you setting in
your board?

> gdb/testsuite/ChangeLog:
> 
> 2013-10-22  Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> 	* lib/gdb.exp (gdb_needs_status_wrapper): New function.
> 	* lib/gdb.exp (gdb_compile, gdb_wrapper_init): Replace inline checks for
> 	needs_status_wrapper value with call to a new function.
> 	* lib/java.exp (java_init): Ditto.
> ---
>  gdb/testsuite/lib/gdb.exp  | 25 +++++++++++++++++++++----
>  gdb/testsuite/lib/java.exp |  2 +-
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 3efd539..7001c8d 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2510,8 +2510,7 @@ proc gdb_wrapper_init { args } {
>  
>      if { $gdb_wrapper_initialized == 1 } { return; }
>  
> -    if {[target_info exists needs_status_wrapper] && \
> -	    [target_info needs_status_wrapper] != "0"} {
> +    if {[gdb_needs_status_wrapper] == 1} {

In TCL, like in C, "1" and "0" are the same as true and false.  This
can just be:

    if {[gdb_needs_status_wrapper]} {

> +# There seems to be a lack of clear definition of what value of target_info
> +# needs_status_wrapper should be considered as False and what as True. Dejagnu

Make it a double-space after each full stop.  Here and everywhere.

Write "DejaGNU".

> +# in remote.exp makes a check comparing with an empty string. Which means that

I think that should be comma instead of period.

> +# False is an empty string or unset value. GCC test suite does the same, but it
> +# also treats zero as a False. GDB previously was using zero and the unset
> +# value as False, but unlike others it was treating an explicitly set empty
> +# string as True. This function applies the same logic as GCC test suite for a
> +# compatibility reasons and should be used instead of any direct checks for a
> +# needs_status_wrapper value.
> +# Return 1 if status wrapper is required, return 0 otherwise.

Here you can say "Return true" ... "return false".

> +# There seems to be a lack of clear definition of the return value of target_info
> +# needs_status_wrapper.  DejaGNU in remote.exp makes a check comparing with an empty string. Which means that
> +# False is an empty string or unset value. GCC test suite does the same, but it
> +# also treats zero as a False. GDB previously was using zero and the unset
> +# value as False, but unlike others it was treating an explicitly set empty
> +# string as True. This function applies the same logic as GCC test suite for a
> +# compatibility reasons and should be used instead of any direct checks for a
> +# needs_status_wrapper value.
> +# Return 1 if status wrapper is required, return 0 otherwise.

Overly long lines.

Hmm, I think this whole comment can be simplified (and thus
clarified) without losing any info.  I think the emphasis first
should be on what the function does, and the return value.

The rationale can come afterwards, for whoever wants to dig
deeper.  See below.

> +proc gdb_needs_status_wrapper { } {
> +	if {[target_info needs_status_wrapper] != "" && \

&& should be in the other line.  Is that \ needed?

> +	    [target_info needs_status_wrapper] != "0" } {
> +		return 1
> +	} else {
> +		return 0
> +	}
> +}
> +

Here's what I suggest:

# Return true if a status wrapper is required, return false otherwise.
# Use this function instead of directly checking the
# needs_status_wrapper value.
#
proc gdb_needs_status_wrapper { } {
    # There seems to be no clear definition of the return value of
    # [target_info needs_status_wrapper].  DejaGNU's
    # remote.exp:check_for_board_status procedure treats unset or
    # empty string as false.  GCC's test suite also treats "0" as
    # false.  GDB used to treat both "0" and the unset value as false,
    # but treat an explicitly set empty string as true.  We now apply
    # the same logic as GCC's test suite in the name of toolchain
    # consistency.
    if {[target_info needs_status_wrapper] != ""
	&& [target_info needs_status_wrapper] != "0" } {
	return 1
    } else {
	return 0
    }
}

But then again, if setting to "0" indeed breaks the DejaGNU
side (in that it treats that as true), I question the value
of propagating that brokenness...

-- 
Pedro Alves

RE: [PATCH v3] testsuite: Treat an empty string in needs_status_wrapper as false

[Anton Kolesov]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> 
> Yeah, this is all odd.
> 
> Looking at gcc's testsuite, even though lots of inconsistency
> had been centralized in wrapper.exp years ago:
> 
>  http://lists.gnu.org/archive/html/dejagnu/2004-09/msg00001.html
> 
> they still remain largely inconsistent:
 [AK:]  Yes I've noticed inconsistency there, GCC test suite should be fixed as well, I think.

> 
> Given all that, I don't think anyone will be setting target_info
> needs_status_wrapper to "0" to say "false".  It just doesn't seem
> it'd work properly anywhere.  I'd guess that "0" check was a little
> mistake that ended blindly copy/pasted everywhere.
[AK:] GCC developer in our project sets needs_status_wrapper to 0. So this happens, even if it is done by mistake. 

> 
> But there's only one that's odd... It looks like:
> 
>  baseboards/mn10300-sim.exp:49:set_board_info needs_status_wrapper ""
> 
> Sigh.  Someone used the empty string to enable the status wrapper...
[AK:] Or someone tried to explicitly disable it.

> 
> Looking above that, to see if there's any comment that explains why
> that one is different, we find:
> 
>  baseboards/mn10300-sim.exp:48:# The simulator doesn't return exit
> statuses and we need to indicate this.
>  baseboards/mn10300-sim.exp-49-set_board_info needs_status_wrapper ""
> 
> Which is just the same comment that appears in all the
> other boards.  So it just looks like nobody tried that
> board with gcc?  I think we can just ignore that.
> 
> In any case, no board set needs_status_wrapper to 0.  I wonder
> if we shouldn't stop looking for "0" explicitly too, like DejaGNU,
> and clean up gcc's checks likewise...
[AK:] In my opinion, there should be legitimate "false" value not just unset value. It shouldn't be just 0, but anything the Tcl treats as false (0, false, no, off). Regrettably, unlike, for example JavaScript, empty string is not a "false" value in Tcl. 

> 
> But how did you notice this?  What value were you setting in
> your board?
[AK:] I've set it to an empty string to disable status wrapper, as it looked like a legitimate way of doing this after studying GCC test suite. It is a long story of how I've reached this, but now I see that this isn't how it is supposed to be done.

> 
> 
> In TCL, like in C, "1" and "0" are the same as true and false.  This
> can just be:
> 
>     if {[gdb_needs_status_wrapper]} {
> 
[AK:]  I was in doubt about how to write this, because there are enough cases of "[]== 1" in test suite, so I've stick to them, as it looks like a more clear way, but I don't have own preferences here, so I will update this for next version.

> 
> But then again, if setting to "0" indeed breaks the DejaGNU
> side (in that it treats that as true), I question the value
> of propagating that brokenness...
[AK:] I would advocate the need to update dejagnu, as it seems that both GCC and GDB consider zero as a false value. As I've said to me it looks better to have some legitimate false value to this property, not only true or unset.

> 
> --
> Pedro Alves