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

[Andrew Burgess <aburgess@redhat.com>]

Luis Machado <luis.machado@arm.com> writes:

> On 8/14/24 18:00, Andrew Burgess wrote:
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> 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.
>>>
>> 
>> That's great news.  If I don't hear anything in a couple of days then
>> I'll push this fix.
>
> The additional testing finished OK. So this is good. Thanks for putting it together.
>
> Approved-By: Luis Machado <luis.machado@arm.com>
> Tested-By: Luis Machado <luis.machado@arm.com>

Thanks Luis, I've gone ahead and pushed this patch.

Thanks,
Andrew