From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id D66B2382AA9C for ; Mon, 3 Jun 2024 18:17:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D66B2382AA9C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D66B2382AA9C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717438635; cv=none; b=fPDpwPh2Kmk0GllQ4+t6RRGznpuhmZWvsKvgIEvS4gQL/O2n7IJsk1q8rhBAbtzbiFbhIYedfJe18DVlK4rZfN7HvoB3zP9uXzlGPquMqUMuUUZyRT/xidpC6v8WXo0grsI3XmXFs9c/QzMhO2lLd4mp0NNM3yREK1QcgI/xenA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717438635; c=relaxed/simple; bh=YIhm4kqCgeGe1d+hFh+DmCas6d8ThoFMKzvMD12CmBs=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=hmr5OMueVr0JrlM5dFMZoKYJ3wq+UnrwI+wajVkIyvQ9jrjNuzDVZJN5Jz9nvv5498d/EGZE4uA5YXdbc38zNeOc5WIrl3e64OX+uUZOhVEHJd4WtZB+dwHJZIc2+7/KKXDoHxbPppL2GnyXbPxKahS7DhbbXk/jSJKxV7moagU= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1717438632; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rZjWx1x+7lb+Hy3vFTQiug0JTjSm9OWsfv6s6XE9I1o=; b=CuqE6TfR2xdkRVj2HdK+jyIngVFpNxwKNNBUvNb0/4E+ZVXaYRfgdqujGxdYbAA8krhV/g N0VYCo+3Pyq94hO92nrM8hxHv7QeDMuSJU1bwX4SZb2/3HptNugWP6RCIVi8vtKlMWCzK2 6Wkwo1ZoJR23AANG9+GCzb3LfIXt3Eo= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-47-3F_OcFoMPVy4ceLvHKpbIQ-1; Mon, 03 Jun 2024 14:17:11 -0400 X-MC-Unique: 3F_OcFoMPVy4ceLvHKpbIQ-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-35dbf83bb20so2728294f8f.2 for ; Mon, 03 Jun 2024 11:17:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717438628; x=1718043428; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=rZjWx1x+7lb+Hy3vFTQiug0JTjSm9OWsfv6s6XE9I1o=; b=e6TcW3z5JKjW/wSN5OPYhw59y8rj33D64aL42dgMnkn64/sf3ajINkQWliO9iNT8mt WFJ8VVhvcOyYKpp1UwlCebz0pGga0RrNaktFFhuJUCaekpX8mlTIL34qNu8Gqf04y79+ ccFt3c7yxTSSpmtV1gAlmEccrT+EbujQvQlb+GdQShI4+Y+vjyuRGLP9KyvaZJIgZlkb v4Cc2ssg1aKIcNl79HH0IbYKneF8xxlDO5XwaRDDqrmaIzTF6ZBwWD99mNsPBQNzr38J F0UoQ58B9L5+Z/tVompxVSb4YXgk9j2DjV+HLh5evPGQhTTm0mghm3PJvJADysfiQe/j 3UHQ== X-Gm-Message-State: AOJu0YxIKjBkMueZkMTxnJl7GONGyrotzPrXgid0yyhZP482BGYt5yDn KdOn7iJnX2GEksuTH9M5C8/cppy5Y7Unwii5hELzl0gyQL4r1k65yC0yNVenlGhSzJekRrjWZAP 8HbcC7CYiX15SAE3h/8C+NmM+WQeuw2p1ks5Pm7ALYyipRJru3Ykkop2CGLaF2G2hyLkZdVaR3x yhRD+WPs0EOXTIrIwVOh5uCCoa9CgoujibSaK+USgnA/M= X-Received: by 2002:adf:f64f:0:b0:35e:5de4:b8e7 with SMTP id ffacd0b85a97d-35e5de4b9cfmr2216577f8f.46.1717438628277; Mon, 03 Jun 2024 11:17:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEt7gmx90W0tpt5TmMQL2MLln4Ln8dCkoXibHqTauI8ADVvlaTgK+AqbmgjoSR2INOrbqompw== X-Received: by 2002:adf:f64f:0:b0:35e:5de4:b8e7 with SMTP id ffacd0b85a97d-35e5de4b9cfmr2216542f8f.46.1717438627400; Mon, 03 Jun 2024 11:17:07 -0700 (PDT) Received: from localhost (92.40.185.136.threembb.co.uk. [92.40.185.136]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-35dd04da182sm9346739f8f.54.2024.06.03.11.17.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Jun 2024 11:17:07 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 4/4] gdb/testsuite: track if a caching proc calls gdb_exit or not Date: Mon, 3 Jun 2024 19:16:55 +0100 Message-Id: <5dc846ffb6cd8f76ba2769ee7679f5d1b01fae0a.1717438458.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_ABUSEAT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. -- 2.25.4