From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id EpVuFpRCyWgCiAYAWB0awg (envelope-from ) for ; Tue, 16 Sep 2025 06:57:24 -0400 Received: by simark.ca (Postfix, from userid 112) id 4465D1E0BA; Tue, 16 Sep 2025 06:57:24 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.3 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED,RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 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 3D17C1E04C for ; Tue, 16 Sep 2025 06:57:23 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8F67E385735A for ; Tue, 16 Sep 2025 10:57:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8F67E385735A Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by sourceware.org (Postfix) with ESMTPS id 6AD1A3858C2A for ; Tue, 16 Sep 2025 10:56:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6AD1A3858C2A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6AD1A3858C2A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.50 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1758020208; cv=none; b=TT/Y1gzpqmUhiO3XNpQH9mPrQSV5cFOmHCaR8iEVe5nIfrnMRVz8E4pcB7392WamtO7E4JowI6MXiNUiUpML1YUTetdVcGUX/prPtAuwsjHs3GobdTIE29auVpvBrFXdNZJi4znypNSDyOEvIi4UDnk+U6Ftm/qvQfN4pEoCfzw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1758020208; c=relaxed/simple; bh=jgQOI8Tntj6kjjROBHBayGsqw12C7I250KJZGZKHH5A=; h=Message-ID:Date:MIME-Version:Subject:From:To; b=x0O1EnRb4hrXw0RXfkcgy/f1P5zCULa7afmKgkyOav7V3JT+7JQc0HnWduKxdUifTcG4G/6b4+p3Sn7pIKHL3tSfXTtPz7IBLw4wWT88CDk4t8dUje8AeL9GHzpPyNJ+25msEyFTaM2P+of5dR52+e9kZ1KVL1O1dtS49i7tbko= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6AD1A3858C2A Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-45f2acb5f42so16201245e9.1 for ; Tue, 16 Sep 2025 03:56:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1758020207; x=1758625007; h=content-transfer-encoding:in-reply-to:content-language:references :to:from:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bJ9JJMxARqmBDDt9Gr0rNXmdQpScgiDeyt+P9E3WSs4=; b=DLM44PHe3HyeEY3ycOkhRyNsbd8R+ttqjWySfL+WXfQZ0xE2FPiXrmA0gMlYLx+Jja qR+soz7SeaVDsNHZRmbU3HTrm4mUKbvMOK0iH4ubHqi9AJVwLNtpPNw3sL4KtpzaVdKW qzG+U86iIr+oP9RANw32c+FKYBcKdgnWyNPxvuidANeziH+waoRuHzamMJdWKOLvOHjf pcVKvt+HowR0q3bsLTo9AWZhdwHWe+CSGawha0tQOAQA/QmQSIrG8L8MWIuX87Cz+yTx Up07XVCdtv04KM1ArTlaWL9prNg/QfA6x0QtBW+V3a48bqx+P8BP2rl0GbAZXJrt1AjR O6vA== X-Gm-Message-State: AOJu0Ywhg9c6SLNpIR/G+fAfNXB0LR5x3tAZBrn/aOqiz5ytKOZqENnB vNepQBtB8vBk/mP4vyy2ErifKzFLFm+sevCymmyCGQWGFyUR/Qx09g6Cc09MfRGl X-Gm-Gg: ASbGnctxtfegC+03EpXsJc8fSvXQq2WLPPJihNHalh3wkEwFgTD7XYDG1qutEIbvfkq eGiHoxK5hV+hllGZRe/48LkMWIgQCh3VYFoDUiH/ekTezBPF76Yp7uS7iwzxShC464Zrx6/SpzH zn7A/puJ8MbvA77vnhRtCSwtS0DJPUJmUsMYv0OWfr5P2B+dgT3r41xx2IiFI1CyujitO3E5FUK a03P7NVOX3qMVC0ktIXYGNlIEOWD6CZJcop/wgvE/+AXA3e2T2XD2IV2l5OLQs7X53nIH4blzVF bukT0g5WZNNt2oHKE9OykkXAdsR4GrhDv3q484mZLX5gih0jOaUNAHnDp1Asl5B/SLLK3vaZQEm 772EfK4H/EPW9rzSUNiS28ZCI/1+l+l3yvWOXXqSRf0MBNfLaNAd2kE6te1s9FDA= X-Google-Smtp-Source: AGHT+IGVPzH7kHTX52uHTQCQILH3ZM/Kkuupu6TjEqMP1SP1P70AqCRjN6YQ5Q+o61lwtzEoWKVnqA== X-Received: by 2002:a05:600c:840f:b0:458:b8b0:6338 with SMTP id 5b1f17b1804b1-45f32d173d5mr19756415e9.6.1758020206685; Tue, 16 Sep 2025 03:56:46 -0700 (PDT) Received: from ?IPV6:2001:8a0:fad8:400:81eb:cc29:e92d:bfa3? ([2001:8a0:fad8:400:81eb:cc29:e92d:bfa3]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3e98b439442sm10957529f8f.38.2025.09.16.03.56.45 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Sep 2025 03:56:46 -0700 (PDT) Message-ID: Date: Tue, 16 Sep 2025 11:56:44 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] Fix nested gdb_caching_proc with args; Fix gdb.rocm/ tests From: Pedro Alves To: gdb-patches@sourceware.org References: <20250915212841.4161603-1-pedro@palves.net> <3e29d575-22c4-42b7-a8e9-2d57b7367a49@palves.net> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 On 2025-09-16 10:31, Pedro Alves wrote: > On 2025-09-15 22:54, Pedro Alves wrote: >> I should have mentioned something here: >> >> On 2025-09-15 22:28, Pedro Alves wrote: >> >>> --- a/gdb/testsuite/lib/cache.exp >>> +++ b/gdb/testsuite/lib/cache.exp >>> @@ -24,26 +24,9 @@ proc ignore_pass { msg } { >>> >>> # Call proc real_name and return the result, while ignoring calls to pass. >>> proc gdb_do_cache_wrap {real_name args} { >>> - if { [info procs save_pass] != "" } { >>> - return [uplevel 2 $real_name] >>> + with_override pass ignore_pass { >>> + return [$real_name {*}$args] >>> } >> >> The previous code was using "uplevel 2" here. But I'm struggling to see why that is the >> right level. >> >> When gdb_do_cache_wrap is called via gdb_caching_proc => gdb_do_cache, that takes us to the scope of >> the caller of the gdb_caching_proc, which off hand would seem right. >> >> However, when gdb_do_cache_wrap is called directly by gdb.testsuite/gdb-caching-proc-consistency.exp, it >> takes us to ... the caller of test_proc? >> >> So I'm wondering, what could we possibly want to reference from a higher level in gdb_do_cache_wrap >> that not having the uplevel in gdb_do_cache_wrap would get wrong? A caching proc IMO shouldn't be doing >> something like taking the name of a variable as argument instead of a value, as then the procedure wouldn't >> be guaranteed to be idempotent? So I dropped the uplevel, and found that that doesn't cause any >> testsuite regression. But maybe I'm missing something. If I am, it'd be nice to add a testcase for it. > > I guess that is sort of a tangent. I should preserve the uplevel so that the patch does just one thing. > A change to that detail can always be done separately. > > Here's a new version of the patch without that change. With that out of the way, this patch should be a > lot more obvious. > > I'd like to merge it soon to unbreak the gdb.rocm/ tests. I noticed now that while a full test run did not expose this issue, which explains why I didn't see it originally, I'm able to trigger this with tests other than gdb.rocm/, as long as you run them in isolation, so that the cache is cold for the nested procs. For example, make check TESTS="gdb.base/attach.exp" triggers it too. I'm going ahead and merging it, to minimize disturbances to everyone, with the adjusted commit log below. === 8< === >From 2e7ea115f4bddcd7d3f81e0e9026b76fef4d4253 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 15 Sep 2025 22:12:28 +0100 Subject: [PATCH] Fix nested gdb_caching_proc with args Commit d09eba07 ("Make get_compiler_info use gdb_caching_proc") regressed some tests when you run them in isolation (as this depends on the order the gdb_caching_proc procs' results are cached). E.g.: Running /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp ... ERROR: tcl error sourcing /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.rocm/simple.exp. ERROR: tcl error code TCL WRONGARGS ERROR: wrong # args: should be "gdb_real__get_compiler_info_1 language" while executing "gdb_real__get_compiler_info_1" ("uplevel" body line 1) invoked from within "uplevel 2 $real_name" (procedure "gdb_do_cache_wrap" line 3) invoked from within "gdb_do_cache_wrap $real_name {*}$args" (procedure "gdb_do_cache" line 98) invoked from within gdb.base/attach.exp triggers it too, for example. This is actually a latent problem in gdb_do_cache_wrap, introduced in: commit 71f1ab80f1aabd70bce526635f84c7b849e8a0f4 CommitDate: Mon Mar 6 16:49:19 2023 +0100 [gdb/testsuite] Allow args in gdb_caching_proc This change: # Call proc real_name and return the result, while ignoring calls to pass. -proc gdb_do_cache_wrap {real_name} { +proc gdb_do_cache_wrap {real_name args} { if { [info procs save_pass] != "" } { return [uplevel 2 $real_name] <<<<<<<<<<<<<<<<<<<<<<< HERE } @@ -31,7 +31,7 @@ proc gdb_do_cache_wrap {real_name} { rename pass save_pass rename ignore_pass pass - set code [catch {uplevel 2 $real_name} result] + set code [catch {uplevel 2 [list $real_name {*}$args]} result] Missed updating the line marked with HERE above, to pass down $args. So the case of a caching proc calling another caching proc with args isn't handled correctly. We could fix this by fixing the HERE line like so: - return [uplevel 2 $real_name] + return [uplevel 2 [list $real_name {*}$args]] However, we have with_override nowadays that we can use here which eliminates the duplicated logic, which was what was missed originally. A new test that exposes the problem is added to gdb.testsuite/gdb-caching-proc.exp. This also adds a new test to gdb.testsuite/with-override.exp that I think was missing, making sure that the inner foo override restores the outer foo override. Tested-By: Simon Marchi Change-Id: I8b2a7366bf910902fe5f547bde58c3b475bf5133 --- .../gdb.testsuite/gdb-caching-proc.exp | 21 +++++++++++++------ gdb/testsuite/gdb.testsuite/with-override.exp | 4 ++++ gdb/testsuite/lib/cache.exp | 21 ++----------------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp index f9610afd076..6b46b1c555a 100644 --- a/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp +++ b/gdb/testsuite/gdb.testsuite/gdb-caching-proc.exp @@ -22,23 +22,32 @@ gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg { arg } { return $arg } +gdb_caching_proc gdb_testsuite_gdb_caching_proc_exp_arg_nested { arg } { + incr ::count + return [gdb_testsuite_gdb_caching_proc_exp_arg $arg] +} + +# List of "expected $::count after running expression" and +# "expression". set assertions { - { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 } - { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 } - { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" } + 1 { [gdb_testsuite_gdb_caching_proc_exp_noarg] == 1 } + 1 { [gdb_testsuite_gdb_caching_proc_exp_arg 1] == 1 } + 1 { [gdb_testsuite_gdb_caching_proc_exp_arg "foo foo"] == "foo foo" } + 1 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "foo foo"] == "foo foo" } + 2 { [gdb_testsuite_gdb_caching_proc_exp_arg_nested "bar bar"] == "bar bar" } } set assertion_nr 0 -foreach assertion $assertions { +foreach {expected_count assertion} $assertions { with_test_prefix $assertion_nr { set ::count 0 gdb_assert $assertion - gdb_assert { $::count == 1 } + gdb_assert { $::count == $expected_count } with_test_prefix cached { gdb_assert $assertion - gdb_assert { $::count == 1 } + gdb_assert { $::count == $expected_count } } } incr assertion_nr diff --git a/gdb/testsuite/gdb.testsuite/with-override.exp b/gdb/testsuite/gdb.testsuite/with-override.exp index 1a4ee6adbbb..2a316f904e4 100644 --- a/gdb/testsuite/gdb.testsuite/with-override.exp +++ b/gdb/testsuite/gdb.testsuite/with-override.exp @@ -42,6 +42,10 @@ with_test_prefix no-args { gdb_assert { [foo] == 2 } } } + + with_test_prefix "foo1 again" { + gdb_assert { [foo] == 1 } + } } with_test_prefix after { diff --git a/gdb/testsuite/lib/cache.exp b/gdb/testsuite/lib/cache.exp index 6ca3f183f9f..4ebb8259957 100644 --- a/gdb/testsuite/lib/cache.exp +++ b/gdb/testsuite/lib/cache.exp @@ -24,26 +24,9 @@ proc ignore_pass { msg } { # Call proc real_name and return the result, while ignoring calls to pass. proc gdb_do_cache_wrap {real_name args} { - if { [info procs save_pass] != "" } { - return [uplevel 2 $real_name] + with_override pass ignore_pass { + return [uplevel 2 [list $real_name {*}$args]] } - - rename pass save_pass - rename ignore_pass pass - - set code [catch {uplevel 2 [list $real_name {*}$args]} result] - - rename pass ignore_pass - rename save_pass pass - - if {$code == 1} { - global errorInfo errorCode - return -code error -errorinfo $errorInfo -errorcode $errorCode $result - } elseif {$code > 1} { - return -code $code $result - } - - return $result } # Global written to by gdb_exit_called proc. Is set to true to base-commit: d09eba07ca013e6b95eeafd67d79c32ebf6f28eb -- 2.51.0