From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id aukdAiXjvGb0GwcAWB0awg (envelope-from ) for ; Wed, 14 Aug 2024 13:02:29 -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=W2gppjnN; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D58331E0D0; Wed, 14 Aug 2024 13:02:28 -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 B5DC01E08C for ; Wed, 14 Aug 2024 13:02:26 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 706FE3858D3C for ; Wed, 14 Aug 2024 17:02:26 +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 CD2EA3858D34 for ; Wed, 14 Aug 2024 17:00:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CD2EA3858D34 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 CD2EA3858D34 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=1723654840; cv=none; b=rTNiHwp40ws9IYp4SbQJaaJKRNmlQhIN8m4kmj4ORqVZ8+1tN8PCqwtUz+2/0m/bpARs95ZC3/Qb+CBSMiY3Atn2pDOm4l5pzslqcYvAGyCA4xyi6jlIkyXS6KsNSgXouXbBRWqcfGqW8YuSD/HYsQaIeQzuG+Z+FX5cCH2HYSA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723654840; c=relaxed/simple; bh=fs+TDATbCy6HHJwdxdgANEwKvbp+hpgWgYHRhRj6ND8=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=fMe2oKo55K2+FWl4LAFZRuHtNOxbrqcks9mdS4kPHFwyLjVCB1Jn0rvKOBeoWi5ASALF6olrJb73fuGXAhdKSgihkDDU12g6XCDT3dVPGjXV/ixuXm85MOgKa4Q5IDJZD+mS7WKnLXKqT6uEV+cs5LvT4zVrniCJP6zfbUJ2EIM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723654834; 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=Etrj/Ffni87IphTgH1+IKPYSQ/8T5KhvJKTjhUmUnKU=; b=W2gppjnNcqg+7lOkMgMUbTd4dnlWNi7UKkjGw4d0yg2mkvNUsMuEeh/thUe5zBy/6idHfK NG4vThRk8NYzGZI0yutqPi6F/JVHpsHPm+jFdikbx7eA1G+v1rKFuh3x9uM/op9prbtKU8 BIKJPxqPYWITMos9BareI0MXlxOSTE0= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-7--F9nBDlyNVy10eTp2p4XiA-1; Wed, 14 Aug 2024 13:00:33 -0400 X-MC-Unique: -F9nBDlyNVy10eTp2p4XiA-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4281310bf7aso81125e9.1 for ; Wed, 14 Aug 2024 10:00:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723654832; x=1724259632; 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=Etrj/Ffni87IphTgH1+IKPYSQ/8T5KhvJKTjhUmUnKU=; b=R1lRLHq4TsHn0ZOe96OrxXCOBKvwOAaPXRhzay+Vij8vdj0hTrMj0UREIEd1Gc4NjW Y9WFFtOwwQycUyctKHhrTjZeCKXfYv803hP9982OmEw3H3zhAML9S1BW8Up8J5/xNJA9 fzsAoeEfBIvbW/s9KbxqJhLdoh9Lap+0nyd5oS8aTdwcceaLIBpbUcVb95PZl5id1nLN QAda1GC5Tqrm0YPKXfWVIqgDhwPGEY2MIRp8hWuVbwYmD7TJtAlDgCMZGsygxfkIzX3k wsKCqr2/XAidbQit7VW/ZoxVyA7yeU7vVZcnMDH9Szg91YqFhhqOs+6PiFYEPcbm/Kew RbRQ== X-Forwarded-Encrypted: i=1; AJvYcCXQWp+FPDzoT8IiGpt5Uvm6D7yW3o8P/yqrxXnDBp5UO+9agXs80cO446sl2xKp6EwVelXCG/z5m/TI8w==@sourceware.org X-Gm-Message-State: AOJu0Yzua+jJn6WeBZH8gwxZSm1qeGGGFr8YWCCJWQJtbpgJvF9Vm+l4 j2A/YHwzKG5cLWpYHbGUAgz1D59c8oks4TJSZVit3nMltoNZqk7zV15u9ZsE2ShibTx84iud6+J lbFgN3BzLjFULORYU+2pMuAfaF2pXLkll1O+s9KELAf00oli/nFyFPLJdako= X-Received: by 2002:a05:600c:35c3:b0:426:627d:5542 with SMTP id 5b1f17b1804b1-429dd25fa48mr22804515e9.28.1723654831146; Wed, 14 Aug 2024 10:00:31 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH9X++XhReFFeBa5DxwVj+Jh+RCqe9+JkMpiuDRnIAjbiE+3dkjvWPvQNeuVW+SCY1qRDSTkA== X-Received: by 2002:a05:600c:35c3:b0:426:627d:5542 with SMTP id 5b1f17b1804b1-429dd25fa48mr22804075e9.28.1723654830341; Wed, 14 Aug 2024 10:00:30 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-429dec83d2asm25647945e9.0.2024.08.14.10.00.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Aug 2024 10:00:29 -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: <7b23d70f-7392-4729-aff7-fc1190d4a274@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> <875xsbysp9.fsf@redhat.com> <67150769-8f37-4457-ab8e-7f4910bc449f@arm.com> <87bk1wxu5p.fsf@redhat.com> <7b23d70f-7392-4729-aff7-fc1190d4a274@arm.com> Date: Wed, 14 Aug 2024 18:00:29 +0100 Message-ID: <875xs3xcnm.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_H3, RCVD_IN_MSPIKE_WL, 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 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/13/24 17:30, Andrew Burgess wrote: >> Luis Machado writes: >> >>> On 8/8/24 15:50, Andrew Burgess wrote: >>>> 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... >>> >>> I did some more debugging on this, and I think I see what's going on here. I haven't >>> checked the flow in detail yet, but hopefully it will ring some bells. >>> >>> My theory is that your patch's logic is sane, but we're hitting concurrency issues when >>> running things in parallel, or parallel enough. For instance, I consistently hit issues when >>> using -j4, but I can't hit it with anything below -j4. >>> >>> I added a couple debugging statements in gdb_do_cache, at the end within the if {[info exists GDB_PARALLEL]} block. >>> >>> The statements check if the $cache_filename exists and what the contents are, before we rename >>> $cache_filename.[pid] to $cache_filename, essentially overwriting the old cache file. >>> >>> What I saw was the following: >>> >>> --- >>> >>> gdb_do_cache: aarch64_initialize_sve_information ( ) >>> gdb_caching_proc aarch64_initialize_sve_information caused gdb_exit to be called >>> 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 >>> gdb_real__allow_aarch64_sve_tests >>> allow_aarch64_sve_tests >>> >>> XXXX: File builds/binutils-gdb/gdb/testsuite/cache/unix/allow_aarch64_sve_tests already exists before rename. >>> XXXX: Old contents: 1 true aarch64_initialize_sve_information >>> XXXX: New contents: 1 true {} >>> gdb_caching_proc allow_aarch64_sve_tests caused gdb_exit to be called >>> 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 >>> >>> --- >>> >>> So we replace a cache file that contains more information with a copy that has less information. >>> >>> Eventually we call aarch64_initialize_sve_information again, via a different function, and things go bad. >>> >>> I'm guessing the logic of updating the cache files needs to be atomic, and we're having some timing issues >>> where two (or more) tests try to update the same file and we end up losing information. >> >> Thanks for doing the leg work on this. The information you provided is >> spot on, and it was indeed a timing issue as you predicted. Here's >> what's happening: >> >> In thread #1 we call 'allow_aarch64_sme_tests', there's no cache file >> yet we eval the body, this calls 'aarch64_initialize_sme_information'. >> So we try to create two cache files in this order: >> >> unix/aarch64_initialize_sme_information >> value: ... whatever ... >> exit_called?: true >> also_called: {} >> >> unix/allow_aarch64_sme_tests >> value: ... whatever ... >> exit_called?: true >> also_called: aarch64_initialize_sme_information >> >> In thread #2 we also call 'allow_aarch64_sme_tests', however, if we >> manage to land after the first cache file was created but before the >> second file was created then we pick up the cached result for >> 'aarch64_initialize_sme_information'. >> >> And this is where my bug was: I didn't add >> 'aarch64_initialize_sme_information' to the also_called list for a >> caching proc if we managed to find a cached value. As a result, the >> thread #2 cache file looked like this: >> >> unix/allow_aarch64_sme_tests >> value: ... whatever ... >> exit_called?: true >> also_called: {} >> >> which then overwrote the first cache file (atomically). >> >> A later test would call 'allow_aarch64_sme_tests' and then separately >> call 'aarch64_initialize_sme_information'. Due to the corrupted cache >> file calling 'allow_aarch64_sme_tests' would not mark >> 'aarch64_initialize_sme_information' as having been called. So when we >> separately call 'aarch64_initialize_sme_information' gdb_exit would be >> called which broke the test. >> >> The fix, of course, is to ensure that when we get a cache hit we still >> record the called function as being something that is "also called". >> >> I have a new patch below (discard the previous patch I sent) which I >> think should resole the problems you're having. I'm still running the >> full tests on my end, but initial (limited) testing looks good. > > Great. Glad the information was useful. I gave the attached patch a try and > managed to run all the sme tests in parallel and did not see any FAIL's or > errors. > > I'm doing a few more runs just in case, but I think we're good. > That's great news. If I don't hear anything in a couple of days then I'll push this fix. Thanks, Andrew