[gdb/testsuite] Allow args in gdb_caching_proc

  Test-case gdb.base/morestack.exp contains:
...
require {have_compile_flag -fsplit-stack}
...
and I want to cache the result of have_compile_flag.

Currently gdb_caching_proc doesn't allow args, so I could add:
...
gdb_caching_proc have_compile_flag_fsplit_stack {
    return [have_compile_flag -fsplit-stack]
}
...
and then use that proc instead, but I find this cumbersome and
maintenance-unfriendly.

Instead, allow args in a gdb_caching_proc, such that I can simply do:
...
-proc have_compile_flag { flag } {
+gdb_caching_proc have_compile_flag { flag } {
...

Note that gdb_caching_procs with args do not work with the
gdb.base/gdb-caching-procs.exp test-case, so those procs are skipped.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.base/gdb-caching-proc.exp | 11 +++--
 gdb/testsuite/lib/cache.exp                 | 50 ++++++++++++++++-----
 gdb/testsuite/lib/gdb.exp                   |  2 +-
 3 files changed, 48 insertions(+), 15 deletions(-)


base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134
  

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Test-case gdb.base/morestack.exp contains:
Tom> ...
Tom> require {have_compile_flag -fsplit-stack}
Tom> ...
Tom> and I want to cache the result of have_compile_flag.

Seems reasonable.

Tom> +	if { ![regexp -- "^gdb_caching_proc \[ \t\]*(\[^ \t\]*)" $line \
Tom> +		   match procname]  } {
Tom> +	    continue
Tom>  	}
Tom> +	if { [regexp -- "{\[ \t\]*\[^ \t\].*}" $line] } {
Tom> +	    # With args.
Tom> +	    continue
Tom> +	}
Tom> +	lappend procnames $procname

In Tcl you can inspect the arguments to a proc using 'info args', which
may be more robust than a regexp.  I didn't read the test to see if this
is really possible in this case, though.

Tom> -    set code [catch {uplevel 2 $real_name} result]
Tom> +    set code [catch {uplevel 2 $real_name {*}$args} result]

I think this is the wrong quoting and will cause problems if any
argument has a space.  Instead I think you want:

+    set code [catch {uplevel 2 [list $real_name {*}$args]} result]

Tom> -proc gdb_caching_proc {name body} {
Tom> +proc gdb_caching_proc_1 { name args body } {

I would call this something other than 'args'.  The name 'args' is
special in Tcl when it's the last argument, so it's confusing to find it
other places.

Tom>      # Define the advertised proc.
Tom> -    proc $name {} [list gdb_do_cache $name]
Tom> +    set body [list "gdb_do_cache" "$name"]

The quotes here aren't needed.  It's safe to add them if you like them,
but they don't do anything.

Also reusing 'body' here seems weird.

Tom> +    foreach arg $args {
Tom> +	lappend body $$arg
Tom> +    }
Tom> +    set body [join $body]

"join" is usually wrong when constructing lists, and I think here this
line can just be dropped.

I notice this doesn't handle defaulted arguments.  This isn't hard if
you want to do it; each argument is a list and in that loop you'd just
use [lindex $arg 0].

Tom> +# Define a new proc named NAME, with optional args ARGS.  BODY is the body of
Tom> +# the proc.  The proc will evaluate BODY and cache the results, both in memory
Tom> +# and, if GDB_PARALLEL is defined, in the filesystem for use across
Tom> +# invocations of dejagnu.
Tom> +#
Tom> +# Valid forms:
Tom> +#   gdb_caching_proc NAME BODY
Tom> +#   gdb_caching_proc NAME ARGS BODY
Tom> +
Tom> +proc gdb_caching_proc { name args } {

I think it would be better to just require gdb_caching_proc to work
exactly like proc, and fix all the existing calls to add a {} for the
arg list.

Tom> +    if { [llength $args] == 1 } {
Tom> +	set body [lindex $args 0]
Tom> +	set args {}
Tom> +    } elseif { [llength $args] == 2 } {
Tom> +	set body [lindex $args 1]
Tom> +	set args [lindex $args 0]
Tom> +    } else {
Tom> +	error "Incorrect number of arguments"
Tom> +    }

Then none of this would be needed either.

Tom
  

Tom>      # (e.g. unix/{-m32,-64}) correctly.  We use "file join" here
Tom>      # because we later use this in a real filename.
Tom>      set cache_name [file join [target_info name] $name]
Tom> +    foreach arg $args {
Tom> +	set cache_name [file join $cache_name $arg]
Tom> +    }

Also I think this can all be:

set cache_name [file join [target_info name] $name {*}$args]

Tom
  

On 3/3/23 15:32, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Test-case gdb.base/morestack.exp contains:
> Tom> ...
> Tom> require {have_compile_flag -fsplit-stack}
> Tom> ...
> Tom> and I want to cache the result of have_compile_flag.
> 
> Seems reasonable.
> 
> Tom> +	if { ![regexp -- "^gdb_caching_proc \[ \t\]*(\[^ \t\]*)" $line \
> Tom> +		   match procname]  } {
> Tom> +	    continue
> Tom>  	}
> Tom> +	if { [regexp -- "{\[ \t\]*\[^ \t\].*}" $line] } {
> Tom> +	    # With args.
> Tom> +	    continue
> Tom> +	}
> Tom> +	lappend procnames $procname
> 
> In Tcl you can inspect the arguments to a proc using 'info args', which
> may be more robust than a regexp.  I didn't read the test to see if this
> is really possible in this case, though.
> 

Fixed.

> Tom> -    set code [catch {uplevel 2 $real_name} result]
> Tom> +    set code [catch {uplevel 2 $real_name {*}$args} result]
> 
> I think this is the wrong quoting and will cause problems if any
> argument has a space.  Instead I think you want:
> 
> +    set code [catch {uplevel 2 [list $real_name {*}$args]} result]
> 

I wrote a test-case and managed to trigger the problem, and your 
suggestion indeed fixes it.

> Tom> -proc gdb_caching_proc {name body} {
> Tom> +proc gdb_caching_proc_1 { name args body } {
> 
> I would call this something other than 'args'.  The name 'args' is
> special in Tcl when it's the last argument, so it's confusing to find it
> other places.
> 

Renamed to arglist.

> Tom>      # Define the advertised proc.
> Tom> -    proc $name {} [list gdb_do_cache $name]
> Tom> +    set body [list "gdb_do_cache" "$name"]
> 
> The quotes here aren't needed.  It's safe to add them if you like them,
> but they don't do anything.
> 

Ack, removed quotes.

> Also reusing 'body' here seems weird.
> 
> Tom> +    foreach arg $args {
> Tom> +	lappend body $$arg
> Tom> +    }
> Tom> +    set body [join $body]
> 

Renamed to caching_proc_body.

> "join" is usually wrong when constructing lists, and I think here this
> line can just be dropped.
> 

Actually, it can't.  Removing the line makes the new test-case fail.

> I notice this doesn't handle defaulted arguments.  This isn't hard if
> you want to do it; each argument is a list and in that loop you'd just
> use [lindex $arg 0].
> 

I've let this aside for now.

> Tom> +# Define a new proc named NAME, with optional args ARGS.  BODY is the body of
> Tom> +# the proc.  The proc will evaluate BODY and cache the results, both in memory
> Tom> +# and, if GDB_PARALLEL is defined, in the filesystem for use across
> Tom> +# invocations of dejagnu.
> Tom> +#
> Tom> +# Valid forms:
> Tom> +#   gdb_caching_proc NAME BODY
> Tom> +#   gdb_caching_proc NAME ARGS BODY
> Tom> +
> Tom> +proc gdb_caching_proc { name args } {
> 
> I think it would be better to just require gdb_caching_proc to work
> exactly like proc, and fix all the existing calls to add a {} for the
> arg list.
> 

I've added a separate patch for that.

> Tom> +    if { [llength $args] == 1 } {
> Tom> +	set body [lindex $args 0]
> Tom> +	set args {}
> Tom> +    } elseif { [llength $args] == 2 } {
> Tom> +	set body [lindex $args 1]
> Tom> +	set args [lindex $args 0]
> Tom> +    } else {
> Tom> +	error "Incorrect number of arguments"
> Tom> +    }
> 
> Then none of this would be needed either.

Ack, removed.

I've submitted a v2 series here ( 
https://sourceware.org/pipermail/gdb-patches/2023-March/197645.html ).

Thanks,
- Tom