Re: [PATCH 4/4] gdb/testsuite: track if a caching proc calls gdb_exit or not

[Luis Machado <luis.machado@arm.com>]

Hi Andrew,

On 8/7/24 15:31, Andrew Burgess wrote:
> Luis Machado <luis.machado@arm.com> writes:
> 
>> Hi Andrew,
>>
>> On 6/3/24 19:16, Andrew Burgess wrote:
>>> After a recent patch review I asked myself why can_spawn_for_attach
>>> exists.  This proc currently does some checks, and then calls
>>> can_spawn_for_attach_1 which is an actual caching proc.
>>>
>>> The answer is that can_spawn_for_attach exists in order to call
>>> gdb_exit the first time can_spawn_for_attach is called within any test
>>> script.
>>>
>>> The reason this is useful is that can_spawn_for_attach_1 calls
>>> gdb_exit.  If imagine the user calling can_spawn_for_attach_1 directly
>>> then a problem might exist.  Imagine a test written like this:
>>>
>>>   gdb_start
>>>
>>>   if { [can_spawn_for_attach_1] } {
>>>     ... do stuff that assumes GDB is running ...
>>>   }
>>>
>>> If this test is NOT the first test run, and if an earlier test calls
>>> can_spawn_for_attach_1, then when the above test is run the
>>> can_spawn_for_attach_1 call will return the cached value and gdb_exit
>>> will not be called.
>>>
>>> But, if the above test IS the first test run then
>>> can_spawn_for_attach_1 will not returned the cached value, but will
>>> instead compute the cached value, a process that ends up calling
>>> gdb_exit.  When the body of the if is executed GDB would no longer be
>>> running and the test would fail!
>>>
>>> So can_spawn_for_attach was added which ensures that we _always_ call
>>> gdb_exit the first time can_spawn_for_attach is called within a single
>>> test script, this ensures that in the above case, even if the above is
>>> not the first test run, gdb_exit will still be called.  This avoids
>>> some hidden bugs in the testsuite.
>>>
>>> However, what I observe is that can_spawn_for_attach is not the only
>>> caching proc that calls gdb_exit.  Why does can_spawn_for_attach get
>>> special treatment when surely the same issue exists for any other
>>> caching proc that calls gdb_exit?
>>>
>>> I think a better solution is to move the logic from
>>> can_spawn_for_attach into cache.exp and generalise it so that it
>>> applies to all caching procs.
>>>
>>> This commit does this by:
>>>
>>>  1. When the underlying caching proc is executed we wrap gdb_exit.
>>>     This wrapper sets a global to true if gdb_exit is called.  The
>>>     value of this global is stored in gdb_data_cache (using a ',exit'
>>>     suffix), and also written to the cache file if appropriate.
>>>
>>>  2. When a cached value is returned from gdb_do_cache, if the
>>>     underlying proc would have called gdb_exit, and if this is the
>>>     first use of the caching proc in this test script, then we call
>>>     gdb_exit.
>>>
>>> When storing the ',exit' value into the on-disk cache file, the flag
>>> value is stored on a second line.  Currently every cached value only
>>> occupies a single line, and a check is added to ensure this remains
>>> true in the future.
>>>
>>> One issue did come up in testing, a FAIL in gdb.base/break-interp.exp,
>>> this was caused by can_spawn_for_attach_1 calling gdb_start without
>>> first calling gdb_exit.  Under the old way of doing things
>>> can_spawn_for_attach would call gdb_exit _before_ possibly calling the
>>> actual caching proc.  Under the new scheme gdb_exit is called _after_
>>> calling the actual caching proc.  What was happening was that
>>> break-interp.exp would leave GDB running then call
>>> can_spawn_for_attach, when the test in can_spawn_for_attach_1 tried to
>>> attach to the inferior, state left in the running GDB would cause some
>>> unexpected behaviour.  Fixed by having can_spawn_for_attach_1 call
>>> gdb_exit before calling gdb_start, this ensures we have a fresh GDB.
>>>
>>> With this done can_spawn_for_attach_1 can be renamed to
>>> can_spawn_for_attach, and the existing can_spawn_for_attach can be
>>> deleted.
>>> ---
>>>  gdb/testsuite/lib/cache.exp | 86 +++++++++++++++++++++++++++++++------
>>>  gdb/testsuite/lib/gdb.exp   | 83 +++++++++--------------------------
>>>  2 files changed, 93 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
>>> index e7b9114058b..fef065ec8b0 100644
>>> --- a/gdb/testsuite/lib/cache.exp
>>> +++ b/gdb/testsuite/lib/cache.exp
>>> @@ -46,6 +46,40 @@ proc gdb_do_cache_wrap {real_name args} {
>>>      return $result
>>>  }
>>>  
>>> +# Global written to by wrap_gdb_exit.  Set to true if wrap_gdb_exit is
>>> +# called.
>>> +
>>> +set gdb_exit_called false
>>> +
>>> +# Wrapper around gdb_exit.  Use with_override to replace gdb_exit with
>>> +# wrap_gdb_exit, the original gdb_exit is renamed to orig_gdb_exit.
>>> +
>>> +proc wrap_gdb_exit {} {
>>> +    set ::gdb_exit_called true
>>> +    orig_gdb_exit
>>> +}
>>> +
>>> +# If DO_EXIT is false then this proc does nothing.  If DO_EXIT is true
>>> +# then call gdb_exit the first time this proc is called for each
>>> +# unique value of NAME within a single test.  Every subsequent time
>>> +# this proc is called within a single test (for a given value of
>>> +# NAME), don't call gdb_exit.
>>> +
>>> +proc gdb_cache_maybe_gdb_exit { name do_exit } {
>>> +    if { !$do_exit } {
>>> +	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}] } {
>>> +	gdb_exit
>>> +	set ::${global_name} true
>>> +    }
>>> +}
>>> +
>>>  # A helper for gdb_caching_proc that handles the caching.
>>>  
>>>  proc gdb_do_cache {name args} {
>>> @@ -71,10 +105,12 @@ proc gdb_do_cache {name args} {
>>>  
>>>      set is_cached 0
>>>      if {[info exists gdb_data_cache(${cache_name},value)]} {
>>> -	set cached $gdb_data_cache(${cache_name},value)
>>> -	verbose "$name: returning '$cached' from cache" 2
>>> +	set cached_value $gdb_data_cache(${cache_name},value)
>>> +	set cached_exit $gdb_data_cache(${cache_name},exit)
>>> +	verbose "$name: returning '$cached_value' from cache" 2
>>>  	if { $cache_verify == 0 } {
>>> -	    return $cached
>>> +	    gdb_cache_maybe_gdb_exit $name $cached_exit
>>> +	    return $cached_value
>>>  	}
>>>  	set is_cached 1
>>>      }
>>> @@ -83,24 +119,46 @@ proc gdb_do_cache {name args} {
>>>  	set cache_filename [make_gdb_parallel_path cache $cache_name]
>>>  	if {[file exists $cache_filename]} {
>>>  	    set fd [open $cache_filename]
>>> -	    set gdb_data_cache(${cache_name},value) [read -nonewline $fd]
>>> +	    set content [split [read -nonewline $fd] \n]
>>>  	    close $fd
>>> -	    set cached $gdb_data_cache(${cache_name},value)
>>> -	    verbose "$name: returning '$cached' from file cache" 2
>>> +	    set gdb_data_cache(${cache_name},value) [lindex $content 0]
>>> +	    set gdb_data_cache(${cache_name},exit) [lindex $content 1]
>>> +	    set cached_value $gdb_data_cache(${cache_name},value)
>>> +	    set cached_exit $gdb_data_cache(${cache_name},exit)
>>> +	    verbose "$name: returning '$cached_value' from file cache" 2
>>>  	    if { $cache_verify == 0 } {
>>> -		return $cached
>>> +		gdb_cache_maybe_gdb_exit $name $cached_exit
>>> +		return $cached_value
>>>  	    }
>>>  	    set is_cached 1
>>>  	}
>>>      }
>>>  
>>> -    set real_name gdb_real__$name
>>> -    set gdb_data_cache(${cache_name},value) [gdb_do_cache_wrap $real_name {*}$args]
>>> +    set ::gdb_exit_called false
>>> +    with_override gdb_exit wrap_gdb_exit orig_gdb_exit {
>>> +	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
>>> +
>>> +    # If a value being stored in the cache contains a newline then
>>> +    # when we try to read the value back from an on-disk cache file
>>> +    # we'll interpret the second line of the value as the ',exit' value.
>>> +    if { [regexp "\[\r\n\]" $gdb_data_cache(${cache_name},value)] } {
>>> +	set computed_value $gdb_data_cache(${cache_name},value)
>>> +	error "Newline found in value for $cache_name: $computed_value"
>>> +    }
>>> +
>>>      if { $cache_verify == 1 && $is_cached == 1 } {
>>> -	set computed $gdb_data_cache(${cache_name},value)
>>> -	if { $cached != $computed } {
>>> -	    error [join [list "Inconsistent results for $cache_name:"
>>> -			 "cached: $cached vs. computed: $computed"]]
>>> +	set computed_value $gdb_data_cache(${cache_name},value)
>>> +	set computed_exit $gdb_data_cache(${cache_name},exit)
>>> +	if { $cached_value != $computed_value } {
>>> +	    error [join [list "Inconsistent value results for $cache_name:"
>>> +			 "cached: $cached_value vs. computed: $computed_value"]]
>>> +	}
>>> +	if { $cached_exit != $computed_exit } {
>>> +	    error [join [list "Inconsistent exit results for $cache_name:"
>>> +			 "cached: $cached_exit vs. computed: $computed_exit"]]
>>>  	}
>>>      }
>>>  
>>> @@ -110,9 +168,11 @@ proc gdb_do_cache {name args} {
>>>  	# Make sure to write the results file atomically.
>>>  	set fd [open $cache_filename.[pid] w]
>>>  	puts $fd $gdb_data_cache(${cache_name},value)
>>> +	puts $fd $gdb_data_cache(${cache_name},exit)
>>>  	close $fd
>>>  	file rename -force -- $cache_filename.[pid] $cache_filename
>>>      }
>>> +    gdb_cache_maybe_gdb_exit $name $gdb_data_cache(${cache_name},exit)
>>>      return $gdb_data_cache(${cache_name},value)
>>>  }
>>>  
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index 8235d4f28eb..d29fd740f91 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -6186,14 +6186,23 @@ proc gdb_exit { } {
>>>      catch default_gdb_exit
>>>  }
>>>  
>>> -# Helper function for can_spawn_for_attach.  Try to spawn and attach, and
>>> -# return 0 only if we cannot attach because it's unsupported.
>>> -
>>> -gdb_caching_proc can_spawn_for_attach_1 {} {
>>> -    # For the benefit of gdb-caching-proc-consistency.exp, which
>>> -    # calls can_spawn_for_attach_1 directly.  Keep in sync with
>>> -    # can_spawn_for_attach.
>>> -    if { [is_remote target] || [target_info exists use_gdb_stub] } {
>>> +# Return true if we can spawn a program on the target and attach to
>>> +# it.
>>> +
>>> +gdb_caching_proc can_spawn_for_attach {} {
>>> +    # We use exp_pid to get the inferior's pid, assuming that gives
>>> +    # back the pid of the program.  On remote boards, that would give
>>> +    # us instead the PID of e.g., the ssh client, etc.
>>> +    if {[is_remote target]} {
>>> +	verbose -log "can't spawn for attach (target is remote)"
>>> +	return 0
>>> +    }
>>> +
>>> +    # The "attach" command doesn't make sense when the target is
>>> +    # stub-like, where GDB finds the program already started on
>>> +    # initial connection.
>>> +    if {[target_info exists use_gdb_stub]} {
>>> +	verbose -log "can't spawn for attach (target is stub)"
>>>  	return 0
>>>      }
>>>  
>>> @@ -6218,6 +6227,9 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
>>>      set test_spawn_id [spawn_wait_for_attach_1 $obj]
>>>      remote_file build delete $obj
>>>  
>>> +    # In case GDB is already running.
>>> +    gdb_exit
>>> +    
>>>      gdb_start
>>>  
>>>      set test_pid [spawn_id_get_pid $test_spawn_id]
>>> @@ -6239,61 +6251,6 @@ gdb_caching_proc can_spawn_for_attach_1 {} {
>>>      return $res
>>>  }
>>>  
>>> -# Return true if we can spawn a program on the target and attach to
>>> -# it.  Calls gdb_exit for the first call in a test-case.
>>> -
>>> -proc can_spawn_for_attach { } {
>>> -    # We use exp_pid to get the inferior's pid, assuming that gives
>>> -    # back the pid of the program.  On remote boards, that would give
>>> -    # us instead the PID of e.g., the ssh client, etc.
>>> -    if {[is_remote target]} {
>>> -	verbose -log "can't spawn for attach (target is remote)"
>>> -	return 0
>>> -    }
>>> -
>>> -    # The "attach" command doesn't make sense when the target is
>>> -    # stub-like, where GDB finds the program already started on
>>> -    # initial connection.
>>> -    if {[target_info exists use_gdb_stub]} {
>>> -	verbose -log "can't spawn for attach (target is stub)"
>>> -	return 0
>>> -    }
>>> -
>>> -    # The normal sequence to use for a runtime test like
>>> -    # can_spawn_for_attach_1 is:
>>> -    # - gdb_exit (don't use a running gdb, we don't know what state it is in),
>>> -    # - gdb_start (start a new gdb), and
>>> -    # - gdb_exit (cleanup).
>>> -    #
>>> -    # By making can_spawn_for_attach_1 a gdb_caching_proc, we make it
>>> -    # unpredictable which test-case will call it first, and consequently a
>>> -    # test-case may pass in say a full test run, but fail when run
>>> -    # individually, due to a can_spawn_for_attach call in a location where a
>>> -    # gdb_exit (as can_spawn_for_attach_1 does) breaks things.
>>> -    # To avoid this, we move the initial gdb_exit out of
>>> -    # can_spawn_for_attach_1, guaranteeing that we end up in the same state
>>> -    # regardless of whether can_spawn_for_attach_1 is called.  However, that
>>> -    # is only necessary for the first call in a test-case, so cache the result
>>> -    # in a global (which should be reset after each test-case) to keep track
>>> -    # of that.
>>> -    #
>>> -    # In summary, we distinguish between three cases:
>>> -    # - first call in first test-case.  Executes can_spawn_for_attach_1.
>>> -    #   Calls gdb_exit, gdb_start, gdb_exit.
>>> -    # - first call in following test-cases.  Uses cached result of
>>> -    #   can_spawn_for_attach_1.  Calls gdb_exit.
>>> -    # - rest.  Use cached result in cache_can_spawn_for_attach_1. Calls no
>>> -    #   gdb_start or gdb_exit.
>>> -    global cache_can_spawn_for_attach_1
>>> -    if { [info exists cache_can_spawn_for_attach_1] } {
>>> -	return $cache_can_spawn_for_attach_1
>>> -    }
>>> -    gdb_exit
>>> -
>>> -    set cache_can_spawn_for_attach_1 [can_spawn_for_attach_1]
>>> -    return $cache_can_spawn_for_attach_1
>>> -}
>>> -
>>>  # Centralize the failure checking of "attach" command.
>>>  # Return 0 if attach failed, otherwise return 1.
>>>  
>>
>> This is a bit after the fact, but I tracked down some aarch64 sme test regressions
>> to this particular patch. I'm still investigating exactly why it stopped working, but I
>> can tell it only happens if we run 2 or more tests in the same run. It is not
>> clear if making things parallel has an impact, or if it is just the fact we
>> run 2+ tests in the same run.
>>
>> I suspect we may be calling gdb_exit when we shouldn't, and then things just
>> stop working.
>>
>> ---
>>
>> Running target unix
>> Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
>> Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
>> Using repos/binutils-gdb/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
>> Running repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-core-0.exp ...
>> Running repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-regs-unavailable-3.exp ...
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> FAIL: gdb.arch/aarch64-sme-regs-unavailable-3.exp: prctl, vl=32 svl=256: check_regs: incorrect ZA state
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> ERROR: no fileid for ubuntu
>> FAIL: gdb.arch/aarch64-sme-regs-unavailable-3.exp: gdb, vl=32 svl=256: check_regs: incorrect ZA state
> 
> Luis,
> 
> Could you please test the patch below to see if this fixes the issues
> you are seeing.  This is also running through local testing at my side,
> but I thought I'd get your feedback early.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 27d8be90b65e0b18ff9ca8e356f0b4e95352f446
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Aug 7 14:51:06 2024 +0100
> 
>     gdb/testsuite: track nested caching proc calls
>     
>     It was pointed out in this email:
>     
>       https://inbox.sourceware.org/gdb-patches/97973506-79f4-4216-9c0b-57401b3933f5@arm.com
>     
>     that this commit:
>     
>       commit 0726729d344fecf98f8d138e688e77201cc3cece
>       Date:   Mon Jun 3 13:56:54 2024 +0100
>     
>           gdb/testsuite: track if a caching proc calls gdb_exit or not
>     
>     had broken some AArch64 tests.
>     
>     What is going on is that there are two caching procs:
>     
>       allow_aarch64_sme_tests
>       aarch64_initialize_sme_information
>     
>     the allow_aarch64_sme_tests proc makes a call to
>     aarch64_initialize_sme_information, but
>     aarch64_initialize_sme_information is also called from other
>     non-caching procs, like aarch64_supports_sme_svl.
>     
>     Both of the caching procs mentioned above compile and run a helper
>     program, and both of them call gdb_exit.
>     
>     After the above commit, the first call to any caching proc, the body
>     of which calls gdb_exit, will result in a gdb_exit call even if the
>     body is not executed and the result is fetched from the cache.
>     
>     What was observed is that in the first test script
>     allow_aarch64_sme_tests is called, the body of this caching proc is
>     run which calls gdb_exit.  Then allow_aarch64_sme_tests calls
>     aarch64_initialize_sme_information, the body of which is run and
>     gdb_exit is called again.  The results from both procs are added to
>     the cache.
>     
>     In the next test script allow_aarch64_sme_tests is called.  This
>     results in a cache hit, but gdb_exit is also called as this is the
>     first call in this second test script.
>     
>     Later in the test script aarch64_supports_sme_svl is called which
>     calls aarch64_initialize_sme_information.  As this is the first call
>     to aarch64_initialize_sme_information in this second test
>     script (remember the body of allow_aarch64_sme_tests was never run)
>     then gdb_exit is called.  This call to gdb_exit is new after the above
>     commit and is unexpected.
>     
>     I think the idea behind the above commit is still sound though.  If
>     the call to allow_aarch64_sme_tests was removed from the second test
>     script then we would want the extra gdb_exit call as this would expose
>     a real bug in the test.  The problem is that after the above commit
>     the nested nature of the caching proc calls becomes important: a call
>     to allow_aarch64_sme_tests should mean that we've also called
>     aarch64_initialize_sme_information, and that relationship isn't
>     currently captured.
>     
>     So in this commit I'm adding another field to the global
>     gdb_data_cache (in lib/cache.exp).  This new field is 'also_called'.
>     For every caching proc we'll populate this field with a list of names,
>     these are the names of any nested caching procs that are called when
>     the body of a caching proc is executed.
>     
>     Now when we get a cache hit in gdb_data_cache we mark every proc in
>     the 'also_called' list as having been called.  This means that further
>     calls to these procs will no longer trigger a gdb_exit call.
> 
> diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
> index 092b7f136e8..7e1eae9259e 100644
> --- a/gdb/testsuite/lib/cache.exp
> +++ b/gdb/testsuite/lib/cache.exp
> @@ -59,24 +59,48 @@ proc gdb_exit_called { args } {
>      set ::gdb_exit_called true
>  }
>  
> -# If DO_EXIT is false then this proc does nothing.  If DO_EXIT is true
> -# then call gdb_exit the first time this proc is called for each
> -# unique value of NAME within a single test.  Every subsequent time
> -# this proc is called within a single test (for a given value of
> -# NAME), don't call gdb_exit.
> -
> -proc gdb_cache_maybe_gdb_exit { name do_exit } {
> -    if { !$do_exit } {
> +# While calling the implementation of a caching proc, that
> +# implementation might itself call additional caching procs.  We need
> +# to track all of the nested caching procs that are called and we do
> +# that in this list which is a list containing the names of any nested
> +# caching procs that are called.
> +
> +set gdb_nested_caching_proc_calls {}
> +
> +# Called before returning from gdb_do_cache.  PROC_NAME is the name of
> +# the caching proc that was called and CACHE_NAME is that name used to
> +# store information in the global gdb_data_cache for PROC_NAME.
> +#
> +# If CACHE_NAME's 'exit' flag is true in gdb_data_cache then we call
> +# gdb_exit and then set a global flag to indicate that gdb_exit should
> +# not be called again for either PROC_NAME or for any caching proc's
> +# that PROC_NAME itself calls.  The list of nested caching procs that
> +# need to be marked is obtained from the 'also_called' key in the
> +# gdb_data_cache for CACHE_NAME.
> +
> +proc gdb_cache_maybe_gdb_exit { proc_name cache_name } {
> +    global gdb_data_cache
> +
> +    # The cache 'exit' entry will be true if this caching proc, or any
> +    # caching proc that is recursively called from this caching proc,
> +    # called exit.
> +    if { !$gdb_data_cache(${cache_name},exit) } {
>  	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
> +    set global_name __${proc_name}__cached_gdb_exit_called
>      if { ![info exists ::${global_name}] } {
>  	gdb_exit
> +	verbose -log "gdb_caching_proc $proc_name caused gdb_exit to be called"
>  	set ::${global_name} true
> +
> +	foreach other_name $gdb_data_cache(${cache_name},also_called) {
> +	    set global_name __${other_name}__cached_gdb_exit_called
> +	    set ::${global_name} true
> +	}
>      }
>  }
>  
> @@ -107,9 +131,10 @@ proc gdb_do_cache {name args} {
>      if {[info exists gdb_data_cache(${cache_name},value)]} {
>  	set cached_value $gdb_data_cache(${cache_name},value)
>  	set cached_exit $gdb_data_cache(${cache_name},exit)
> +	set cached_also_called $gdb_data_cache(${cache_name},also_called)
>  	verbose "$name: returning '$cached_value' from cache" 2
>  	if { $cache_verify == 0 } {
> -	    gdb_cache_maybe_gdb_exit $name $cached_exit
> +	    gdb_cache_maybe_gdb_exit $name $cache_name
>  	    return $cached_value
>  	}
>  	set is_cached 1
> @@ -123,11 +148,13 @@ proc gdb_do_cache {name args} {
>  	    close $fd
>  	    set gdb_data_cache(${cache_name},value) [lindex $content 0]
>  	    set gdb_data_cache(${cache_name},exit) [lindex $content 1]
> +	    set gdb_data_cache(${cache_name},also_called) [lindex $content 2]
>  	    set cached_value $gdb_data_cache(${cache_name},value)
>  	    set cached_exit $gdb_data_cache(${cache_name},exit)
> +	    set cached_also_called $gdb_data_cache(${cache_name},also_called)
>  	    verbose "$name: returning '$cached_value' from file cache" 2
>  	    if { $cache_verify == 0 } {
> -		gdb_cache_maybe_gdb_exit $name $cached_exit
> +		gdb_cache_maybe_gdb_exit $name $cache_name
>  		return $cached_value
>  	    }
>  	    set is_cached 1
> @@ -144,10 +171,16 @@ proc gdb_do_cache {name args} {
>  	trace add execution gdb_exit enter gdb_exit_called
>  	set ::gdb_exit_trace_in_place true
>  	set gdb_exit_trace_created true
> +	set gdb_current_nested_proc_calls {}
>      } else {
>  	set gdb_exit_trace_created false
> +	set gdb_current_nested_proc_calls $::gdb_nested_caching_proc_calls
>      }
>  
> +    # Reset the global list of nested caching procs, this means that
> +    # we are only gathering results for the current call.
> +    set ::gdb_nested_caching_proc_calls {}
> +
>      # As above, we need to consider recursive calls into gdb_do_cache.
>      # Store the old value of gdb_exit_called global and then set the
>      # flag to false.  Initially gdb_exit_called is always false, but
> @@ -167,6 +200,15 @@ proc gdb_do_cache {name args} {
>      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) $::gdb_nested_caching_proc_calls
> +
> +    # We have recorded the list of nested caching procs into the cache
> +    # (above) for CACHE_NAME.  Now we add back the previous value and
> +    # also append the current caching proc's name, this means that if
> +    # this is a nested call then out caller will see the complete list
> +    # of nested caching procs so far.
> +    append ::gdb_nested_caching_proc_calls $gdb_current_nested_proc_calls
> +    lappend ::gdb_nested_caching_proc_calls $name
>  
>      # See comment above where OLD_GDB_EXIT_CALLED is set: if
>      # GDB_EXIT_CALLED was previously true then this is a recursive
> @@ -183,6 +225,7 @@ proc gdb_do_cache {name args} {
>  	trace remove execution gdb_exit enter gdb_exit_called
>  	unset ::gdb_exit_trace_in_place
>  	set ::gdb_exit_called false
> +	set ::gdb_nested_caching_proc_calls {}
>      }
>  
>      # If a value being stored in the cache contains a newline then
> @@ -196,6 +239,7 @@ proc gdb_do_cache {name args} {
>      if { $cache_verify == 1 && $is_cached == 1 } {
>  	set computed_value $gdb_data_cache(${cache_name},value)
>  	set computed_exit $gdb_data_cache(${cache_name},exit)
> +	set computed_also_called $gdb_data_cache(${cache_name},also_called)
>  	if { $cached_value != $computed_value } {
>  	    error [join [list "Inconsistent value results for $cache_name:"
>  			 "cached: $cached_value vs. computed: $computed_value"]]
> @@ -204,6 +248,10 @@ proc gdb_do_cache {name args} {
>  	    error [join [list "Inconsistent exit results for $cache_name:"
>  			 "cached: $cached_exit vs. computed: $computed_exit"]]
>  	}
> +	if { $cached_also_called != $computed_also_called } {
> +	    error [join [list "Inconsistent also_called results for $cache_name:"
> +			 "cached: $cached_also_called vs. computed: $computed_also_called"]]
> +	}
>      }
>  
>      if {[info exists GDB_PARALLEL]} {
> @@ -213,10 +261,11 @@ proc gdb_do_cache {name args} {
>  	set fd [open $cache_filename.[pid] w]
>  	puts $fd $gdb_data_cache(${cache_name},value)
>  	puts $fd $gdb_data_cache(${cache_name},exit)
> +	puts $fd $gdb_data_cache(${cache_name},also_called)
>  	close $fd
>  	file rename -force -- $cache_filename.[pid] $cache_filename
>      }
> -    gdb_cache_maybe_gdb_exit $name $gdb_data_cache(${cache_name},exit)
> +    gdb_cache_maybe_gdb_exit $name $cache_name
>      return $gdb_data_cache(${cache_name},value)
>  }
>  
> 

Thanks for the patch.

On a first try (parallel run), I still see the errors with gdb_exit being called, but I'm trying to
see if they go away in a serialized run, as it seemed to work. I'll report back when the tests are done
running.

Reading through the code, things make sense overall. But there are a few levels to it and it can get a bit
complicated. So I'm taking my time to understand it.