[PATCH] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

[PATCH] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

[Pedro Alves]

Commit d09eba07 ("Make get_compiler_info use gdb_caching_proc")
regressed the gdb.rocm/ testsuite.  E.g.:

 Running /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp ...
 ERROR: tcl error sourcing /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp.
 ERROR: tcl error code TCL WRONGARGS
 ERROR: wrong # args: should be "gdb_real__get_compiler_info_1 language"
     while executing
 "gdb_real__get_compiler_info_1"
     ("uplevel" body line 1)
     invoked from within
 "uplevel 2 $real_name"
     (procedure "gdb_do_cache_wrap" line 3)
     invoked from within
 "gdb_do_cache_wrap $real_name {*}$args"
     (procedure "gdb_do_cache" line 98)
     invoked from within

This is actually a latent problem in gdb_do_cache_wrap, introduced in:

 commit 71f1ab80f1aabd70bce526635f84c7b849e8a0f4
 CommitDate: Mon Mar 6 16:49:19 2023 +0100

    [gdb/testsuite] Allow args in gdb_caching_proc

This change:

   # Call proc real_name and return the result, while ignoring calls to pass.
  -proc gdb_do_cache_wrap {real_name} {
  +proc gdb_do_cache_wrap {real_name args} {
       if { [info procs save_pass] != "" } {
	  return [uplevel 2 $real_name]   <<<<<<<<<<<<<<<<<<<<<<< HERE
       }
  @@ -31,7 +31,7 @@ proc gdb_do_cache_wrap {real_name} {
       rename pass save_pass
       rename ignore_pass pass

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

Missed updating the line marked with HERE above, to pass down $args.
So the case of a caching proc calling another caching proc with args
isn't handled correctly.

We could fix this by fixing the HERE line like so:

 -	  return [uplevel 2 $real_name]
 +	  return [uplevel 2 [list $real_name {*}$args]]

However, we have with_override nowadays that we can use here which
eliminates the duplicated logic, which was what was missed originally.

A new test that exposes the problem is added to
gdb.testsuite/gdb-caching-proc.exp.

This also adds a new test to gdb.testsuite/with-override.exp that I
think was missing, making sure that the inner foo override restores
the outer foo override.

Change-Id: I8b2a7366bf910902fe5f547bde58c3b475bf5133
---
 .../gdb.testsuite/gdb-caching-proc.exp        | 21 +++++++++++++------
 gdb/testsuite/gdb.testsuite/with-override.exp |  4 ++++
 gdb/testsuite/lib/cache.exp                   | 21 ++-----------------
 3 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
index f9610afd076..6b46b1c555a 100644
--- a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
+++ b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
@@ -22,23 +22,32 @@ gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg { arg } {
     return $arg
 }
 
+gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg_nested { arg } {
+    incr ::count
+    return [gdb_testsuite_gdb_caching_proc_exp_arg $arg]
+}
+
+# List of "expected $::count after running expression" and
+# "expression".
 set assertions {
-    { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" }
+    2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" }
 }
 
 set assertion_nr 0
-foreach assertion $assertions {
+foreach {expected_count assertion} $assertions {
     with_test_prefix $assertion_nr {
 	set ::count 0
 
 	gdb_assert $assertion
-	gdb_assert { $::count == 1 }
+	gdb_assert { $::count == $expected_count }
 
 	with_test_prefix cached {
 	    gdb_assert $assertion
-	    gdb_assert { $::count == 1 }
+	    gdb_assert { $::count == $expected_count }
 	}
     }
     incr assertion_nr
diff --git a/gdb/testsuite/gdb.testsuite/with-override.exp b/gdb/testsuite/gdb.testsuite/with-override.exp
index 1a4ee6adbbb..2a316f904e4 100644
--- a/gdb/testsuite/gdb.testsuite/with-override.exp
+++ b/gdb/testsuite/gdb.testsuite/with-override.exp
@@ -42,6 +42,10 @@ with_test_prefix no-args {
 		gdb_assert { [foo] == 2 }
 	    }
 	}
+
+	with_test_prefix "foo1 again" {
+	    gdb_assert { [foo] == 1 }
+	}
     }
 
     with_test_prefix after {
diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index 6ca3f183f9f..b4c5fab2b98 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -24,26 +24,9 @@ proc ignore_pass { msg } {
 
 # Call proc real_name and return the result, while ignoring calls to pass.
 proc gdb_do_cache_wrap {real_name args} {
-    if { [info procs save_pass] != "" } {
-	return [uplevel 2 $real_name]
+    with_override pass ignore_pass {
+	return [$real_name {*}$args]
     }
-
-    rename pass save_pass
-    rename ignore_pass pass
-
-    set code [catch {uplevel 2 [list $real_name {*}$args]} result]
-
-    rename pass ignore_pass
-    rename save_pass pass
-
-    if {$code == 1} {
-	global errorInfo errorCode
-	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
-    } elseif {$code > 1} {
-	return -code $code $result
-    }
-
-    return $result
 }
 
 # Global written to by gdb_exit_called proc.  Is set to true to

base-commit: d09eba07ca013e6b95eeafd67d79c32ebf6f28eb
-- 
2.51.0

Re: [PATCH] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

[Pedro Alves]

I should have mentioned something here:

On 2025-09-15 22:28, Pedro Alves wrote:

> --- a/gdb/testsuite/lib/cache.exp
> +++ b/gdb/testsuite/lib/cache.exp
> @@ -24,26 +24,9 @@ proc ignore_pass { msg } {
>  
>  # Call proc real_name and return the result, while ignoring calls to pass.
>  proc gdb_do_cache_wrap {real_name args} {
> -    if { [info procs save_pass] != "" } {
> -	return [uplevel 2 $real_name]
> +    with_override pass ignore_pass {
> +	return [$real_name {*}$args]
>      }

The previous code was using "uplevel 2" here.  But I'm struggling to see why that is the
right level.

When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of
the caller of the gdb_caching_proc, which off hand would seem right.

However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it
takes us to ... the caller of test_proc?

So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap
that not having the uplevel in gdb_do_cache_wrap would get wrong?  A caching proc IMO shouldn't be doing
something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't
be guaranteed to be idempotent?  So I dropped the uplevel, and found that that doesn't cause any
testsuite regression.  But maybe I'm missing something.  If I am, it'd be nice to add a testcase for it.

Pedro Alves

[PATCH] Make gdb_caching_proc support namespaces

[Pedro Alves]

On 2025-09-15 22:54, Pedro Alves wrote:
> I should have mentioned something here:
> 
> On 2025-09-15 22:28, Pedro Alves wrote:
> 
>> --- a/gdb/testsuite/lib/cache.exp
>> +++ b/gdb/testsuite/lib/cache.exp
>> @@ -24,26 +24,9 @@ proc ignore_pass { msg } {
>>  
>>  # Call proc real_name and return the result, while ignoring calls to pass.
>>  proc gdb_do_cache_wrap {real_name args} {
>> -    if { [info procs save_pass] != "" } {
>> -	return [uplevel 2 $real_name]
>> +    with_override pass ignore_pass {
>> +	return [$real_name {*}$args]
>>      }
> 
> The previous code was using "uplevel 2" here.  But I'm struggling to see why that is the
> right level.
> 
> When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of
> the caller of the gdb_caching_proc, which off hand would seem right.
> 
> However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it
> takes us to ... the caller of test_proc?
> 
> So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap
> that not having the uplevel in gdb_do_cache_wrap would get wrong?  A caching proc IMO shouldn't be doing
> something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't
> be guaranteed to be idempotent?  So I dropped the uplevel, and found that that doesn't cause any
> testsuite regression.  But maybe I'm missing something.  If I am, it'd be nice to add a testcase for it.

Trying to find a testcase for the above, I thought to try a gdb_caching_proc defined in a namespace,
and have that proc access a namespace variable, in its own namespace.

That ran into the fact that gdb_caching_proc doesn't support namespaces today.  So I fixed that, with
the patch below (on top of the previous one), which includes tests.  That still didn't require any
uplevel in gdb_do_cache_wrap.

I don't have a current use for this, but I figure that eventually we will find a use, and since
I wrote it...

== 8< ===
From e9bad21e186f6024cbbecc97b4b0f593bc8ed7ea Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 16 Sep 2025 00:41:30 +0100
Subject: [PATCH] Make gdb_caching_proc support namespaces

Currently, defining a gdb_caching_proc in a namespace, like:

namespace eval myns {
    gdb_caching_proc nsproc {} {
      return 1
    }
}

Incorrectly defines the advertised procedure in the global namespace
instead of in the namespace of the procedure being wrapped.

I.e., for the example above, calling myns::nsproc would fail, and
calling "::nsproc" would succeed.

This patch fixes it, and adds tests to
gdb.testsuite/gdb-caching-proc.exp.

Change-Id: I1853f9f411406331eb44d53d028d65a3d400db7f
---
 .../gdb.testsuite/gdb-caching-proc.exp        | 39 ++++++++++----
 gdb/testsuite/lib/cache.exp                   | 54 ++++++++++++-------
 2 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
index 6b46b1c555a..8b5168e5394 100644
--- a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
+++ b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
@@ -27,27 +27,46 @@ gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg_nested { arg } {
     return [gdb_testsuite_gdb_caching_proc_exp_arg $arg]
 }
 
-# List of "expected $::count after running expression" and
+namespace eval myns {
+    variable ns_count 1
+
+    gdb_caching_proc nsproc {} {
+	variable ns_count
+	incr ns_count
+	return 1
+    }
+}
+
+gdb_caching_proc ::fq_proc {} {
+    incr ::count
+    return 1
+}
+
+# List of "VAR, expected value of VAR after running expression", and
 # "expression".
 set assertions {
-    1 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
-    1 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
-    1 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
-    1 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" }
-    2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" }
+    count 2 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
+    count 2 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
+    count 2 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    count 2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" }
+    count 3 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" }
+    count 2 { [fq_proc] == 1 }
+    myns::ns_count 2 { [myns::nsproc] == 1 }
 }
 
 set assertion_nr 0
-foreach {expected_count assertion} $assertions {
+foreach {var expected_var_value assertion} $assertions {
     with_test_prefix $assertion_nr {
-	set ::count 0
+	# Start at 1 to be sure the "incr" in the test procs isn't the
+	# one creating the variable.
+	set $var 1
 
 	gdb_assert $assertion
-	gdb_assert { $::count == $expected_count }
+	gdb_assert { [set $var] == $expected_var_value }
 
 	with_test_prefix cached {
 	    gdb_assert $assertion
-	    gdb_assert { $::count == $expected_count }
+	    gdb_assert { [set $var] == $expected_var_value }
 	}
     }
     incr assertion_nr
diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index b4c5fab2b98..bbe07387b21 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -78,31 +78,29 @@ proc gdb_cache_maybe_gdb_exit { name do_exit also_called } {
 	return
     }
 
-    # To track if this proc has been called for NAME we create a
-    # global variable.  In gdb_cleanup_globals (see gdb.exp) this
-    # global will be deleted when the test has finished.
-    set global_name __${name}__cached_gdb_exit_called
-    if { ![info exists ::${global_name}] } {
+    # To track if this proc has been called for NAME we use a global
+    # array.  In gdb_cleanup_globals (see gdb.exp) this array will be
+    # deleted when the test has finished.
+    if { ![info exists ::gdb_caching_proc_called($name)] } {
 	gdb_exit
 	verbose -log "gdb_caching_proc $name caused gdb_exit to be called"
-	set ::${global_name} true
-	verbose -log "  gdb_caching_proc $name marked as called"
+	set ::gdb_caching_proc_called($name) true
+	verbose -log "	gdb_caching_proc $name marked as called"
 
 	foreach other_name $also_called {
 	    verbose -log "  gdb_caching_proc $other_name marked as called"
-	    set global_name __${other_name}__cached_gdb_exit_called
-	    set ::${global_name} true
+	    set ::gdb_caching_proc_called($other_name) true
 	}
     }
 }
 
 # A helper for gdb_caching_proc that handles the caching.
 
-proc gdb_do_cache {name args} {
+proc gdb_do_cache {name real_name args} {
     global gdb_data_cache objdir
     global GDB_PARALLEL
 
-    verbose -log "gdb_do_cache: $name ( $args )"
+    verbose -log "gdb_do_cache: $name $real_name ( $args )"
 
     # Normally, if we have a cached value, we skip computation and return
     # the cached value.  If set to 1, instead don't skip computation and
@@ -115,11 +113,15 @@ proc gdb_do_cache {name args} {
 	set cache_verify 1
     }
 
+    # NAME is always a fully qualified name here.  Don't include the
+    # leading "::" in the cache file name.
+    set cache_file_name [string range $name 2 end]
+
     # See if some other process wrote the cache file.  Cache value per
     # "board" to handle runs with multiple options
     # (e.g. unix/{-m32,-64}) correctly.  We use "file join" here
     # because we later use this in a real filename.
-    set cache_name [file join [target_info name] $name {*}$args]
+    set cache_name [file join [target_info name] $cache_file_name {*}$args]
 
     set is_cached 0
     if {[info exists gdb_data_cache(${cache_name},value)]} {
@@ -194,7 +196,6 @@ proc gdb_do_cache {name args} {
     set old_gdb_nested_caching_proc_calls $::gdb_nested_caching_proc_calls
     set ::gdb_nested_caching_proc_calls {}
 
-    set real_name gdb_real__$name
     set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
     set gdb_data_cache(${cache_name},exit) $::gdb_exit_called
     set gdb_data_cache(${cache_name},also_called) \
@@ -276,15 +277,32 @@ proc gdb_do_cache {name args} {
 #
 
 proc gdb_caching_proc {name arglist body} {
-    # Define the underlying proc that we'll call.
-    set real_name gdb_real__$name
-    proc $real_name $arglist $body
+    # Figure out the caller's namespace.
+    set ns [uplevel 1 {namespace current}]
+
+    # Normalize the advertised name.
+    if {[string match ::* $name]} {
+	set fq_name $name
+    } elseif {$ns eq "::"} {
+	set fq_name ::$name
+    } else {
+	set fq_name ${ns}::$name
+    }
+
+    set tail [namespace tail $fq_name]
+    set qual [namespace qualifiers $fq_name]
+
+    # The name of the underlying proc that we'll call.
+    set fq_real_name ${qual}::gdb_real__$tail
+
+    # Define it.
+    proc $fq_real_name $arglist $body
 
     # Define the advertised proc.
-    set caching_proc_body [list gdb_do_cache $name]
+    set caching_proc_body [list gdb_do_cache $fq_name $fq_real_name]
     foreach arg $arglist {
 	lappend caching_proc_body $$arg
     }
     set caching_proc_body [join $caching_proc_body]
-    proc $name $arglist $caching_proc_body
+    proc $fq_name $arglist $caching_proc_body
 }

base-commit: d09eba07ca013e6b95eeafd67d79c32ebf6f28eb
prerequisite-patch-id: 7193acb0815df9f5fbc926bb2169131538e88bd5
-- 
2.51.0

Re: [PATCH] Make gdb_caching_proc support namespaces

[Simon Marchi]

On 2025-09-15 19:57, Pedro Alves wrote:
> On 2025-09-15 22:54, Pedro Alves wrote:
>> I should have mentioned something here:
>>
>> On 2025-09-15 22:28, Pedro Alves wrote:
>>
>>> --- a/gdb/testsuite/lib/cache.exp
>>> +++ b/gdb/testsuite/lib/cache.exp
>>> @@ -24,26 +24,9 @@ proc ignore_pass { msg } {
>>>  
>>>  # Call proc real_name and return the result, while ignoring calls to pass.
>>>  proc gdb_do_cache_wrap {real_name args} {
>>> -    if { [info procs save_pass] != "" } {
>>> -	return [uplevel 2 $real_name]
>>> +    with_override pass ignore_pass {
>>> +	return [$real_name {*}$args]
>>>      }
>>
>> The previous code was using "uplevel 2" here.  But I'm struggling to see why that is the
>> right level.
>>
>> When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of
>> the caller of the gdb_caching_proc, which off hand would seem right.
>>
>> However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it
>> takes us to ... the caller of test_proc?
>>
>> So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap
>> that not having the uplevel in gdb_do_cache_wrap would get wrong?  A caching proc IMO shouldn't be doing
>> something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't
>> be guaranteed to be idempotent?  So I dropped the uplevel, and found that that doesn't cause any
>> testsuite regression.  But maybe I'm missing something.  If I am, it'd be nice to add a testcase for it.
> 
> Trying to find a testcase for the above, I thought to try a gdb_caching_proc defined in a namespace,
> and have that proc access a namespace variable, in its own namespace.
> 
> That ran into the fact that gdb_caching_proc doesn't support namespaces today.  So I fixed that, with
> the patch below (on top of the previous one), which includes tests.  That still didn't require any
> uplevel in gdb_do_cache_wrap.
> 
> I don't have a current use for this, but I figure that eventually we will find a use, and since
> I wrote it...

I haven't looked at the code, but it fixes the problem for me.

Tested-By: Simon Marchi <simon.marchi@efficios.com>

Simon

[PATCH v2] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

[Pedro Alves]

On 2025-09-15 22:54, Pedro Alves wrote:
> I should have mentioned something here:
> 
> On 2025-09-15 22:28, Pedro Alves wrote:
> 
>> --- a/gdb/testsuite/lib/cache.exp
>> +++ b/gdb/testsuite/lib/cache.exp
>> @@ -24,26 +24,9 @@ proc ignore_pass { msg } {
>>  
>>  # Call proc real_name and return the result, while ignoring calls to pass.
>>  proc gdb_do_cache_wrap {real_name args} {
>> -    if { [info procs save_pass] != "" } {
>> -	return [uplevel 2 $real_name]
>> +    with_override pass ignore_pass {
>> +	return [$real_name {*}$args]
>>      }
> 
> The previous code was using "uplevel 2" here.  But I'm struggling to see why that is the
> right level.
> 
> When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of
> the caller of the gdb_caching_proc, which off hand would seem right.
> 
> However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it
> takes us to ... the caller of test_proc?
> 
> So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap
> that not having the uplevel in gdb_do_cache_wrap would get wrong?  A caching proc IMO shouldn't be doing
> something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't
> be guaranteed to be idempotent?  So I dropped the uplevel, and found that that doesn't cause any
> testsuite regression.  But maybe I'm missing something.  If I am, it'd be nice to add a testcase for it.

I guess that is sort of a tangent.  I should preserve the uplevel so that the patch does just one thing.
A change to that detail can always be done separately.

Here's a new version of the patch without that change.  With that out of the way, this patch should be a
lot more obvious.

I'd like to merge it soon to unbreak the gdb.rocm/ tests.

=== 8< ===
From 50e4abd32c3f162f6221bdb4946ef7c18f29d7ed Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 15 Sep 2025 22:12:28 +0100
Subject: [PATCH v2] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

Commit d09eba07 ("Make get_compiler_info use gdb_caching_proc")
regressed the gdb.rocm/ testsuite.  E.g.:

 Running /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp ...
 ERROR: tcl error sourcing /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp.
 ERROR: tcl error code TCL WRONGARGS
 ERROR: wrong # args: should be "gdb_real__get_compiler_info_1 language"
     while executing
 "gdb_real__get_compiler_info_1"
     ("uplevel" body line 1)
     invoked from within
 "uplevel 2 $real_name"
     (procedure "gdb_do_cache_wrap" line 3)
     invoked from within
 "gdb_do_cache_wrap $real_name {*}$args"
     (procedure "gdb_do_cache" line 98)
     invoked from within

This is actually a latent problem in gdb_do_cache_wrap, introduced in:

 commit 71f1ab80f1aabd70bce526635f84c7b849e8a0f4
 CommitDate: Mon Mar 6 16:49:19 2023 +0100

    [gdb/testsuite] Allow args in gdb_caching_proc

This change:

   # Call proc real_name and return the result, while ignoring calls to pass.
  -proc gdb_do_cache_wrap {real_name} {
  +proc gdb_do_cache_wrap {real_name args} {
       if { [info procs save_pass] != "" } {
	  return [uplevel 2 $real_name]   <<<<<<<<<<<<<<<<<<<<<<< HERE
       }
  @@ -31,7 +31,7 @@ proc gdb_do_cache_wrap {real_name} {
       rename pass save_pass
       rename ignore_pass pass

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

Missed updating the line marked with HERE above, to pass down $args.
So the case of a caching proc calling another caching proc with args
isn't handled correctly.

We could fix this by fixing the HERE line like so:

 -	  return [uplevel 2 $real_name]
 +	  return [uplevel 2 [list $real_name {*}$args]]

However, we have with_override nowadays that we can use here which
eliminates the duplicated logic, which was what was missed originally.

A new test that exposes the problem is added to
gdb.testsuite/gdb-caching-proc.exp.

This also adds a new test to gdb.testsuite/with-override.exp that I
think was missing, making sure that the inner foo override restores
the outer foo override.

Tested-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I8b2a7366bf910902fe5f547bde58c3b475bf5133
---
 .../gdb.testsuite/gdb-caching-proc.exp        | 21 +++++++++++++------
 gdb/testsuite/gdb.testsuite/with-override.exp |  4 ++++
 gdb/testsuite/lib/cache.exp                   | 21 ++-----------------
 3 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
index f9610afd076..6b46b1c555a 100644
--- a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
+++ b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
@@ -22,23 +22,32 @@ gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg { arg } {
     return $arg
 }
 
+gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg_nested { arg } {
+    incr ::count
+    return [gdb_testsuite_gdb_caching_proc_exp_arg $arg]
+}
+
+# List of "expected $::count after running expression" and
+# "expression".
 set assertions {
-    { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" }
+    2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" }
 }
 
 set assertion_nr 0
-foreach assertion $assertions {
+foreach {expected_count assertion} $assertions {
     with_test_prefix $assertion_nr {
 	set ::count 0
 
 	gdb_assert $assertion
-	gdb_assert { $::count == 1 }
+	gdb_assert { $::count == $expected_count }
 
 	with_test_prefix cached {
 	    gdb_assert $assertion
-	    gdb_assert { $::count == 1 }
+	    gdb_assert { $::count == $expected_count }
 	}
     }
     incr assertion_nr
diff --git a/gdb/testsuite/gdb.testsuite/with-override.exp b/gdb/testsuite/gdb.testsuite/with-override.exp
index 1a4ee6adbbb..2a316f904e4 100644
--- a/gdb/testsuite/gdb.testsuite/with-override.exp
+++ b/gdb/testsuite/gdb.testsuite/with-override.exp
@@ -42,6 +42,10 @@ with_test_prefix no-args {
 		gdb_assert { [foo] == 2 }
 	    }
 	}
+
+	with_test_prefix "foo1 again" {
+	    gdb_assert { [foo] == 1 }
+	}
     }
 
     with_test_prefix after {
diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index 6ca3f183f9f..4ebb8259957 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -24,26 +24,9 @@ proc ignore_pass { msg } {
 
 # Call proc real_name and return the result, while ignoring calls to pass.
 proc gdb_do_cache_wrap {real_name args} {
-    if { [info procs save_pass] != "" } {
-	return [uplevel 2 $real_name]
+    with_override pass ignore_pass {
+	return [uplevel 2 [list $real_name {*}$args]]
     }
-
-    rename pass save_pass
-    rename ignore_pass pass
-
-    set code [catch {uplevel 2 [list $real_name {*}$args]} result]
-
-    rename pass ignore_pass
-    rename save_pass pass
-
-    if {$code == 1} {
-	global errorInfo errorCode
-	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
-    } elseif {$code > 1} {
-	return -code $code $result
-    }
-
-    return $result
 }
 
 # Global written to by gdb_exit_called proc.  Is set to true to

base-commit: d09eba07ca013e6b95eeafd67d79c32ebf6f28eb
-- 
2.51.0

Re: [PATCH v2] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests

[Pedro Alves]

On 2025-09-16 10:31, Pedro Alves wrote:
> On 2025-09-15 22:54, Pedro Alves wrote:
>> I should have mentioned something here:
>>
>> On 2025-09-15 22:28, Pedro Alves wrote:
>>
>>> --- a/gdb/testsuite/lib/cache.exp
>>> +++ b/gdb/testsuite/lib/cache.exp
>>> @@ -24,26 +24,9 @@ proc ignore_pass { msg } {
>>>  
>>>  # Call proc real_name and return the result, while ignoring calls to pass.
>>>  proc gdb_do_cache_wrap {real_name args} {
>>> -    if { [info procs save_pass] != "" } {
>>> -	return [uplevel 2 $real_name]
>>> +    with_override pass ignore_pass {
>>> +	return [$real_name {*}$args]
>>>      }
>>
>> The previous code was using "uplevel 2" here.  But I'm struggling to see why that is the
>> right level.
>>
>> When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of
>> the caller of the gdb_caching_proc, which off hand would seem right.
>>
>> However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it
>> takes us to ... the caller of test_proc?
>>
>> So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap
>> that not having the uplevel in gdb_do_cache_wrap would get wrong?  A caching proc IMO shouldn't be doing
>> something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't
>> be guaranteed to be idempotent?  So I dropped the uplevel, and found that that doesn't cause any
>> testsuite regression.  But maybe I'm missing something.  If I am, it'd be nice to add a testcase for it.
> 
> I guess that is sort of a tangent.  I should preserve the uplevel so that the patch does just one thing.
> A change to that detail can always be done separately.
> 
> Here's a new version of the patch without that change.  With that out of the way, this patch should be a
> lot more obvious.
> 
> I'd like to merge it soon to unbreak the gdb.rocm/ tests.


I noticed now that while a full test run did not expose this issue, which explains why I didn't see it
originally, I'm able to trigger this with tests other than gdb.rocm/, as long as you run them in
isolation, so that the cache is cold for the nested procs.  For example, 

  make check TESTS="gdb.base/attach.exp"

triggers it too.

I'm going ahead and merging it, to minimize disturbances to everyone, with the adjusted commit log below.

=== 8< ===
From 2e7ea115f4bddcd7d3f81e0e9026b76fef4d4253 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 15 Sep 2025 22:12:28 +0100
Subject: [PATCH] Fix nested gdb_caching_proc with args

Commit d09eba07 ("Make get_compiler_info use gdb_caching_proc")
regressed some tests when you run them in isolation (as this depends
on the order the gdb_caching_proc procs' results are cached).

E.g.:

 Running /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp ...
 ERROR: tcl error sourcing /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp.
 ERROR: tcl error code TCL WRONGARGS
 ERROR: wrong # args: should be "gdb_real__get_compiler_info_1 language"
     while executing
 "gdb_real__get_compiler_info_1"
     ("uplevel" body line 1)
     invoked from within
 "uplevel 2 $real_name"
     (procedure "gdb_do_cache_wrap" line 3)
     invoked from within
 "gdb_do_cache_wrap $real_name {*}$args"
     (procedure "gdb_do_cache" line 98)
     invoked from within

gdb.base/attach.exp triggers it too, for example.

This is actually a latent problem in gdb_do_cache_wrap, introduced in:

 commit 71f1ab80f1aabd70bce526635f84c7b849e8a0f4
 CommitDate: Mon Mar 6 16:49:19 2023 +0100

    [gdb/testsuite] Allow args in gdb_caching_proc

This change:

   # Call proc real_name and return the result, while ignoring calls to pass.
  -proc gdb_do_cache_wrap {real_name} {
  +proc gdb_do_cache_wrap {real_name args} {
       if { [info procs save_pass] != "" } {
	  return [uplevel 2 $real_name]   <<<<<<<<<<<<<<<<<<<<<<< HERE
       }
  @@ -31,7 +31,7 @@ proc gdb_do_cache_wrap {real_name} {
       rename pass save_pass
       rename ignore_pass pass

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

Missed updating the line marked with HERE above, to pass down $args.
So the case of a caching proc calling another caching proc with args
isn't handled correctly.

We could fix this by fixing the HERE line like so:

 -	  return [uplevel 2 $real_name]
 +	  return [uplevel 2 [list $real_name {*}$args]]

However, we have with_override nowadays that we can use here which
eliminates the duplicated logic, which was what was missed originally.

A new test that exposes the problem is added to
gdb.testsuite/gdb-caching-proc.exp.

This also adds a new test to gdb.testsuite/with-override.exp that I
think was missing, making sure that the inner foo override restores
the outer foo override.

Tested-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I8b2a7366bf910902fe5f547bde58c3b475bf5133
---
 .../gdb.testsuite/gdb-caching-proc.exp        | 21 +++++++++++++------
 gdb/testsuite/gdb.testsuite/with-override.exp |  4 ++++
 gdb/testsuite/lib/cache.exp                   | 21 ++-----------------
 3 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
index f9610afd076..6b46b1c555a 100644
--- a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
+++ b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp
@@ -22,23 +22,32 @@ gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg { arg } {
     return $arg
 }
 
+gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg_nested { arg } {
+    incr ::count
+    return [gdb_testsuite_gdb_caching_proc_exp_arg $arg]
+}
+
+# List of "expected $::count after running expression" and
+# "expression".
 set assertions {
-    { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
-    { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" }
+    1 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" }
+    2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" }
 }
 
 set assertion_nr 0
-foreach assertion $assertions {
+foreach {expected_count assertion} $assertions {
     with_test_prefix $assertion_nr {
 	set ::count 0
 
 	gdb_assert $assertion
-	gdb_assert { $::count == 1 }
+	gdb_assert { $::count == $expected_count }
 
 	with_test_prefix cached {
 	    gdb_assert $assertion
-	    gdb_assert { $::count == 1 }
+	    gdb_assert { $::count == $expected_count }
 	}
     }
     incr assertion_nr
diff --git a/gdb/testsuite/gdb.testsuite/with-override.exp b/gdb/testsuite/gdb.testsuite/with-override.exp
index 1a4ee6adbbb..2a316f904e4 100644
--- a/gdb/testsuite/gdb.testsuite/with-override.exp
+++ b/gdb/testsuite/gdb.testsuite/with-override.exp
@@ -42,6 +42,10 @@ with_test_prefix no-args {
 		gdb_assert { [foo] == 2 }
 	    }
 	}
+
+	with_test_prefix "foo1 again" {
+	    gdb_assert { [foo] == 1 }
+	}
     }
 
     with_test_prefix after {
diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index 6ca3f183f9f..4ebb8259957 100644
--- a/gdb/testsuite/lib/cache.exp
+++ b/gdb/testsuite/lib/cache.exp
@@ -24,26 +24,9 @@ proc ignore_pass { msg } {
 
 # Call proc real_name and return the result, while ignoring calls to pass.
 proc gdb_do_cache_wrap {real_name args} {
-    if { [info procs save_pass] != "" } {
-	return [uplevel 2 $real_name]
+    with_override pass ignore_pass {
+	return [uplevel 2 [list $real_name {*}$args]]
     }
-
-    rename pass save_pass
-    rename ignore_pass pass
-
-    set code [catch {uplevel 2 [list $real_name {*}$args]} result]
-
-    rename pass ignore_pass
-    rename save_pass pass
-
-    if {$code == 1} {
-	global errorInfo errorCode
-	return -code error -errorinfo $errorInfo -errorcode $errorCode $result
-    } elseif {$code > 1} {
-	return -code $code $result
-    }
-
-    return $result
 }
 
 # Global written to by gdb_exit_called proc.  Is set to true to

base-commit: d09eba07ca013e6b95eeafd67d79c32ebf6f28eb
-- 
2.51.0