From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Ua5UNX+Fs2ZCNj8AWB0awg (envelope-from ) for ; Wed, 07 Aug 2024 10:32:31 -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=EhuVgHY7; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id C5F681E0D0; Wed, 7 Aug 2024 10:32:31 -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 A85911E08C for ; Wed, 7 Aug 2024 10:32:29 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 07FEE3857011 for ; Wed, 7 Aug 2024 14:32:29 +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 B96293858C41 for ; Wed, 7 Aug 2024 14:31:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B96293858C41 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 B96293858C41 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=1723041121; cv=none; b=xqgXDFyh6Dwz92qObTB8Wo3cS5PuLEUb4t9ne/tnscXVEZ+cHHOVMJcUftKqi7BNgDdSGtGw/oCNjTZO4D1jy2gA1Nt8MmLfJyEXCEyTkY143/cWVI1typHoaBve9DuWzNvArJpD04h6GNXgOCvLYUU9axg8/iLpu3RqN04OOH8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723041121; c=relaxed/simple; bh=XlvWorutlzphhQIoTTxI1czmYP0QfuenvEglHIfr2+Q=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=OWSU8ySMz0J1p6R+1c0qkcECnovlcChODn7CXvI/tNYJqVkC6VHK0xdNYrPUKGx7WD3ECW4SR4qzxPVBuB89K/y3F+hV6eYLg7CUA+0cZB3/hTIjJhNBfSnUr0Q96REbaRrAUqvQwFqf925oKxHQrefGuAWlomARScDW5JTcmqc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723041115; 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=2A11NOXw3EdY9AbHy0DbfIKIog1syOO4g8bSQBKuN68=; b=EhuVgHY7OthePurwq+IgY29o0lWnminScyeJb+k/sgEQp9nHUWL7hD2rRR2FXapaIfoy6N GiNsC6yNzX41ashcio34mgp1yX0RiTMRbsLkZESzIaWK5TqSmZI83LIEmoDljXPMjFji53 xwaaj+aiDFD6XOlxZxVcMLrUsiQ8me8= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-645-zz6DsRG9MwSeg_OVPGTJ3A-1; Wed, 07 Aug 2024 10:31:53 -0400 X-MC-Unique: zz6DsRG9MwSeg_OVPGTJ3A-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4280d8e685eso14042545e9.1 for ; Wed, 07 Aug 2024 07:31:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723041112; x=1723645912; 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=2A11NOXw3EdY9AbHy0DbfIKIog1syOO4g8bSQBKuN68=; b=Qc853rL0fY0/YVRBzsEO4b0bRfifc+oJqAnWtyA+MwhFzMbk7Y3AGYBTm3T7Yc5bmh sEohmv9xUHGdPbbRfQMu0tv8ACocpGS7FNLmKBz3V3WTV9CEqdEE4bezBa3/U/tuJioD EarbtISlKmpF5P7JKbd+XvFjPYxDDC1dEiHd6puMX6tCk01ZJ6HZf/+keQRth8NTAFno oxZTMcWvoCf4ms9GLc24oam6CKzuVJU1QrwLkDVfAinlOQw2g9GBKctgAc+XGGbqePnX nqtKPwUmkm7XZgzyW63MGHswxsJeuzXV2Fe6aYoflWF2al2gjBpPsw18W67YlODE5aJq jojw== X-Forwarded-Encrypted: i=1; AJvYcCW3YI1FZ+r4JY9KLOFA89VMzIkQ3vn92f48zk8iWzbsP4+1UDQNm8YwOe6W8b+ca2EclDVHYSiVrx4EO/UzWtkv99B6o1G4PO2EmA== X-Gm-Message-State: AOJu0YzGUH+5KadbCphP4UjVaRjIau8S/m43P2TDZrJpVBB7IxgDqSk+ wr2MaXJi5qSrZqpoAx41J+FUSolU25lT7q5nK6mZ4e06VQErtMJR0NHxUNSyu7PtWQHH6b4TCJ0 6iRUK+jP0aYyjmLue81Yhw+jxa8Ymg5z1gxA2sesPCBWljaDjVgFXsqAdTwqGBBm9IHI= X-Received: by 2002:a05:600c:46c8:b0:426:54c9:dfed with SMTP id 5b1f17b1804b1-428e6b7e821mr132914015e9.28.1723041111957; Wed, 07 Aug 2024 07:31:51 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEJHr9SyNyG6RQZZovSxUHhU059NQ1C9D8R4InKsXUDuG9zQgGHGKvTBniwfY185Xe2FF5zVw== X-Received: by 2002:a05:600c:46c8:b0:426:54c9:dfed with SMTP id 5b1f17b1804b1-428e6b7e821mr132913655e9.28.1723041110978; Wed, 07 Aug 2024 07:31:50 -0700 (PDT) Received: from localhost ([31.111.84.186]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36bbcf0cc83sm16353355f8f.1.2024.08.07.07.31.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Aug 2024 07:31:50 -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: <97973506-79f4-4216-9c0b-57401b3933f5@arm.com> References: <5dc846ffb6cd8f76ba2769ee7679f5d1b01fae0a.1717438458.git.aburgess@redhat.com> <97973506-79f4-4216-9c0b-57401b3933f5@arm.com> Date: Wed, 07 Aug 2024 15:31:50 +0100 Message-ID: <878qx8z9nt.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 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: > 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 --- commit 27d8be90b65e0b18ff9ca8e356f0b4e95352f446 Author: Andrew Burgess Date: Wed Aug 7 14:51:06 2024 +0100 gdb/testsuite: track nested caching proc calls It was pointed out in this email: https://inbox.sourceware.org/gdb-patches/97973506-79f4-4216-9c0b-57401b3933f5@arm.com that this commit: commit 0726729d344fecf98f8d138e688e77201cc3cece Date: Mon Jun 3 13:56:54 2024 +0100 gdb/testsuite: track if a caching proc calls gdb_exit or not had broken some AArch64 tests. What is going on is that there are two caching procs: allow_aarch64_sme_tests aarch64_initialize_sme_information the allow_aarch64_sme_tests proc makes a call to aarch64_initialize_sme_information, but aarch64_initialize_sme_information is also called from other non-caching procs, like aarch64_supports_sme_svl. Both of the caching procs mentioned above compile and run a helper program, and both of them call gdb_exit. After the above commit, the first call to any caching proc, the body of which calls gdb_exit, will result in a gdb_exit call even if the body is not executed and the result is fetched from the cache. What was observed is that in the first test script allow_aarch64_sme_tests is called, the body of this caching proc is run which calls gdb_exit. Then allow_aarch64_sme_tests calls aarch64_initialize_sme_information, the body of which is run and gdb_exit is called again. The results from both procs are added to the cache. In the next test script allow_aarch64_sme_tests is called. This results in a cache hit, but gdb_exit is also called as this is the first call in this second test script. Later in the test script aarch64_supports_sme_svl is called which calls aarch64_initialize_sme_information. As this is the first call to aarch64_initialize_sme_information in this second test script (remember the body of allow_aarch64_sme_tests was never run) then gdb_exit is called. This call to gdb_exit is new after the above commit and is unexpected. I think the idea behind the above commit is still sound though. If the call to allow_aarch64_sme_tests was removed from the second test script then we would want the extra gdb_exit call as this would expose a real bug in the test. The problem is that after the above commit the nested nature of the caching proc calls becomes important: a call to allow_aarch64_sme_tests should mean that we've also called aarch64_initialize_sme_information, and that relationship isn't currently captured. So in this commit I'm adding another field to the global gdb_data_cache (in lib/cache.exp). This new field is 'also_called'. For every caching proc we'll populate this field with a list of names, these are the names of any nested caching procs that are called when the body of a caching proc is executed. Now when we get a cache hit in gdb_data_cache we mark every proc in the 'also_called' list as having been called. This means that further calls to these procs will no longer trigger a gdb_exit call. diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp index 092b7f136e8..7e1eae9259e 100644 --- a/gdb/testsuite/lib/cache.exp +++ b/gdb/testsuite/lib/cache.exp @@ -59,24 +59,48 @@ 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 } { +# While calling the implementation of a caching proc, that +# implementation might itself call additional caching procs. We need +# to track all of the nested caching procs that are called and we do +# that in this list which is a list containing the names of any nested +# caching procs that are called. + +set gdb_nested_caching_proc_calls {} + +# Called before returning from gdb_do_cache. PROC_NAME is the name of +# the caching proc that was called and CACHE_NAME is that name used to +# store information in the global gdb_data_cache for PROC_NAME. +# +# If CACHE_NAME's 'exit' flag is true in gdb_data_cache then we call +# gdb_exit and then set a global flag to indicate that gdb_exit should +# not be called again for either PROC_NAME or for any caching proc's +# that PROC_NAME itself calls. The list of nested caching procs that +# need to be marked is obtained from the 'also_called' key in the +# gdb_data_cache for CACHE_NAME. + +proc gdb_cache_maybe_gdb_exit { proc_name cache_name } { + global gdb_data_cache + + # The cache 'exit' entry will be true if this caching proc, or any + # caching proc that is recursively called from this caching proc, + # called exit. + if { !$gdb_data_cache(${cache_name},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 + set global_name __${proc_name}__cached_gdb_exit_called if { ![info exists ::${global_name}] } { gdb_exit + verbose -log "gdb_caching_proc $proc_name caused gdb_exit to be called" set ::${global_name} true + + foreach other_name $gdb_data_cache(${cache_name},also_called) { + set global_name __${other_name}__cached_gdb_exit_called + set ::${global_name} true + } } } @@ -107,9 +131,10 @@ proc gdb_do_cache {name args} { if {[info exists gdb_data_cache(${cache_name},value)]} { set cached_value $gdb_data_cache(${cache_name},value) set cached_exit $gdb_data_cache(${cache_name},exit) + set cached_also_called $gdb_data_cache(${cache_name},also_called) verbose "$name: returning '$cached_value' from cache" 2 if { $cache_verify == 0 } { - gdb_cache_maybe_gdb_exit $name $cached_exit + gdb_cache_maybe_gdb_exit $name $cache_name return $cached_value } set is_cached 1 @@ -123,11 +148,13 @@ proc gdb_do_cache {name args} { close $fd set gdb_data_cache(${cache_name},value) [lindex $content 0] set gdb_data_cache(${cache_name},exit) [lindex $content 1] + set gdb_data_cache(${cache_name},also_called) [lindex $content 2] set cached_value $gdb_data_cache(${cache_name},value) set cached_exit $gdb_data_cache(${cache_name},exit) + set cached_also_called $gdb_data_cache(${cache_name},also_called) verbose "$name: returning '$cached_value' from file cache" 2 if { $cache_verify == 0 } { - gdb_cache_maybe_gdb_exit $name $cached_exit + gdb_cache_maybe_gdb_exit $name $cache_name return $cached_value } set is_cached 1 @@ -144,10 +171,16 @@ proc gdb_do_cache {name args} { trace add execution gdb_exit enter gdb_exit_called set ::gdb_exit_trace_in_place true set gdb_exit_trace_created true + set gdb_current_nested_proc_calls {} } else { set gdb_exit_trace_created false + set gdb_current_nested_proc_calls $::gdb_nested_caching_proc_calls } + # Reset the global list of nested caching procs, this means that + # we are only gathering results for the current call. + set ::gdb_nested_caching_proc_calls {} + # 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 @@ -167,6 +200,15 @@ proc gdb_do_cache {name args} { 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 + set gdb_data_cache(${cache_name},also_called) $::gdb_nested_caching_proc_calls + + # We have recorded the list of nested caching procs into the cache + # (above) for CACHE_NAME. Now we add back the previous value and + # also append the current caching proc's name, this means that if + # this is a nested call then out caller will see the complete list + # of nested caching procs so far. + append ::gdb_nested_caching_proc_calls $gdb_current_nested_proc_calls + lappend ::gdb_nested_caching_proc_calls $name # See comment above where OLD_GDB_EXIT_CALLED is set: if # GDB_EXIT_CALLED was previously true then this is a recursive @@ -183,6 +225,7 @@ proc gdb_do_cache {name args} { trace remove execution gdb_exit enter gdb_exit_called unset ::gdb_exit_trace_in_place set ::gdb_exit_called false + set ::gdb_nested_caching_proc_calls {} } # If a value being stored in the cache contains a newline then @@ -196,6 +239,7 @@ proc gdb_do_cache {name args} { if { $cache_verify == 1 && $is_cached == 1 } { set computed_value $gdb_data_cache(${cache_name},value) set computed_exit $gdb_data_cache(${cache_name},exit) + set computed_also_called $gdb_data_cache(${cache_name},also_called) if { $cached_value != $computed_value } { error [join [list "Inconsistent value results for $cache_name:" "cached: $cached_value vs. computed: $computed_value"]] @@ -204,6 +248,10 @@ proc gdb_do_cache {name args} { error [join [list "Inconsistent exit results for $cache_name:" "cached: $cached_exit vs. computed: $computed_exit"]] } + if { $cached_also_called != $computed_also_called } { + error [join [list "Inconsistent also_called results for $cache_name:" + "cached: $cached_also_called vs. computed: $computed_also_called"]] + } } if {[info exists GDB_PARALLEL]} { @@ -213,10 +261,11 @@ proc gdb_do_cache {name args} { set fd [open $cache_filename.[pid] w] puts $fd $gdb_data_cache(${cache_name},value) puts $fd $gdb_data_cache(${cache_name},exit) + puts $fd $gdb_data_cache(${cache_name},also_called) close $fd file rename -force -- $cache_filename.[pid] $cache_filename } - gdb_cache_maybe_gdb_exit $name $gdb_data_cache(${cache_name},exit) + gdb_cache_maybe_gdb_exit $name $cache_name return $gdb_data_cache(${cache_name},value) }