gdb/testsuite: track if a caching proc calls gdb_exit or not

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 the user calls 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 return the cached value, but will
instead compute the cached value, a process that ends up calling
gdb_exit.  When can_spawn_for_attach_1 returns GDB will have exited
and the test might fail if it is written assuming that GDB is
running.

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 script run, gdb_exit will still be called.  This
ensures consistent behaviour and avoids some hidden bugs in the
testsuite.

The split between can_spawn_for_attach and can_spawn_for_attach_1 was
introduced in this commit:

  commit 147fe7f9fb9a89b217d11d73053f53e8edacf90f
  Date:   Mon May 6 14:27:09 2024 +0200

      [gdb/testsuite] Handle ptrace operation not permitted in can_spawn_for_attach

However, I observe 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 track calls to
    gdb_exit.  If a caching proc calls gdb_exit then this information
    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.

To track calls to gdb_exit I eventually settled on using TCL's trace
mechanism.  We already make use of this in lib/gdb.exp so I figure
this is OK to use.  This should be fine, so long as non of the caching
procs use 'with_override' to replace gdb_exit, or do any other proc
replacement to change gdb_exit, however, I think that is pretty
unlikely.

One issue did come up in testing, a FAIL in gdb.base/break-interp.exp,
prior to this commit can_spawn_for_attach would call gdb_exit before
calling the underlying caching proc.  After this call we call gdb_exit
after calling the caching proc.

The underlying caching proc relies on gdb_exit having been called.  To
resolve this issue I just added a call to gdb_exit into
can_spawn_for_attach.

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.

2 files changed, 135 insertions, 74 deletions

diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp
index e7b9114..092b7f1 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 gdb_exit_called proc.  Is set to true to
+# indicate that a caching proc has called gdb_exit.
+
+set gdb_exit_called false
+
+# This proc is called via TCL's trace mechanism whenever gdb_exit is
+# called during the execution of a caching proc.  This sets the global
+# flag to indicate that gdb_exit has been called.
+
+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 } {
+	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,90 @@ 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
 	}
     }
 
+    # Add a trace hook to gdb_exit.  In the case of recursive calls to
+    # gdb_do_cache we only want to install the trace hook once, so we
+    # set a global to indicate that the trace is in place.
+    #
+    # We also set a local flag to indicate that this is the scope in
+    # which the debug trace needs to be removed.
+    if { ![info exists ::gdb_exit_trace_in_place] } {
+	trace add execution gdb_exit enter gdb_exit_called
+	set ::gdb_exit_trace_in_place true
+	set gdb_exit_trace_created true
+    } else {
+	set gdb_exit_trace_created false
+    }
+
+    # 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
+    # for recursive calls to gdb_do_cache we can't know the state of
+    # gdb_exit_called.
+    #
+    # Before starting a recursive gdb_do_cache call we need
+    # gdb_exit_called to be false, that way the inner call can know if
+    # it invoked gdb_exit or not.
+    #
+    # Once the recursive call completes, if it did call gdb_exit then
+    # the outer, parent call to gdb_do_cache should also be considered
+    # as having called gdb_exit.
+    set old_gdb_exit_called $::gdb_exit_called
+    set ::gdb_exit_called false
+
     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
+
+    # See comment above where OLD_GDB_EXIT_CALLED is set: if
+    # GDB_EXIT_CALLED was previously true then this is a recursive
+    # call and the outer caching proc set the global true, so restore
+    # the true value now.
+    if { $old_gdb_exit_called } {
+	set ::gdb_exit_called true
+    }
+
+    # See comment above where GDB_EXIT_TRACE_CREATED is set: this is
+    # the frame in which the trace was installed.  This must be the
+    # outer caching proc call (if an recursion occurred).
+    if { $gdb_exit_trace_created } {
+	trace remove execution gdb_exit enter gdb_exit_called
+	unset ::gdb_exit_trace_in_place
+	set ::gdb_exit_called false
+    }
+
+    # 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 +212,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 e5cacef..b3e1f30 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6321,14 +6321,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
     }
 
@@ -6353,6 +6362,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]
@@ -6374,61 +6386,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.