From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0jegFqC1xGbN3A0AWB0awg (envelope-from ) for ; Tue, 20 Aug 2024 11:26:24 -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=D7OhDD12; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 4AD421E0D0; Tue, 20 Aug 2024 11:26:24 -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 2F6481E08C for ; Tue, 20 Aug 2024 11:26:22 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A7E4A386D60B for ; Tue, 20 Aug 2024 15:26:21 +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 2A93B3846084 for ; Tue, 20 Aug 2024 15:25:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2A93B3846084 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 2A93B3846084 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=1724167557; cv=none; b=PXpfzgyFl9qbkSJoYNtXnLBHMsIEU/AE7NTHTJw6Gzj2DTI8QWi6R3B7e4Bs3ynHXUjyL6CxV4CGSHJNzkHoRkW3G/wGpUBu1KY1d0sOK0G6rpcdHHl7WpK9E5Ajk8mS3AHvZflBl65jhyy3oVPwMBeKDqtg4J+iKORIx0R3l34= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1724167557; c=relaxed/simple; bh=XI9Xg4clkoKxbGcDfHvEazOMISfqa9JR9M0tk+FTxfw=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=aFTJT+ODvXj6DnVlyQJH/T/YWFNxaoX6i6M44CLrwBXxebaFCVk+15gv2pK9ywsoESlN3SpnThyoAr/uSdUF1m11p2bfb1u2rjkCFKf2g407rMkINR2LXnP9TO+FEPvojNzDOSd4yqzG/hQhUwMD2IVR85s6AjRPtI0T8JT660w= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1724167552; 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=wCsGZ9ArIXmUKvM5dOgpLK23iYtICP+d9JNodClToBQ=; b=D7OhDD12T8TTijohkNQs2g4brzVdGETP4D2IoxjniqDZMqogSGuqwDXfVLTtqlDWTXrdZD NIgPtlZRsLSD/4Ef18WbqvjAwwGkfRIqeFmNQq4uvDCqGYtfmYGg7t+eNeZLwxWx2dkgq2 4UjnOI0Q6krvJ191tWUg42s9wXnRkI0= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-495-T4e_P2oiOGaMjClshH1uQQ-1; Tue, 20 Aug 2024 11:25:51 -0400 X-MC-Unique: T4e_P2oiOGaMjClshH1uQQ-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-371b187634fso1147571f8f.0 for ; Tue, 20 Aug 2024 08:25:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724167550; x=1724772350; 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=wCsGZ9ArIXmUKvM5dOgpLK23iYtICP+d9JNodClToBQ=; b=gO5wB7GDUnEmhVwen1TmU5Clr2iltX/ht5tJEbu9rz1uduyzRkIRsnAXU4DfElDAGo fRsl7xPWdXXWOLz6Y6bih0TRzLCJpgG2fiVARyfHNJDz4nqsrx/JHUTQfWtWJ+uUlojj 4Wxqb1NtittfbwVKak6vvRRZntqeMHumCgpfX31GdLAI6N20N+wu/3YtvneKQe2nO1DH MXdtHFSgVDNr817xgCa+HMOTRlZHhC2zHHVFuVSQIGetsiwXNBaodr1bsalXFhOeCK9/ DMvl97ge6dgDUk03uyb6zO3kuKy+wM4SpEhrYo1aPALD0ka2sNa8lLvAt+x4kmAnggUs hdig== X-Forwarded-Encrypted: i=1; AJvYcCUNRyLtu5CeIcXetakft7lfapGUC157seAHW1VAxaXFprksbDgoLvlI4cRnE7LEvEwktsoRQ59G0XdEQA==@sourceware.org X-Gm-Message-State: AOJu0Yy5FNbbaxSG5VYybqr6A3zIafsE5xnR8cPu0iRu4Odi5WJzXKyx 5AEtqZ9BvKMyITKsOJ2d11+x0ZQI+40uvN9bkz6AAp7L+oymstj+NhXv/wuuYA1eiDYWyclXG1+ /8+awl13IZq0/elyxJ9rklhbGInaMyQwkpHrZPCDro/P6EXH1CCF6QluwDqcUQfOtN5s= X-Received: by 2002:a5d:5d81:0:b0:371:8351:83e4 with SMTP id ffacd0b85a97d-371c4aa004amr2109565f8f.13.1724167549692; Tue, 20 Aug 2024 08:25:49 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG2mRtUVbWWh3ODdHVFBgnV9DGRC4aTtaRZFTis1n7SlSHaU6QTolE4FaJ0svcz68Ra5Gn6pQ== X-Received: by 2002:a5d:5d81:0:b0:371:8351:83e4 with SMTP id ffacd0b85a97d-371c4aa004amr2109524f8f.13.1724167548432; Tue, 20 Aug 2024 08:25:48 -0700 (PDT) Received: from localhost (178.126.90.146.dyn.plus.net. [146.90.126.178]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3718984993fsm13329923f8f.31.2024.08.20.08.25.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Aug 2024 08:25:48 -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: <8dde1d9a-4de3-44d6-9f4c-10820f78af5b@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> <875xs3xcnm.fsf@redhat.com> <8dde1d9a-4de3-44d6-9f4c-10820f78af5b@arm.com> Date: Tue, 20 Aug 2024 16:25:47 +0100 Message-ID: <87msl7w70k.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=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, 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/14/24 18:00, Andrew Burgess wrote: >> 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. > > The additional testing finished OK. So this is good. Thanks for putting it together. > > Approved-By: Luis Machado > Tested-By: Luis Machado Thanks Luis, I've gone ahead and pushed this patch. Thanks, Andrew