Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp

[Bruno Larsen <blarsen@redhat.com>]

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.
>
>    <callee>:
>     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 <blarsen@redhat.com>

> ---
>   .../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 <http://www.gnu.org/licenses/>.  */
> +
> +/* 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.
> +#
> +#   <void_func>:
> +#  	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 <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +/* 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 <http://www.gnu.org/licenses/>.  */
>   
>   # 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.
> +#
> +#  <callee>:
> +#   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"