This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]