From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 205533858D1E for ; Tue, 29 Nov 2022 08:52:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 205533858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669711941; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GRM5AEtdDeESC/bNrP9PuypJkiwODzO7rxKbHfhYcKw=; b=R70xw/X/kR8b6SQolwStzB6tWhxbtn6PVNHn11GEu27yTkYnkGWx8Z8IolcjqExp7lZYaX sZCdV+UY92PKBlDUBQOKVCiDA8qcYF2nkm7QvdNZGxldLNzY57F0B0JgIBmic4N7nk7N3v /sxG4mNgRKJGf7xxo1r8thP2O33+9rY= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-5-JnAwt_9fNcGdYX71LF7nRA-1; Tue, 29 Nov 2022 03:52:20 -0500 X-MC-Unique: JnAwt_9fNcGdYX71LF7nRA-1 Received: by mail-ej1-f71.google.com with SMTP id nb1-20020a1709071c8100b007ae4083d6f5so6037159ejc.15 for ; Tue, 29 Nov 2022 00:52:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=GRM5AEtdDeESC/bNrP9PuypJkiwODzO7rxKbHfhYcKw=; b=yggjK7EJLeNaDCbFpN2SPhhFLnVJ0rzTaDqbyYPzYZHrzUpUIs8nKeTv1pUM3Kugww yF7LCLkfGFtBamDBI3WA7bMXfE6Zd1w722KDsEEXAW//h+hDTSdUL7yUqoZYrJQt9PAN D3Me61w6PjLERYufCPHO5RnakyswqkbFByzuP2VA7uB4tFmZLAJM76y04AR1/fF65lbu AGiR9k9Lh32VQj7VwBWJUFX9YuFbMtrDZQm0wXmbhq2gKLdSC9hMkYIME5PYFbvexfsc bvr336gnNY2I/Hj3EClUW86eJYUhv33w4+XaWS6aWMA6VHJ9cHcbUmGQST8/AC9Uqhp3 /2fg== X-Gm-Message-State: ANoB5pkP5+MDtO5Ej9OXBUmZqXGsjOqQOAmuQXEomBCgAVdC0NjkMdj1 7vURztKt8m6KZR/EEiFB14ZarGK65AlGqAAZrOVPSl7plRz+2hNlXCp+OkTo3EcRFQE0IqdZdIk 6TsiDe1Nz31RzMSspasL6sg== X-Received: by 2002:a17:906:a884:b0:7bd:15a6:27e8 with SMTP id ha4-20020a170906a88400b007bd15a627e8mr16309425ejb.78.1669711939133; Tue, 29 Nov 2022 00:52:19 -0800 (PST) X-Google-Smtp-Source: AA0mqf7KqafGWQFmuTo4Mt/2zqHXdFW5Ai+aUDsR9VIvO61CmWz8QuVKpjmvM0ZPO3aJOjUQNZ+t2g== X-Received: by 2002:a17:906:a884:b0:7bd:15a6:27e8 with SMTP id ha4-20020a170906a88400b007bd15a627e8mr16309402ejb.78.1669711938711; Tue, 29 Nov 2022 00:52:18 -0800 (PST) Received: from [10.43.2.105] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id cw14-20020a056402228e00b00467481df198sm6044547edb.48.2022.11.29.00.52.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Nov 2022 00:52:18 -0800 (PST) Message-ID: <682e79ba-0e4b-3209-4fa2-e94ef8e6c978@redhat.com> Date: Tue, 29 Nov 2022 09:52:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp To: Carl Love , Ulrich Weigand , "gdb-patches@sourceware.org" Cc: "will_schmidt@vnet.ibm.com" References: <69b2451b-1baf-8bd4-25dd-a1b46963981f@redhat.com> <75b124722aa20466d5044fa115b679b61ff83010.camel@de.ibm.com> <526c2ea0e35214ef46c248ae88bcd1346fa8e574.camel@us.ibm.com> From: Bruno Larsen In-Reply-To: <526c2ea0e35214ef46c248ae88bcd1346fa8e574.camel@us.ibm.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 List-Id: On 28/11/2022 19:52, Carl Love wrote: > Bruno, GDB maintainers: > > Version 3, per the comments from Ulrich, I created new source files for > the two tests based on their original source files. The expect files > were then updated to use the new source files. The updated patch has > been re-tested on PowerPC and x86-64 with no new regression test > failures. > > The first attempt to fix test next-reverse-bkpt-over-sr.exp basically > breaks the test per the comments from Bruno in the thread with the > subject [PATCH] Fix test next-reverse-bkpt-over-sr.exp. > > The following is a second, new approach to fixing the issue. > > PowerPC uses a Global Entry Point (GEP) and a Local Entry Point (LEP). > The GEP is located on the first instruction of the function. It sets > up register r2 before the LEP. > > : > lis r2,4098 <- GEP > addi r2,r2,32512 > mflr r0 <- LEP > std r0,16(r1) > ... > > The issue is the gdb command break *callee sets the breakpoint on the > first instruction in the function. The call to function callee enters > the function at the LEP. Thus gdb never "sees" the breakpoint at the > GEP resulting in the test failing on PowerPC. If the function is > called using a function pointer, then the linker will setup the call to > use the GEP rather instead of the LEP and the test then works as > expected on PowerPC. > > A new function pointer call to callee afollowed by a normal call to > callee is added for test next-reverse-bkpt-over-sr.exp at the end of > the main so as to not change the test behavior of step-reverse.exp. > Note, the source file step-reverse.c is used by both test next-reverse- > bkpt-over-sr.exp and test step-reverse.exp. Test next-reverse-bkpt- > over-sr.exp starts at the new call to callee at the end of the main > program. The rest of the test runs without changes. By entering via > the GEP on PowerPC, the breakpoint is seen and the test passes as > expected. > > Note, on non PowerPC systems the GEP and LEP are the same. Calling > function callee normally or with a function pointer has the same > behavior on non-PowerPC systems. This was specifically verified on > Intel X86-64. > > Test gdb.reverse/finish-reverse-bkpt.exp also fails on PowerPC for the > same reasons as discussed above. Again, this test is modified by > adding a function pointer call to void_func at the end of main. The > source code is used for both test finish-reverse-bkpt.exp and finish- > reverse.exp so the a breakpoint is added to finish-reverse-bkpt.exp to > stop before the function pointer call to void_func where the test then > starts. > > The addition of the function pointer declarations in the two source > files require an additional next statement to tests step-precsave.exp > and step-reverse.exp to keep the keep things lined up. > > Hopefully, this approach to fixing the failing tests on PowerPC is > acceptable without changing the behavior on non-PowerPC platforms. > > The patch has been tested on both PowerPC and X86-64 with no > regressions. > > Please let me know if this patch is acceptable. Thanks. > > Carl Love > > -------------------------------- > > PowerPC, version 3, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp > > The tests set a break point with the command break *func. This sets a > breakpoint on the first instruction of the function. PowerPC uses > Global Entry Points (GEP) and Local Entry Points (LEP). The first > instruction in the function is the GEP. The GEP sets up register > r2 before reaching the LEP. When the function is called with func() the > function is entered via the LEP and the test fails because GDB does not > see the breakpoint on the GEP. However, if the function is called via a > function pointer, execution begins at the GEP as the test expects. > > Currently finish-reverse-bkpt.exp uses source file finish-reverse.c and > next-reverse-bpkt-over-sr.exp uses source file step-reverse.c A new > source file was created for tests finish-reverse-bkpt.exp and > next-reverse-bkpt-over-sr.exp. The new files use the new function > pointer method to call the functions so the tests will work correctly on > both PowerPC with a GEP and LEP as well as on other systems. The GEP is > the same as the LEP on non PowerPC systems. > > The expect files were changed to use the new source files and to set the > initial break point for the rest of the test on the function pointer call > for the function. > > This patch fixes two PowerPC test failures in each of the tests > gdb.reverse/finish-reverse-bkpt.exp and > gdb.reverse/next-reverse-bkpt-over-sr.exp. > > Patch tested on PowerPC and Intel X86-64 with no regressions. Hi Carl, I also tested locally and verify that it adds no new regressions, and your explanations make sense. I have some style nits inlined, but with those fixed you can add my R-b tag: Reviewed-By: Bruno Larsen > --- > .../gdb.reverse/finish-reverse-bkpt.c | 37 ++++++++++++++++ > .../gdb.reverse/finish-reverse-bkpt.exp | 32 +++++++++++++- > .../gdb.reverse/next-reverse-bkpt-over-sr.c | 44 +++++++++++++++++++ > .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 33 +++++++++++--- > 4 files changed, 139 insertions(+), 7 deletions(-) > create mode 100644 gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c > create mode 100644 gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c > > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c > new file mode 100644 > index 00000000000..771793c389f > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c > @@ -0,0 +1,37 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2008-2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +/* Test gdb's "return" command in reverse. The code for this test is based > + on the code in test finish-reverse.c. The code was modified to call the > + function via a function pointer so the test will behave the same on all > + platforms. See comments in finish-reverse-bkpt.exp. */ > + > +int void_test = 0; > + > +void void_func () GDB's coding style would say this should be: void void_func () { I know that a lot of .c files in the test suite aren't completely following the coding standards, but we should avoid introducing new ones if not necessary. > +{ > + void_test = 1; /* VOID FUNC */ > +} > + > +int main (int argc, char **argv) same here. > +{ > + int i; > + void (*funp) (void) = void_func; > + > + funp (); /* FUNCTION PTR call to void_func */ > + return 0; > +} /* end of main */ > diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp > index 2a204748d98..f9682b2962d 100644 > --- a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp > +++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp > @@ -19,11 +19,32 @@ > # the functions entry would be ignored. Make sure the bug doesn't > # reappear. > > +# The test sets a breakpoint with the command break *void_func to set a > +# breakpoint on the first instruction of the function. The issue is on > +# PowerPC it uses Global Entry Points (GEP) and Local Entry Points (LEP). > +# The GEP is the first instruction in the function. It sets up register > +# r2 and then reaches the LEP. > +# > +# : > +# lis r2,4098 <- GEP > +# addi r2,r2,32512 > +# mflr r0 <- LEP > +# std r0,16(r1) > +# .... > + > +# > +# The command break *void_func sets the breakpoint on the GEP. Calling > +# the function with void_func() will enter the function via the LEP. So, > +# this test needs to use a function pointer to call void_func() so the > +# function will be entered via the GEP to work as designed on PowerPC in > +# addition to non-PowerPC systems. On non-PowerPC systems, the GEP and LEP > +# are the same. > + > if ![supports_reverse] { > return > } > > -standard_testfile finish-reverse.c > +standard_testfile > > if { [prepare_for_testing "failed to prepare" "$testfile" $srcfile] } { > return -1 > @@ -38,6 +59,15 @@ if [supports_process_record] { > gdb_test_no_output "record" "turn on process record" > } > > +# Move to the function pointer call to void_func so we will use the GEP > +# to enter void_func and break. > +set breakloc [gdb_get_line_number "FUNCTION PTR" "$srcfile"] > +gdb_test "break $breakloc" \ > + "Breakpoint $decimal at .*$srcfile, line $breakloc\." \ > + "set breakpoint on funp" > +gdb_continue_to_breakpoint "funp call" ".*$srcfile:$breakloc.*" > + > +# Start the test missing period at the end of the comment. > set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"] > gdb_test "tbreak void_func" \ > "Temporary breakpoint $decimal at .*$srcfile, line $breakloc\." \ > diff --git a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c > new file mode 100644 > index 00000000000..ad771e05ca1 > --- /dev/null > +++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c > @@ -0,0 +1,44 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2008-2022 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > +#include > + > +/* Test reverse finish command. The code for this test is based on the code > + in test step-reverse.c. The code was modified to call the function via > + a function pointer so the test will behave the same on all platforms. > + See comments in next-reverse-bkpt-over-sr.exp. */ > + > +int myglob = 0; > + > +int callee() { /* ENTER CALLEE */ Same comment about C coding style. > + return myglob++; /* ARRIVED IN CALLEE */ > +} /* RETURN FROM CALLEE */ > + > +int main () { > + int (*funp) (void) = callee; > + > + /* Test next-reverse-bkpt-over-sr.exp needs to call function callee using > + a function pointer to work correctly on PowerPC. See comments in > + next-reverse-bkpt-over-sr.exp. */ > + funp (); /* FUNCTION PTR CALL TO CALLEE */ > + > + /* Test that "step" doesn't */ > + callee (); /* STEP INTO CALL AFTER FUNP CALL */ I don't see the need for a callee call here. The test doesn't use the second call at any point. As I said before, these are all minor style comments, so I don't think a new version is needed for these comments :) -- Cheers, Bruno > + > + exit (0); /* end of main */ > +} > diff --git a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp > index 6ef56d30e7b..671b1c09dd4 100644 > --- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp > +++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp > @@ -14,20 +14,37 @@ > # along with this program. If not, see . */ > > # This file is part of the GDB testsuite. It tests reverse stepping. > -# Lots of code borrowed from "step-test.exp". > - > # > # reverse-next over a function call sets a step-resume breakpoint at > # callee's entry point, runs to it, and then does an extra single-step > # to get at the callee's caller. Test that a user breakpoint set at > # the same location as the step-resume breakpoint isn't ignored. > # > +# The test sets a breakpoint with the command break *callee to set a > +# breakpoint on the first instruction of the function. The issue is on > +# PowerPC it uses Global Entry Points (GEP) and Local Entry Points (LEP). > +# The GEP is the first instruction in the function. It sets up register > +# r2 and then reaches the LEP. > +# > +# : > +# lis r2,4098 <- GEP > +# addi r2,r2,32512 > +# mflr r0 <- LEP > +# std r0,16(r1) > + > +# > +# The command break *callee sets the breakpoint on the GEP. Calling > +# the function with callee() will enter the function via the LEP. So, > +# this test needs to use a function pointer to call callee() so the > +# function will be entered via the GEP to work as designed on PowerPC in > +# addition to non-PowerPC systems. On non-PowerPC systems, the GEP and LEP > +# are the same. > > if ![supports_reverse] { > return > } > > -standard_testfile step-reverse.c > +standard_testfile > > if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { > return -1 > @@ -42,8 +59,12 @@ if [supports_process_record] { > gdb_test_no_output "record" "turn on process record" > } > > -set lineno [gdb_get_line_number "STEP INTO THIS CALL"] > -gdb_test "advance $lineno" ".*STEP INTO THIS CALL.*" "get past callee call" > +# Move to the function pointer call to the callee call after the function > +# pointer call to callee to begin the test. The function pointer call to > +# callee will use the Global Entry Point on Power. > +set lineno [gdb_get_line_number "STEP INTO CALL AFTER FUNP CALL"] > +gdb_test "advance $lineno" ".*STEP INTO CALL AFTER FUNP CALL.*" \ > + "get past callee call" > > gdb_test "b \*callee" "" "set breakpoint at callee's entry" > > @@ -53,5 +74,5 @@ gdb_test "reverse-next" \ > "reverse-next over call trips user breakpoint at function entry" > > gdb_test "up" \ > - ".*NEXT OVER THIS CALL.*" \ > + ".*FUNCTION PTR CALL TO CALLEE.*" \ > "stopped at the right callee call"