From 0726729d344fecf98f8d138e688e77201cc3cece Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Mon, 3 Jun 2024 13:56:54 +0100 Subject: 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. --- gdb/testsuite/lib/cache.exp | 126 ++++++++++++++++++++++++++++++++++++++++---- gdb/testsuite/lib/gdb.exp | 83 +++++++---------------------- 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. -- cgit v1.1