From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id /aC1EVbbtGbYRwAAWB0awg (envelope-from ) for ; Thu, 08 Aug 2024 10:51:02 -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=RuaEOKX6; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 31C811E0D0; Thu, 8 Aug 2024 10:51:02 -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 0B7D11E08C for ; Thu, 8 Aug 2024 10:51:00 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 843353860C37 for ; Thu, 8 Aug 2024 14:50:59 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 63DD6385E459 for ; Thu, 8 Aug 2024 14:50:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 63DD6385E459 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 63DD6385E459 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723128636; cv=none; b=ECKCLPbDv0X9JMpHFY+N2A0XjAE9KsI00mBoPZw+iPn8AEzqXed0Pzb7sY99R72zvUNWNrFvCloRj+HdZfa8hnAa8WaSoA6SnBcPc/DD72lmSyCWBG42czHr3giS38pUG3AtftgrT/A9GokBDwUAe8RiKEnWptmuUnkEkAjUPYY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723128636; c=relaxed/simple; bh=nLH0RyIzMDpZNcFYYtr/4gZe5lRN1EzAAGFL9ae+Tx4=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=oUzGePmG+mQKHpg9o/QKvm7550vYWdVvmM2z9su+HIeRXdzS8tvghP3ALmLdQuEFwCyg0FCnOrMahUqHjSBxPTZsej0KTpPnI7czXXgFVQ54swO/o7UWMJB2phGNWQnkSv3sUg4VCDZa22uGzPQ6xgsQ9ps89j7ymWOYeEHEyf8= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723128632; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=mulW59sHpslwGeGOH+0KYpooaSCTPUbe9O8ccjxiW2c=; b=RuaEOKX6wSBe5YLXMfBNs+5OXr1roY6Q5yXuu1VIuxnN2rZUAFuzJFPPqmTiaJTdQV6E5Q VWea0ud//R4GLOq0HuUT56BxGc3JZLonqZQ5/3BNuRDTLmdC59p6DDYSGMj4G1fUmjToEy z1pfsMzz9BvsyI1WEzlSALjIIb3RViY= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-220-sO0VpJsHM_erTS-4gAa3Lw-1; Thu, 08 Aug 2024 10:50:30 -0400 X-MC-Unique: sO0VpJsHM_erTS-4gAa3Lw-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4281b7196bbso8158865e9.0 for ; Thu, 08 Aug 2024 07:50:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723128629; x=1723733429; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=mulW59sHpslwGeGOH+0KYpooaSCTPUbe9O8ccjxiW2c=; b=HB2NcWOADx7nhqDjq4EUX/U1HuobLciHi0jpyUqYg1NRDsU/Z40VtXsLQEncKhk/yw iepTNZRsbEwO6uqU+KKS0pGwFF3wPSGHr/gc/U3wNmkN3j5Xxf/sDXRABFFO9AaeR3Hi ZwtJ7+4JgbN+bn/mzUH2HoWGJK8WEK+hGuF9U13eUHVlwWpLUAm6qJdNMLjVCSP/agwB 42YAL5egxpDEFtOQQSmFbuGqo8B4kV4xgkqOLkAsvW3xb+a+PAyQdOYfhA6bT2rATokD J35OiZ3oqx7RuPY36nYmydyTXG/2su5zabxRnFhd7jMamIkfpHIB7+ptVNkU/u2j5DyG kssA== X-Forwarded-Encrypted: i=1; AJvYcCWmwZirVELNgxp5yciOnQHpmUc7WXuxk3Po2592eomEIfS/L7GPoTnSzBVDZIv+l+jfdisVH32qlWUpvEoF3mvbnKB69ocvxswwjg== X-Gm-Message-State: AOJu0YzqHRmsMAsjMk64rs61vgu9LIaXUWC9J98a2VjeJSp1m17ANbvG WhybInOuQ68kDxV0CD3t3v+GOZIXgP/+PcH/KgdxjM72lVg/UQBaWu2a8vdpQDXOP9U6KHjgvru C4gXFxsaXR3P0aSZAWsaCsbN9+CX5vs3MsEkxrO40ASfvg1OQd8fPAT00BJ6dn2RpPJk= X-Received: by 2002:a5d:494c:0:b0:367:9988:84a0 with SMTP id ffacd0b85a97d-36d27577977mr1494621f8f.58.1723128628578; Thu, 08 Aug 2024 07:50:28 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFXuBVRjIsFooz+GEMwaqIzPOGRT+u1RrwfVoHceRmRHP7OZaRI1lmZh1ALu7BXaSKSd5kf0g== X-Received: by 2002:a5d:494c:0:b0:367:9988:84a0 with SMTP id ffacd0b85a97d-36d27577977mr1494597f8f.58.1723128627773; Thu, 08 Aug 2024 07:50:27 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36d27229121sm2143625f8f.101.2024.08.08.07.50.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Aug 2024 07:50:27 -0700 (PDT) From: Andrew Burgess To: Luis Machado , gdb-patches@sourceware.org Subject: Re: [PATCH 4/4] gdb/testsuite: track if a caching proc calls gdb_exit or not In-Reply-To: <865dd933-bbfe-44d9-94e2-b7e133b05ed5@arm.com> References: <5dc846ffb6cd8f76ba2769ee7679f5d1b01fae0a.1717438458.git.aburgess@redhat.com> <97973506-79f4-4216-9c0b-57401b3933f5@arm.com> <878qx8z9nt.fsf@redhat.com> <9fbc6f52-bc2f-43c8-80b0-3f4c495df76e@arm.com> <8f70328b-8a35-463f-b153-25c0b63956d7@arm.com> <865dd933-bbfe-44d9-94e2-b7e133b05ed5@arm.com> Date: Thu, 08 Aug 2024 15:50:26 +0100 Message-ID: <875xsbysp9.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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_H4, 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 Luis Machado 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 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... Thanks, Andrew > > ERROR: no fileid for ubuntu > Couldn't send delete breakpoints to GDB. > UNRESOLVED: gdb.arch/aarch64-sme-core-2.exp: state=ssve vl=16 svl=16: delete all breakpoints, watchpoints, tracepoints, and catchpoints in delete_breakpoints > ERROR: breakpoints not deleted > ERROR: no fileid for ubuntu > Couldn't send break -qualified main to GDB. > UNRESOLVED: gdb.arch/aarch64-sme-core-2.exp: state=ssve vl=16 svl=16: gdb_breakpoint: set breakpoint at main > ERROR: no fileid for ubuntu > UNRESOLVED: gdb.arch/aarch64-sme-core-2.exp: state=ssve vl=16 svl=16: runto: run to main (timeout) > UNTESTED: gdb.arch/aarch64-sme-core-2.exp: state=ssve vl=16 svl=16: could not run to main > testcase builds/binutils-gdb/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch/aarch64-sme-core-2.exp completed in 73 seconds > > ---