Message ID | c74d2f2f63597ca1af35dea08e5bd5a22501fde7.1684179487.git.aburgess@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F02433858408 for <patchwork@sourceware.org>; Mon, 15 May 2023 19:39:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F02433858408 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1684179546; bh=ppL6XEvINrpSp1JjY3ig/mhXKbUdKddNanEB8gfgko8=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=cM6XumYSAtBoipv6lT/b8cIUqf9Tz7tx64ZYhMDp/LtqCz3awdVp/p13Z7a9UaGpH MotUZVZmk3MZGbPGZ+ToFsw6BBqppRKngo3+BHXN/Jyp6u2t9xcKAJVzygxEE9coR+ ErUrkvAey4DtzQpYC1ldDYkOw7LQ7yGCn9DnNGwU= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7192C3858D33 for <gdb-patches@sourceware.org>; Mon, 15 May 2023 19:38:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7192C3858D33 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-551-kTMApMJwNqKz4bIhhblzYg-1; Mon, 15 May 2023 15:38:40 -0400 X-MC-Unique: kTMApMJwNqKz4bIhhblzYg-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-3064d0b726fso4623100f8f.0 for <gdb-patches@sourceware.org>; Mon, 15 May 2023 12:38:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684179519; x=1686771519; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ppL6XEvINrpSp1JjY3ig/mhXKbUdKddNanEB8gfgko8=; b=bzNnYdR3y4vGPTkkf7R/m55dsXpP7aa16dV2FTMsbg+X+gcQDfPAoOxjR7kOtsGHsF ulF8Of2ANc4/w0GjJ2c99kpZSkA1lvDISQ6HEB6xyrOtmQVND+T4NocUkp0Y47d+cyTt dOH4WbLFTo4je5WRj0/rwZhH/SV6N5FCBFSjLni0RrohxbDOhTg485BSQDMxyKzeJTCY w7dqhFoeQTnB47X5tl2z2prEK5tBUqWAPx41eN/DIqjG4nTYLGPb3LnK1WUR1+ARg5zw KeiZIicyChRh2oquoE013bwlMab63K5H5gynX57JXy/OJMsqniYvzxaR8NpbQFKqiamg 1PVg== X-Gm-Message-State: AC+VfDwq3SntSXYwWlvMXo9ilFLBsB/bcsqLTxvVRRyGf0q310jcFYI0 mJus5oO1ohOtgmxOqsiQdFDOYOJGezeQxBRmfEF9890iiuhnahgTTSWdNHJr+mbq1AHbaZfA8OO 7pg7ytc4n5Msmevf8+EpprxmJP4uuTnO+fuf8yyqt/uz/NEn+5eHMgvOBOZES7jqCmmxkxwq/YA rqrD+l3g== X-Received: by 2002:adf:ff84:0:b0:306:2aa0:ce81 with SMTP id j4-20020adfff84000000b003062aa0ce81mr25367140wrr.30.1684179519208; Mon, 15 May 2023 12:38:39 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4BRP2FBInLi9qqG4ZsLWP3YpnEFZGWV7+o16vsHmyNeSDB+mmSfSvMJinOI+Jxd+UL14ysgw== X-Received: by 2002:adf:ff84:0:b0:306:2aa0:ce81 with SMTP id j4-20020adfff84000000b003062aa0ce81mr25367130wrr.30.1684179518777; Mon, 15 May 2023 12:38:38 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id v15-20020a5d43cf000000b003063a1cdaf2sm113766wrr.48.2023.05.15.12.38.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 May 2023 12:38:38 -0700 (PDT) To: gdb-patches@sourceware.org Cc: Andrew Burgess <aburgess@redhat.com> Subject: [PATCH] gdb/testsuite: fix s390 failure in break-main-file-remove-fail.exp Date: Mon, 15 May 2023 20:38:33 +0100 Message-Id: <c74d2f2f63597ca1af35dea08e5bd5a22501fde7.1684179487.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, HEXHASH_WORD, RCVD_IN_DNSWL_NONE, 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.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Andrew Burgess <aburgess@redhat.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
gdb/testsuite: fix s390 failure in break-main-file-remove-fail.exp
|
|
Commit Message
Andrew Burgess
May 15, 2023, 7:38 p.m. UTC
After this commit: commit a68f7e9844208ad8cd498f89b5100084ece7d0f6 Date: Tue May 9 10:28:42 2023 +0100 gdb/testsuite: extend special '^' handling to gdb_test_multiple buildbot notified me of a regression on s390 in the test: gdb.base/break-main-file-remove-fail.exp the failure looks like this: print /d ((int (*) (void *, size_t)) munmap) (16781312, 4096) warning: Error removing breakpoint 0 $2 = 0 (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: get integer valueof "((int (*) (void *, size_t)) munmap) (16781312, 4096)" The above commit changed get_integer_valueof so that no output is expected between the command and the '$2 = 0' line. In this case the 'warning: Error removing breakpoint 0' output is causing the get_integer_valueof call to fail. The reason for this warning is that this test deliberately calls munmap on a page of the inferior's code. The test is checking that GDB can handle the situation where a s/w breakpoint can't be removed (due to the page no longer being readable/writable). The test that is supposed to trigger the warning is later in the test script when we delete a breakpoint. So why does s390 trigger the breakpoint earlier during the inferior call? The s390 target uses AT_ENTRY_POINT as it's strategy for handling inferior calls, that is, the trampoline that calls the inferior function is places at the program's entry point, e.g. often the _start label. If this location happens to be on the same page as the test script unmaps then when the inferior function call returns GDB will not be able to remove the temporary breakpoint that is inserted to catch the inferior function call returning! As a result we end up seeing the warning earlier than expected. I did wonder if this means I should relax the pattern in get_integer_valueof - just accept that there might be additional output from GDB which we should ignore. However, I don't think this the right way to go. With the change in a68f7e984420 we are now stricter for GDB emitting warnings, and I think that's a good thing. So, I think, in this case, in order to handle the possible extra output, we should implement something like get_integer_valueof directly in the gdb.base/break-main-file-remove-fail.exp test script. After this the test once again passes on s390. --- .../gdb.base/break-main-file-remove-fail.exp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) base-commit: 6a1cf1bfedbcdb977d9ead3bf6a228360d78cc1b
Comments
On 5/15/23 20:38, Andrew Burgess via Gdb-patches wrote: > After this commit: > > commit a68f7e9844208ad8cd498f89b5100084ece7d0f6 > Date: Tue May 9 10:28:42 2023 +0100 > > gdb/testsuite: extend special '^' handling to gdb_test_multiple > > buildbot notified me of a regression on s390 in the test: > > gdb.base/break-main-file-remove-fail.exp > > the failure looks like this: > > print /d ((int (*) (void *, size_t)) munmap) (16781312, 4096) > warning: Error removing breakpoint 0 > $2 = 0 > (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: get integer valueof "((int (*) (void *, size_t)) munmap) (16781312, 4096)" > > The above commit changed get_integer_valueof so that no output is > expected between the command and the '$2 = 0' line. In this case the > 'warning: Error removing breakpoint 0' output is causing the > get_integer_valueof call to fail. > > The reason for this warning is that this test deliberately calls > munmap on a page of the inferior's code. The test is checking that > GDB can handle the situation where a s/w breakpoint can't be > removed (due to the page no longer being readable/writable). > > The test that is supposed to trigger the warning is later in the test > script when we delete a breakpoint. > > So why does s390 trigger the breakpoint earlier during the inferior > call? > > The s390 target uses AT_ENTRY_POINT as it's strategy for handling > inferior calls, that is, the trampoline that calls the inferior > function is places at the program's entry point, e.g. often the _start > label. > > If this location happens to be on the same page as the test script > unmaps then when the inferior function call returns GDB will not be > able to remove the temporary breakpoint that is inserted to catch the > inferior function call returning! As a result we end up seeing the > warning earlier than expected. > > I did wonder if this means I should relax the pattern in > get_integer_valueof - just accept that there might be additional > output from GDB which we should ignore. > > However, I don't think this the right way to go. With the change in > a68f7e984420 we are now stricter for GDB emitting warnings, and I > think that's a good thing. > > So, I think, in this case, in order to handle the possible extra > output, we should implement something like get_integer_valueof > directly in the gdb.base/break-main-file-remove-fail.exp test script. > > After this the test once again passes on s390. > --- > .../gdb.base/break-main-file-remove-fail.exp | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp > index 66ccc60a21a..c7cf4f3df00 100644 > --- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp > +++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp > @@ -89,7 +89,22 @@ proc test_remove_bp { initial_load } { > set align_addr [expr $bp_addr - $bp_addr % $pagesize] > set munmap_prototype "int (*) (void *, size_t)" > set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)" > - set munmap [get_integer_valueof $munmap_expr -1] > + > + # Use gdb_test_multiple here rather than get_integer_valueof. > + # Targets that use the AT_ENTRY_POINT strategy for inferior > + # function calls will place a breakpoint near the entry point > + # to catch the return from the inferior function call, and > + # this is likely on the page we are about to unmap. As a > + # consequence we will see the warning about being unable to > + # remove the breakpoint here, which throws off > + # get_integer_valueof. > + set munmap -1 > + gdb_test_multiple "print /d ${munmap_expr}" "get result of munmap call" { > + -re -wrap "^(?:warning: Error removing breakpoint $::decimal\r\n)?\\$\[0-9\]* = (\[-\]*\[0-9\]*).*" { > + set munmap $expect_out(1,string) > + pass $gdb_test_name > + } > + } > > if {$munmap != 0} { > unsupported "can't munmap foo's page" > > base-commit: 6a1cf1bfedbcdb977d9ead3bf6a228360d78cc1b Thanks for the patch. I was chasing this one as well, as it also fails for arm and aarch64. From what I noticed, it also fails for ppc/ppc64. LGTM.
Luis Machado <luis.machado@arm.com> writes: > On 5/15/23 20:38, Andrew Burgess via Gdb-patches wrote: >> After this commit: >> >> commit a68f7e9844208ad8cd498f89b5100084ece7d0f6 >> Date: Tue May 9 10:28:42 2023 +0100 >> >> gdb/testsuite: extend special '^' handling to gdb_test_multiple >> >> buildbot notified me of a regression on s390 in the test: >> >> gdb.base/break-main-file-remove-fail.exp >> >> the failure looks like this: >> >> print /d ((int (*) (void *, size_t)) munmap) (16781312, 4096) >> warning: Error removing breakpoint 0 >> $2 = 0 >> (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: get integer valueof "((int (*) (void *, size_t)) munmap) (16781312, 4096)" >> >> The above commit changed get_integer_valueof so that no output is >> expected between the command and the '$2 = 0' line. In this case the >> 'warning: Error removing breakpoint 0' output is causing the >> get_integer_valueof call to fail. >> >> The reason for this warning is that this test deliberately calls >> munmap on a page of the inferior's code. The test is checking that >> GDB can handle the situation where a s/w breakpoint can't be >> removed (due to the page no longer being readable/writable). >> >> The test that is supposed to trigger the warning is later in the test >> script when we delete a breakpoint. >> >> So why does s390 trigger the breakpoint earlier during the inferior >> call? >> >> The s390 target uses AT_ENTRY_POINT as it's strategy for handling >> inferior calls, that is, the trampoline that calls the inferior >> function is places at the program's entry point, e.g. often the _start >> label. >> >> If this location happens to be on the same page as the test script >> unmaps then when the inferior function call returns GDB will not be >> able to remove the temporary breakpoint that is inserted to catch the >> inferior function call returning! As a result we end up seeing the >> warning earlier than expected. >> >> I did wonder if this means I should relax the pattern in >> get_integer_valueof - just accept that there might be additional >> output from GDB which we should ignore. >> >> However, I don't think this the right way to go. With the change in >> a68f7e984420 we are now stricter for GDB emitting warnings, and I >> think that's a good thing. >> >> So, I think, in this case, in order to handle the possible extra >> output, we should implement something like get_integer_valueof >> directly in the gdb.base/break-main-file-remove-fail.exp test script. >> >> After this the test once again passes on s390. >> --- >> .../gdb.base/break-main-file-remove-fail.exp | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp >> index 66ccc60a21a..c7cf4f3df00 100644 >> --- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp >> +++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp >> @@ -89,7 +89,22 @@ proc test_remove_bp { initial_load } { >> set align_addr [expr $bp_addr - $bp_addr % $pagesize] >> set munmap_prototype "int (*) (void *, size_t)" >> set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)" >> - set munmap [get_integer_valueof $munmap_expr -1] >> + >> + # Use gdb_test_multiple here rather than get_integer_valueof. >> + # Targets that use the AT_ENTRY_POINT strategy for inferior >> + # function calls will place a breakpoint near the entry point >> + # to catch the return from the inferior function call, and >> + # this is likely on the page we are about to unmap. As a >> + # consequence we will see the warning about being unable to >> + # remove the breakpoint here, which throws off >> + # get_integer_valueof. >> + set munmap -1 >> + gdb_test_multiple "print /d ${munmap_expr}" "get result of munmap call" { >> + -re -wrap "^(?:warning: Error removing breakpoint $::decimal\r\n)?\\$\[0-9\]* = (\[-\]*\[0-9\]*).*" { >> + set munmap $expect_out(1,string) >> + pass $gdb_test_name >> + } >> + } >> >> if {$munmap != 0} { >> unsupported "can't munmap foo's page" >> >> base-commit: 6a1cf1bfedbcdb977d9ead3bf6a228360d78cc1b > > Thanks for the patch. I was chasing this one as well, as it also fails > for arm and aarch64. From what I noticed, it also fails for ppc/ppc64. Sorry for introducing the regressions. I've gone ahead and pushed this patch as it was impacting so many targets. If there's any follow up feedback I'm happy to make any additional changes needed. Thanks, Andrew
diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp index 66ccc60a21a..c7cf4f3df00 100644 --- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp +++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp @@ -89,7 +89,22 @@ proc test_remove_bp { initial_load } { set align_addr [expr $bp_addr - $bp_addr % $pagesize] set munmap_prototype "int (*) (void *, size_t)" set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)" - set munmap [get_integer_valueof $munmap_expr -1] + + # Use gdb_test_multiple here rather than get_integer_valueof. + # Targets that use the AT_ENTRY_POINT strategy for inferior + # function calls will place a breakpoint near the entry point + # to catch the return from the inferior function call, and + # this is likely on the page we are about to unmap. As a + # consequence we will see the warning about being unable to + # remove the breakpoint here, which throws off + # get_integer_valueof. + set munmap -1 + gdb_test_multiple "print /d ${munmap_expr}" "get result of munmap call" { + -re -wrap "^(?:warning: Error removing breakpoint $::decimal\r\n)?\\$\[0-9\]* = (\[-\]*\[0-9\]*).*" { + set munmap $expect_out(1,string) + pass $gdb_test_name + } + } if {$munmap != 0} { unsupported "can't munmap foo's page"