From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4OTOJYoHpmbJYDQAWB0awg (envelope-from ) for ; Sun, 28 Jul 2024 04:55:38 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=V/bnqma7; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 94E8F1E0D0; Sun, 28 Jul 2024 04:55:38 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 7421E1E030 for ; Sun, 28 Jul 2024 04:55:36 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 02D5B385C6CE for ; Sun, 28 Jul 2024 08:55:36 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 6170C3858420 for ; Sun, 28 Jul 2024 08:55:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6170C3858420 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 6170C3858420 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=1722156914; cv=none; b=jhBTapF9o7BSs73E7XEDEYUc+L8CpZF+ncvwm77lc6tNBD7AU4dsVkxmXD72i4avqF0E9fbZkQ9dQ2fdJuR49qwi6BGwvrQ8yNQ5Mx4BBWRVS709cSs/XxA/Nflz4iD61oL1aWtqly4+l/hrGqXwsu77ZbBHdXKgqTn1NmXb66s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1722156914; c=relaxed/simple; bh=CHjAZRTdziS8Yn29ppIb8NypGQUbI9Gt/lCp7730Fvs=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=fuZUqQl2xArL1AiB0NjA+ri/AXVCV+XGIKYIo3ceQ6YN7Y2EoaCBA4HpIBl3QSSOUG1pp1j0ffvW6mymBYpd3PUUVym82aV5OxaJv4zAA6+w6hx5NNovPZi+t1kEUPN1jzUb0xpmYwaCjGq4Zr1pmasj3Z1x7HkJ7heAgujYais= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1722156910; 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=BwzOdMst5zv7b0twN//0K1dPdZIBHvdylQ9hp/wseTI=; b=V/bnqma7BtrToH55xY416neW06qrk+p6aabSvDEl2879WC0dA5McmIlsroXgEQFYOV5fxw qxT46QTGN9tbbNAfvjWTKorBjltyfyDEQEfBJWCpC0lkEqbg7lfelMBSzAFuhkkCn8OHa1 JLJwSTuOGcyVsdaBNrLEM30Ms6dQ66Y= 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-209-DYG3wnhNPjipV41J-GWQAg-1; Sun, 28 Jul 2024 04:55:08 -0400 X-MC-Unique: DYG3wnhNPjipV41J-GWQAg-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-36835f6ebdcso980427f8f.1 for ; Sun, 28 Jul 2024 01:55:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722156906; x=1722761706; 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=BwzOdMst5zv7b0twN//0K1dPdZIBHvdylQ9hp/wseTI=; b=aH1pleLWLsy7ueJSRGOGw5CHAAMaR4I3068F84RkrGYrBf6Ce4fLLTVzkSS48+8YTe 4HnZOq8MtCzNn209RFJQx1ekZUK3Cm/TKq7MZWX6pwIUxPHyYNWXLURsoUL876ICBbfM og8rGJ6is2XaovuwDhurIRYpnqcWx6mgdcDsvcA5dnepMkiDfEFxAv4Xm+tV+fH11ucK i3Avkvdft4OYzjDTsfi6DPAa0uUhGabviAZUG3Dw7B8437d/f923q4dJ1wRyuv58mahU KZ8PZTr7anKHYaQ1ZFvhwUBvXFBtM3FPPBXUDHxRihyWGESMWzREAPNkJoHpcEiK69AG L3Qg== X-Gm-Message-State: AOJu0YwXEZjTfQlUOTqLzxeNnzpSqRHAxMpQmJ7yQnFxeuPbJ6AZqAce oT9WeYo+ACj8IjG509bvXKI9+KX3qVXk2fbF0DAR9bH7SEaLJ8jwNAniqtyS4tj/jOCxTPQnyXi pKqzMo+2/iLqpAwhDGwN59PROPyBh57+HIS0BDUaXUV23nSOJlQ90P0EDmaIJKJVLQRoWRTk4+A tG+SOX14Xm4GIMmDDE4XSFp6roW59q9gQJJuNspq4xnkQ= X-Received: by 2002:a5d:598b:0:b0:368:7e26:38d1 with SMTP id ffacd0b85a97d-36b5cee21fbmr3874844f8f.21.1722156906567; Sun, 28 Jul 2024 01:55:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEZTM/YvGlPWtTAT/RpMvHBaZ0FK6bq2JbME0h8JopTIZGVD/D80t20cd/VNmaYlanmWnS2nw== X-Received: by 2002:a5d:598b:0:b0:368:7e26:38d1 with SMTP id ffacd0b85a97d-36b5cee21fbmr3874827f8f.21.1722156906014; Sun, 28 Jul 2024 01:55:06 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36b367c028bsm9317974f8f.24.2024.07.28.01.55.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 28 Jul 2024 01:55:04 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PUSHED 2/2] gdb/testsuite: track if a caching proc calls gdb_exit or not Date: Sun, 28 Jul 2024 09:54:58 +0100 Message-Id: <0726729d344fecf98f8d138e688e77201cc3cece.1722156763.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=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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 e7b9114058b..092b7f136e8 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 e5cacefeb13..b3e1f306a29 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. -- 2.25.4