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

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

On 8/13/24 17:30, Andrew Burgess wrote:
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 8/8/24 15:50, Andrew Burgess wrote:
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> On 8/8/24 11:50, Luis Machado wrote:
>>>>> On 8/8/24 11:20, Luis Machado wrote:
>>>>>> 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
>>>>>>>
>>>>>>
>>>>>> Well, it's one of those things I guess. I saw some errors the first time I tried the patch, but then
>>>>>> I couldn't reproduce it anymore. So far it's been running pretty smoothly for both parallel and
>>>>>> serialized runs. So I'd say this patch does the job and we should push it.
>>>>>> Thanks for putting it together.
>>>>>>
>>>>>> I'll do a complete run overnight just to make sure, but it will take a little bit before I can report
>>>>>> it.
>>>>>
>>>>> Of course, a short while after sending this, I managed to reproduce the error.
>>>>>
>>>>> I'm running the following:
>>>>>
>>>>> make check-gdb TESTS=gdb.arch/*.exp -j$(nproc). Let me fetch some more information.
>>>>
>>>> It seems we're hitting the same situation with aarch64_initialize_sve_information via aarch64_supports_sve_vl,
>>>> which causes the testsuite to call gdb_exit.
>>>>
>>>> It is as you described, it is the first time we're running gdb.arch/aarch64-sme-core-2.exp, so we go through
>>>> caching etc.
>>>>
>>>> ---
>>>>
>>>> Running builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-core-2.exp ...
>>>> gdb_caching_proc allow_aarch64_sve_tests caused gdb_exit to be called
>>>> stack trace is Stack trace:
>>>>  gdb_cache_maybe_gdb_exit proc_name='allow_aarch64_sve_tests' cache_name='unix/allow_aarch64_sve_tests'
>>>>   gdb_do_cache name='allow_aarch64_sve_tests' args=''
>>>>    allow_aarch64_sve_tests
>>>
>>> This makes sense assuming that this is not the first time
>>> `allow_aarch64_sve_tests` was called in this test run.  This looks like
>>> its using the previously cached result.
>>>
>>> If all had gone as expected then this should have marked `gdb_exit` as
>>> having been called for both `allow_aarch64_sve_tests` and
>>> `aarch64_initialize_sve_information`, though given what happens below I
>>> guess that the marker for `aarch64_initialize_sve_information` isn't
>>> being created correctly.
>>>
>>> You could try applying this patch:
>>>
>>> ### START ###
>>>
>>> diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
>>> index 7e1eae9259e..d6027b352f1 100644
>>> --- a/gdb/testsuite/lib/cache.exp
>>> +++ b/gdb/testsuite/lib/cache.exp
>>> @@ -98,6 +98,7 @@ proc gdb_cache_maybe_gdb_exit { proc_name cache_name } {
>>>  	set ::${global_name} true
>>>  
>>>  	foreach other_name $gdb_data_cache(${cache_name},also_called) {
>>> +	    verbose -log "  gdb_caching_proc $other_name also called exit"
>>>  	    set global_name __${other_name}__cached_gdb_exit_called
>>>  	    set ::${global_name} true
>>>  	}
>>> @@ -110,6 +111,8 @@ proc gdb_do_cache {name args} {
>>>      global gdb_data_cache objdir
>>>      global GDB_PARALLEL
>>>  
>>> +    verbose -log "gdb_do_cache: $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
>>>      # verify against the cached value.
>>>
>>> ### END ###
>>>
>>> which will log the "other" caching procs that are recorded as having
>>> been called.
>>>
>>> Also, if you are running `make -j...." then you can look into the cache
>>> files which will be 'gdb/testsuite/cache/unix/allow_aarch64_sve_tests'
>>> and 'gdb/testsuite/cache/unix/aarch64_initialize_sve_information' as
>>> this should also include the information about the nested caching proc
>>> structure.
>>>
>>>>
>>>> gdb_caching_proc allow_aarch64_sme_tests caused gdb_exit to be called
>>>> stack trace is Stack trace:
>>>>  gdb_cache_maybe_gdb_exit proc_name='allow_aarch64_sme_tests' cache_name='unix/allow_aarch64_sme_tests'
>>>>   gdb_do_cache name='allow_aarch64_sme_tests' args=''
>>>>    allow_aarch64_sme_tests
>>>>
>>>> get_compiler_info: gcc-13-2-0
>>>> Executing on host: gcc  -fno-stack-protector  -fdiagnostics-color=never -g3 -march=armv8.5-a+sve -c -g  -o builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-20.o builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-core.c    (timeout = 300)
>>>> builtin_spawn -ignore SIGHUP gcc -fno-stack-protector -fdiagnostics-color=never -g3 -march=armv8.5-a+sve -c -g -o builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-20.o builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-core.c
>>>> Executing on host: gcc  -fno-stack-protector builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-20.o  -fdiagnostics-color=never -g3 -march=armv8.5-a+sve -g  -lm   -o builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-2    (timeout = 300)
>>>> builtin_spawn -ignore SIGHUP gcc -fno-stack-protector builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-20.o -fdiagnostics-color=never -g3 -march=armv8.5-a+sve -g -lm -o builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-2
>>>> builtin_spawn builds/binutils-gdb/gdb/testsuite/../../gdb/gdb -nw -nx -q -iex set height 0 -iex set width 0 -data-directory builds/binutils-gdb/gdb/data-directory
>>>> (gdb) set height 0
>>>> (gdb) set width 0
>>>> (gdb) dir
>>>> Reinitialize source path to empty? (y or n) y
>>>> Source directories searched: $cdir:$cwd
>>>> (gdb) dir builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch
>>>> Source directories searched: builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch:$cdir:$cwd
>>>> (gdb) kill
>>>> The program is not being run.
>>>> (gdb) file builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-2
>>>> Reading symbols from builds/binutils-gdb/gdb/testsuite/outputs/gdb.arch/aarch64-sme-core-2/aarch64-sme-core-2...
>>>> (gdb) gdb_caching_proc aarch64_initialize_sve_information caused gdb_exit to be called
>>>> stack trace is Stack trace:
>>>>  gdb_cache_maybe_gdb_exit proc_name='aarch64_initialize_sve_information' cache_name='unix/aarch64_initialize_sve_information'
>>>>   gdb_do_cache name='aarch64_initialize_sve_information' args=''
>>>>    aarch64_initialize_sve_information
>>>>     aarch64_supports_sve_vl length='16'
>>>>      test_sme_core_file id_start='50' id_end='74'
>>>
>>> This should have been prevented by the earlier allow_aarch64_sve_tests
>>> call.  Either we failed to correctly spot the relationship between the
>>> two caching procs, or we're failing to mark this second one as having
>>> been called for some reason...
>>>
>>> I think we need more data to try and debug this...
>>
>> I did some more debugging on this, and I think I see what's going on here. I haven't
>> checked the flow in detail yet, but hopefully it will ring some bells.
>>
>> My theory is that your patch's logic is sane, but we're hitting concurrency issues when
>> running things in parallel, or parallel enough. For instance, I consistently hit issues when
>> using -j4, but I can't hit it with anything below -j4.
>>
>> I added a couple debugging statements in gdb_do_cache, at the end within the if {[info exists GDB_PARALLEL]} block.
>>
>> The statements check if the $cache_filename exists and what the contents are, before we rename
>> $cache_filename.[pid] to $cache_filename, essentially overwriting the old cache file.
>>
>> What I saw was the following:
>>
>> ---
>>
>> gdb_do_cache: aarch64_initialize_sve_information (  )
>> gdb_caching_proc aarch64_initialize_sve_information caused gdb_exit to be called
>> Stack trace:
>>  gdb_cache_maybe_gdb_exit proc_name='aarch64_initialize_sve_information' cache_name='unix/aarch64_initialize_sve_information'
>>   gdb_do_cache name='aarch64_initialize_sve_information' args=''
>>    aarch64_initialize_sve_information
>>     gdb_real__allow_aarch64_sve_tests
>>      allow_aarch64_sve_tests
>>
>> XXXX: File builds/binutils-gdb/gdb/testsuite/cache/unix/allow_aarch64_sve_tests already exists before rename.
>> XXXX: Old contents: 1 true aarch64_initialize_sve_information
>> XXXX: New contents: 1 true {}
>> gdb_caching_proc allow_aarch64_sve_tests caused gdb_exit to be called
>> Stack trace:
>>  gdb_cache_maybe_gdb_exit proc_name='allow_aarch64_sve_tests' cache_name='unix/allow_aarch64_sve_tests'
>>   gdb_do_cache name='allow_aarch64_sve_tests' args=''
>>    allow_aarch64_sve_tests
>>
>> ---
>>
>> So we replace a cache file that contains more information with a copy that has less information.
>>
>> Eventually we call aarch64_initialize_sve_information again, via a different function, and things go bad.
>>
>> I'm guessing the logic of updating the cache files needs to be atomic, and we're having some timing issues
>> where two (or more) tests try to update the same file and we end up losing information.
> 
> Thanks for doing the leg work on this.  The information you provided is
> spot on, and it was indeed a timing issue as you predicted.  Here's
> what's happening:
> 
> In thread #1 we call 'allow_aarch64_sme_tests', there's no cache file
> yet we eval the body, this calls 'aarch64_initialize_sme_information'.
> So we try to create two cache files in this order:
> 
>   unix/aarch64_initialize_sme_information
>     value: ... whatever ...
>     exit_called?: true
>     also_called: {}
> 
>   unix/allow_aarch64_sme_tests
>     value: ... whatever ...
>     exit_called?: true
>     also_called: aarch64_initialize_sme_information
> 
> In thread #2 we also call 'allow_aarch64_sme_tests', however, if we
> manage to land after the first cache file was created but before the
> second file was created then we pick up the cached result for
> 'aarch64_initialize_sme_information'.
> 
> And this is where my bug was: I didn't add
> 'aarch64_initialize_sme_information' to the also_called list for a
> caching proc if we managed to find a cached value.  As a result, the
> thread #2 cache file looked like this:
> 
>   unix/allow_aarch64_sme_tests
>     value: ... whatever ...
>     exit_called?: true
>     also_called: {}
> 
> which then overwrote the first cache file (atomically).
> 
> A later test would call 'allow_aarch64_sme_tests' and then separately
> call 'aarch64_initialize_sme_information'.  Due to the corrupted cache
> file calling 'allow_aarch64_sme_tests' would not mark
> 'aarch64_initialize_sme_information' as having been called.  So when we
> separately call 'aarch64_initialize_sme_information' gdb_exit would be
> called which broke the test.
> 
> The fix, of course, is to ensure that when we get a cache hit we still
> record the called function as being something that is "also called".
> 
> I have a new patch below (discard the previous patch I sent) which I
> think should resole the problems you're having.  I'm still running the
> full tests on my end, but initial (limited) testing looks good.

Great. Glad the information was useful. I gave the attached patch a try and
managed to run all the sme tests in parallel and did not see any FAIL's or
errors.

I'm doing a few more runs just in case, but I think we're good.

> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 7384f334613a548bc26eaf238650076537c9b8fb
> 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.  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 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..ca3dca22f96 100644
> --- a/gdb/testsuite/lib/cache.exp
> +++ b/gdb/testsuite/lib/cache.exp
> @@ -59,13 +59,38 @@ 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.
> +# 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.
>  
> -proc gdb_cache_maybe_gdb_exit { name do_exit } {
> +set gdb_nested_caching_proc_calls {}
> +
> +# Called before returning from gdb_do_cache.  NAME is the name of the
> +# caching proc that was called.
> +#
> +# When DO_EXIT is true then this proc should call gdb_exit before
> +# returning, otherwise gdb_exit is not called.
> +#
> +# ALSO_CALLED is a list of the names all the nested caching procs that
> +# the proc NAME called.  This proc appends NAME as well as everything
> +# in ALSO_CALLED to the global GDB_NESTED_CACHING_PROC_CALLS, this
> +# aids in tracking recursive caching proc calls.
> +
> +proc gdb_cache_maybe_gdb_exit { name do_exit also_called } {
> +
> +    # Record all the procs that have been called, but only if the exit
> +    # trace is in place (this is done in gdb_do_cache) and indicates
> +    # that we are in data gathering mode.
> +    if { [info exists ::gdb_exit_trace_in_place] } {
> +	set ::gdb_nested_caching_proc_calls \
> +	    [list {*}$::gdb_nested_caching_proc_calls $name {*}$also_called]
> +    }
> +
> +    # 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 { !$do_exit } {
>  	return
>      }
> @@ -76,7 +101,15 @@ proc gdb_cache_maybe_gdb_exit { name do_exit } {
>      set global_name __${name}__cached_gdb_exit_called
>      if { ![info exists ::${global_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"
> +
> +	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
> +	}
>      }
>  }
>  
> @@ -86,6 +119,8 @@ proc gdb_do_cache {name args} {
>      global gdb_data_cache objdir
>      global GDB_PARALLEL
>  
> +    verbose -log "gdb_do_cache: $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
>      # verify against the cached value.
> @@ -107,9 +142,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 $cached_exit $cached_also_called
>  	    return $cached_value
>  	}
>  	set is_cached 1
> @@ -123,11 +159,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 $cached_exit $cached_also_called
>  		return $cached_value
>  	    }
>  	    set is_cached 1
> @@ -164,9 +202,29 @@ proc gdb_do_cache {name args} {
>      set old_gdb_exit_called $::gdb_exit_called
>      set ::gdb_exit_called false
>  
> +    # As with the exit tracking above we also need to track any nested
> +    # caching procs that this proc might call.  To do this we backup
> +    # the current global list of nested caching proc calls and reset
> +    # the global back ot the empty list.  As nested caching procs are
> +    # called their names are added to the global list, see
> +    # gdb_cache_maybe_gdb_exit for where this is done.
> +    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) \
> +	[lsort -unique $::gdb_nested_caching_proc_calls]
> +
> +    # Now that the actual implementation of this caching proc has been
> +    # executed the gdb_nested_caching_proc_calls global will contain
> +    # the names of any nested caching procs that were called.  We
> +    # append this new set of names onto the set of names we backed up
> +    # above.
> +    set ::gdb_nested_caching_proc_calls \
> +	[list {*}$old_gdb_nested_caching_proc_calls \
> +	     {*}$::gdb_nested_caching_proc_calls]
>  
>      # See comment above where OLD_GDB_EXIT_CALLED is set: if
>      # GDB_EXIT_CALLED was previously true then this is a recursive
> @@ -183,6 +241,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 +255,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 +264,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 +277,12 @@ 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 $gdb_data_cache(${cache_name},exit) \
> +	$gdb_data_cache(${cache_name},also_called)
>      return $gdb_data_cache(${cache_name},value)
>  }
>  
>