[PATCH] gdb/testsuite: allow "require" callbacks to provide a reason

[PATCH] gdb/testsuite: allow "require" callbacks to provide a reason

[Simon Marchi]

When an allow_* proc returns false, it can be a bit difficult what check
failed exactly, if the procedure does multiple checks.  To make
investigation easier, I propose to allow the "require" callbacks to be
able to return a list of two elements: the zero/non-zero value, and a
reason string.

Use the new feature in allow_hipcc_tests to demonstrate it (it's also
where I hit actually hit this inconvenience).  On my computer (where GDB
is built with amd-dbgapi support but where I don't have a suitable GPU
target), I get:

    UNSUPPORTED: gdb.rocm/simple.exp: require failed: allow_hipcc_tests (no suitable amdgpu targets found)

vs before:

    UNSUPPORTED: gdb.rocm/simple.exp: require failed: allow_hipcc_tests

Change-Id: Id1966535b87acfcbe9eac99f49dc1196398c6578
---
 gdb/testsuite/lib/gdb.exp  | 37 +++++++++++++++++++++++++++++--------
 gdb/testsuite/lib/rocm.exp |  8 ++++----
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 46aa1441d6d0..7fb4f1cbdab4 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9251,22 +9251,43 @@ gdb_caching_proc have_avx {} {
 #
 # ARG can either be a name, or of the form !NAME.
 #
-# Each name is a proc to evaluate in the caller's context.  It returns
-# a boolean, and a "!" means to invert the result.  If this is
-# nonzero, all is well.  If it is zero, an "untested" is emitted and
-# this proc causes the caller to return.
+# Each name is a proc to evaluate in the caller's context.  It can return a
+# boolean or a two element list with a boolean and a reason string.
+# A "!" means to invert the result.  If this is true, all is well.  If it is
+# false, an "unsupported" is emitted and this proc causes the caller to return.
+#
+# The reason string is used to provide some context about a require failure,
+# and is included in the "unsupported" message.
 
 proc require { args } {
     foreach arg $args {
 	if {[string index $arg 0] == "!"} {
-	    set ok 0
+	    set required_val 0
 	    set fn [string range $arg 1 end]
 	} else {
-	    set ok 1
+	    set required_val 1
 	    set fn $arg
 	}
-	if {$ok != !![uplevel 1 $fn]} {
-	    unsupported "require failed: $arg"
+
+	set result [uplevel 1 $fn]
+	set len [llength $result]
+	if { $len == 2 } {
+	    set actual_val [lindex $result 0]
+	    set msg [lindex $result 1]
+	} elseif { $len == 1 } {
+	    set actual_val $result
+	    set msg ""
+	} else {
+	    error "proc $fn returned a list of unexpected length $len"
+	}
+
+	if {$required_val != !!$actual_val} {
+	    if { [string length $msg] > 0 } {
+		unsupported "require failed: $arg ($msg)"
+	    } else {
+		unsupported "require failed: $arg"
+	    }
+
 	    return -code return 0
 	}
     }
diff --git a/gdb/testsuite/lib/rocm.exp b/gdb/testsuite/lib/rocm.exp
index b3e435339dbc..389d73bcaa5f 100644
--- a/gdb/testsuite/lib/rocm.exp
+++ b/gdb/testsuite/lib/rocm.exp
@@ -64,19 +64,19 @@ gdb_caching_proc allow_hipcc_tests {} {
     # testing against GDBserver, there's no point in running the ROCm
     # tests.
     if {[target_info gdb_protocol] != ""} {
-	return 0
+	return {0 "remote debugging"}
     }
 
     # Ensure that GDB is built with amd-dbgapi support.
     set output [remote_exec host $::GDB "$::INTERNAL_GDBFLAGS --configuration"]
     if { [string first "--with-amd-dbgapi" $output] == -1 } {
-	return 0
+	return {0 "amd-dbgapi not supported"}
     }
 
     # Check we have a working hipcc compiler available.
     set targets [hcc_amdgpu_targets]
     if { [llength $targets] == 0} {
-	return 0
+	return {0 "no suitable amdgpu targets found"}
     }
 
     set flags [list hip additional_flags=--offload-arch=[join $targets ","]]
@@ -93,7 +93,7 @@ gdb_caching_proc allow_hipcc_tests {} {
 		return 0;
 	    }
 	} executable $flags]} {
-	return 0
+	return {0 "failed to compile hip program"}
     }
 
     return 1
-- 
2.40.0

Re: [PATCH] gdb/testsuite: allow "require" callbacks to provide a reason

[Tom de Vries]

On 3/28/23 14:23, Simon Marchi via Gdb-patches wrote:
> When an allow_* proc returns false, it can be a bit difficult what check
> failed exactly, if the procedure does multiple checks.  To make
> investigation easier, I propose to allow the "require" callbacks to be
> able to return a list of two elements: the zero/non-zero value, and a
> reason string.
> 

Hi,

I like the idea.

The only question I had was what the desired behaviour is for:
...
   return {1 "bla"}
...

AFAICT, the current implementation just ignores "bla", and I wondered if 
perhaps we should error out to avoid the impression that something will 
be done with "bla".

That however will make this fail if $res == 1:
...
set res [try_foo]
return {$res "foo"}
...
so probably that's just too restrictive.

LGTM as-is.

Thanks,
- Tom

> Use the new feature in allow_hipcc_tests to demonstrate it (it's also
> where I hit actually hit this inconvenience).  On my computer (where GDB
> is built with amd-dbgapi support but where I don't have a suitable GPU
> target), I get:
> 
>      UNSUPPORTED: gdb.rocm/simple.exp: require failed: allow_hipcc_tests (no suitable amdgpu targets found)
> 
> vs before:
> 
>      UNSUPPORTED: gdb.rocm/simple.exp: require failed: allow_hipcc_tests
> 
> Change-Id: Id1966535b87acfcbe9eac99f49dc1196398c6578
> ---
>   gdb/testsuite/lib/gdb.exp  | 37 +++++++++++++++++++++++++++++--------
>   gdb/testsuite/lib/rocm.exp |  8 ++++----
>   2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 46aa1441d6d0..7fb4f1cbdab4 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9251,22 +9251,43 @@ gdb_caching_proc have_avx {} {
>   #
>   # ARG can either be a name, or of the form !NAME.
>   #
> -# Each name is a proc to evaluate in the caller's context.  It returns
> -# a boolean, and a "!" means to invert the result.  If this is
> -# nonzero, all is well.  If it is zero, an "untested" is emitted and
> -# this proc causes the caller to return.
> +# Each name is a proc to evaluate in the caller's context.  It can return a
> +# boolean or a two element list with a boolean and a reason string.
> +# A "!" means to invert the result.  If this is true, all is well.  If it is
> +# false, an "unsupported" is emitted and this proc causes the caller to return.
> +#
> +# The reason string is used to provide some context about a require failure,
> +# and is included in the "unsupported" message.
>   
>   proc require { args } {
>       foreach arg $args {
>   	if {[string index $arg 0] == "!"} {
> -	    set ok 0
> +	    set required_val 0
>   	    set fn [string range $arg 1 end]
>   	} else {
> -	    set ok 1
> +	    set required_val 1
>   	    set fn $arg
>   	}
> -	if {$ok != !![uplevel 1 $fn]} {
> -	    unsupported "require failed: $arg"
> +
> +	set result [uplevel 1 $fn]
> +	set len [llength $result]
> +	if { $len == 2 } {
> +	    set actual_val [lindex $result 0]
> +	    set msg [lindex $result 1]
> +	} elseif { $len == 1 } {
> +	    set actual_val $result
> +	    set msg ""
> +	} else {
> +	    error "proc $fn returned a list of unexpected length $len"
> +	}
> +
> +	if {$required_val != !!$actual_val} {
> +	    if { [string length $msg] > 0 } {
> +		unsupported "require failed: $arg ($msg)"
> +	    } else {
> +		unsupported "require failed: $arg"
> +	    }
> +
>   	    return -code return 0
>   	}
>       }
> diff --git a/gdb/testsuite/lib/rocm.exp b/gdb/testsuite/lib/rocm.exp
> index b3e435339dbc..389d73bcaa5f 100644
> --- a/gdb/testsuite/lib/rocm.exp
> +++ b/gdb/testsuite/lib/rocm.exp
> @@ -64,19 +64,19 @@ gdb_caching_proc allow_hipcc_tests {} {
>       # testing against GDBserver, there's no point in running the ROCm
>       # tests.
>       if {[target_info gdb_protocol] != ""} {
> -	return 0
> +	return {0 "remote debugging"}
>       }
>   
>       # Ensure that GDB is built with amd-dbgapi support.
>       set output [remote_exec host $::GDB "$::INTERNAL_GDBFLAGS --configuration"]
>       if { [string first "--with-amd-dbgapi" $output] == -1 } {
> -	return 0
> +	return {0 "amd-dbgapi not supported"}
>       }
>   
>       # Check we have a working hipcc compiler available.
>       set targets [hcc_amdgpu_targets]
>       if { [llength $targets] == 0} {
> -	return 0
> +	return {0 "no suitable amdgpu targets found"}
>       }
>   
>       set flags [list hip additional_flags=--offload-arch=[join $targets ","]]
> @@ -93,7 +93,7 @@ gdb_caching_proc allow_hipcc_tests {} {
>   		return 0;
>   	    }
>   	} executable $flags]} {
> -	return 0
> +	return {0 "failed to compile hip program"}
>       }
>   
>       return 1

Re: [PATCH] gdb/testsuite: allow "require" callbacks to provide a reason

[Simon Marchi]

On 3/28/23 11:44, Tom de Vries wrote:
> On 3/28/23 14:23, Simon Marchi via Gdb-patches wrote:
>> When an allow_* proc returns false, it can be a bit difficult what check
>> failed exactly, if the procedure does multiple checks.  To make
>> investigation easier, I propose to allow the "require" callbacks to be
>> able to return a list of two elements: the zero/non-zero value, and a
>> reason string.
>>
> 
> Hi,
> 
> I like the idea.
> 
> The only question I had was what the desired behaviour is for:
> ...
>   return {1 "bla"}
> ...
> 
> AFAICT, the current implementation just ignores "bla", and I wondered if perhaps we should error out to avoid the impression that something will be done with "bla".
> 
> That however will make this fail if $res == 1:
> ...
> set res [try_foo]
> return {$res "foo"}
> ...
> so probably that's just too restrictive.

I can't really think of a use case where we would return 1, with a
justification, if we always formulate our procs in the "allow" form.
That is:

  proc allow_foo_tests { } {
    if ... {
      return {0 "reason A"}
    }

    if ... {
      return {0 "reason B"}
    }

    return 1
  }

But if we have a proc in the opposite "prevent" or "skip" form (like we
had before the conversion to "allow"), it would typically written as:

  proc prevent_foo_tests { } {
    if ... {
      return {1 "reason A"}
    }

    if ... {
      return {1 "reason B"}
    }

    return 0
  }

and it could be used as:

  require !prevent_foo_tests

So that's an example where we could theoritically return 1 and use the
reason string.  It just won't happen often, because we it's better to
write things in the "allow" form, to avoid the double negative.


> LGTM as-is.

Thanks, will push.

Simon