public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix test next-reverse-bkpt-over-sr.exp
@ 2022-09-27 16:14 Carl Love
  2022-09-28  7:35 ` Bruno Larsen
  0 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2022-09-27 16:14 UTC (permalink / raw)
  To: gdb-patches; +Cc: Ulrich Weigand, Will Schmidt, cel


GDB maintainers:

The next-reverse-bkpt-over-sr.exp test sets a breakpoint on *callee.
The intention is the test is setting a breakpoint on the entry point of
the function.  However on PowerPC, the statement sets the breakpoint on
the global entry point.  The test uses the local entry point to the
function callee and thus the breakpoint on the global entry point is
never hit resulting in the test failing.

This patch changes the break instruction to callee to properly set the
breakpoint.  

The patch has been tested on both Power10 and X86-64 with no regression
errors.

Please let me know if this patch is acceptable for mainline.  Thanks.

                    Carl Love


---------------------------------------------------------------
Fix test next-reverse-bkpt-over-sr.exp

The test suses a gdb_test to set a breakpoint at *callee.  The message
says "set breakpoint at callee's entry".

The statement b *callee will set a breakpoint on the first instruction of
the function.  On some architectres, that may be the function entry point.
The PowerPC ABI specifies both local and global entry points.  The statement
b *callee sets the breakpoint t the global entry point and b callee sets
the breakpoint at the beginning of the fucntion, i.e. after the local
entry point and prolog.  The local entry point is generally used.  The
global entry point is only used if the TOC needs to be stup.

  int callee() {          /* ENTER CALLEE */
      100007ac:   02 10 40 3c     lis     r2,4098     <- global entry point
      100007b0:   00 7f 42 38     addi    r2,r2,32512
      100007b4:   f8 ff e1 fb     std     r31,-8(r1)  <- local entry point
      100007b8:   d1 ff 21 f8     stdu    r1,-48(r1)  <- prolog
      100007bc:   78 0b 3f 7c     mr      r31,r1      <- prolog
    return myglob++;	/* ARRIVED IN CALLEE */
      100007c0:	00 00 00 60 	nop                       <- location b callee
      100007c4:	40 81 22 39 	addi    r9,r2,-32448
      100007c8:	00 00 29 81 	lwz     r9,0(r9)
      100007cc:	01 00 09 39 	addi    r8,r9,1
       ...

The test should break on callee, not *callee, to get the desired behaviour
on PowerPC as well as other architecturs like X86-64.

This patch has been tested on Poser PC and X86-64 without any regression
failures.
---
 gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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..4e163c5c2f8 100644
--- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
+++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
@@ -45,7 +45,7 @@ if [supports_process_record] {
 set lineno [gdb_get_line_number "STEP INTO THIS CALL"]
 gdb_test "advance $lineno" ".*STEP INTO THIS CALL.*" "get past callee call"
 
-gdb_test "b \*callee" "" "set breakpoint at callee's entry"
+gdb_test "b callee" "" "set breakpoint at callee's entry"
 
 set bpnum [get_integer_valueof "\$bpnum" 0]
 gdb_test "reverse-next" \
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Fix test next-reverse-bkpt-over-sr.exp
  2022-09-27 16:14 [PATCH] Fix test next-reverse-bkpt-over-sr.exp Carl Love
@ 2022-09-28  7:35 ` Bruno Larsen
  2022-11-14 21:05   ` [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp Carl Love
  0 siblings, 1 reply; 17+ messages in thread
From: Bruno Larsen @ 2022-09-28  7:35 UTC (permalink / raw)
  To: Carl Love, gdb-patches; +Cc: Ulrich Weigand

On 27/09/2022 18:14, Carl Love via Gdb-patches wrote:
> GDB maintainers:
>
> The next-reverse-bkpt-over-sr.exp test sets a breakpoint on *callee.
> The intention is the test is setting a breakpoint on the entry point of
> the function.  However on PowerPC, the statement sets the breakpoint on
> the global entry point.  The test uses the local entry point to the
> function callee and thus the breakpoint on the global entry point is
> never hit resulting in the test failing.
>
> This patch changes the break instruction to callee to properly set the
> breakpoint.
>
> The patch has been tested on both Power10 and X86-64 with no regression
> errors.
>
> Please let me know if this patch is acceptable for mainline.  Thanks.
>
>                      Carl Love
>
Hi Carl,

I don't think this change is acceptable. Looking at the comment at the 
top of next-reverse-bkpt-over-sr.exp, we see the following:

# 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.

Based on this, the test seems to expect that a user has placed the 
breakpoint at the exact same instruction as where GDB will place the 
step-resume breakpoint.

By getting GDB to print where it is placing the step-resume breakpoint 
(using the patch at the end of this email), and running the test 
manually to see where breakpoints are located when using "break callee" 
and "break *callee", I get the following output:

(gdb) rn
setting step_resume_breakpoint at 0x401136
55         callee();    /* NEXT OVER THIS CALL */
(gdb) b callee
Breakpoint 2 at 0x40113a: file 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/step-reverse.c, 
line 26.
(gdb) b *callee
Breakpoint 3 at 0x401136: file 
/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/step-reverse.c, 
line 25.

As you can see, setting a breakpoint at callee does not create a user 
breakpoint at the same instruction as the step-resume breakpoint in 
x86_64 processors, rendering this test useless.

I encourage you to try the same patch I used to get the location for the 
step-resume breakpoint on powerpc architectures, and if they turn out to 
be the same, maybe you can make an architecture check at the top of this 
test, changing the location of the breakpoint based on it.

Cheers,
Bruno

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1957e8020dd..1699f4a0c6c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7130,6 +7130,7 @@ process_event_stop_test (struct 
execution_control_state *ecs)
                 {
                   /* Normal function call return (static or dynamic).  */
                   symtab_and_line sr_sal;
+                 gdb_printf ("setting step_resume_breakpoint at 
0x%lx\n", ecs->stop_func_start);
                   sr_sal.pc = ecs->stop_func_start;
                   sr_sal.pspace = get_frame_program_space (frame);
                   insert_step_resume_breakpoint_at_sal (gdbarch,


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-09-28  7:35 ` Bruno Larsen
@ 2022-11-14 21:05   ` Carl Love
  2022-11-21 16:36     ` [PING] " Carl Love
  2022-11-22  9:42     ` Bruno Larsen
  0 siblings, 2 replies; 17+ messages in thread
From: Carl Love @ 2022-11-14 21:05 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches; +Cc: Ulrich Weigand, Will Schmidt, cel

Bruno, GDB maintainers:

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, 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.

The tests were modified to call the function with a function pointer so
the test 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.

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.
---
 .../gdb.reverse/finish-reverse-bkpt.exp       | 30 +++++++++++++++++++
 gdb/testsuite/gdb.reverse/finish-reverse.c    |  4 +++
 .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 29 ++++++++++++++++--
 gdb/testsuite/gdb.reverse/step-precsave.exp   |  1 +
 gdb/testsuite/gdb.reverse/step-reverse.c      |  9 ++++++
 gdb/testsuite/gdb.reverse/step-reverse.exp    |  1 +
 6 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
index 2a204748d98..94bcf41dc67 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
@@ -19,6 +19,27 @@
 # 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
 }
@@ -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
 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/finish-reverse.c b/gdb/testsuite/gdb.reverse/finish-reverse.c
index 316d6f6aa7e..676d960ca9c 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse.c
+++ b/gdb/testsuite/gdb.reverse/finish-reverse.c
@@ -89,6 +89,7 @@ int main (int argc, char **argv)
   float float_resultval;
   double double_resultval;
   int i;
+  void (*funp) (void) = void_func;
 
   /* A "test load" that will insure that the function really returns 
      a ${type} (as opposed to just a truncated or part of a ${type}).  */
@@ -123,6 +124,9 @@ int main (int argc, char **argv)
   testval.double_testval = 3.14159265358979323846; /* float_checkpoint */
   double_resultval    = double_func ();		
   main_test = 1;				/* double_checkpoint */
+
+  /* This call is used with finish-reverse-bkpt.exp.  */
+  funp();                              /* FUNCTION PTR call to void_func */
   return 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..d78c6ac490e 100644
--- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
+++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
@@ -22,6 +22,25 @@
 # 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
@@ -42,8 +61,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 +76,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"
diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
index 3279b6ce879..602dd7e6976 100644
--- a/gdb/testsuite/gdb.reverse/step-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
@@ -76,6 +76,7 @@ gdb_test "record restore $precsave" \
 
 # plain vanilla step/next (no count)
 
+gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
 gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
 gdb_test "step" ".*STEP TEST 1.*" "step test 1"
 
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
index 809c7d16dc9..984fd336510 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -54,6 +54,7 @@ large_struct_by_value (struct rhomboidal r)
 int main () {
    int w,x,y,z;
    int a[10], b[10];
+   int (*funp) (void) = callee;
 
    /* Test "next" and "step" */
    w = 0;	/* BREAK AT MAIN */
@@ -90,6 +91,14 @@ int main () {
      large_struct_by_value (r);  /* step-test.exp: large struct by value */
    }
 
+   /* 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 */
+
    exit (0); /* end of main */
 }
 
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index d2975cffb5c..bce137a97ad 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -40,6 +40,7 @@ if [supports_process_record] {
 
 # plain vanilla step/next (no count)
 
+gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
 gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
 gdb_test "step" ".*STEP TEST 1.*" "step test 1"
 
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PING] Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-14 21:05   ` [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp Carl Love
@ 2022-11-21 16:36     ` Carl Love
  2022-11-22  9:42     ` Bruno Larsen
  1 sibling, 0 replies; 17+ messages in thread
From: Carl Love @ 2022-11-21 16:36 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches; +Cc: Ulrich Weigand, Will Schmidt

Bruno:

When you have a chance, could you take a look at the new version of the
test and see if you agree this does not break the test like my initial
fix did.  Thanks.

                      Carl Love

On Mon, 2022-11-14 at 13:05 -0800, Carl Love wrote:
> Bruno, GDB maintainers:
> 
> 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, 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.
> 
> The tests were modified to call the function with a function pointer
> so
> the test 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.
> 
> 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.
> ---
>  .../gdb.reverse/finish-reverse-bkpt.exp       | 30
> +++++++++++++++++++
>  gdb/testsuite/gdb.reverse/finish-reverse.c    |  4 +++
>  .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 29 ++++++++++++++++-
> -
>  gdb/testsuite/gdb.reverse/step-precsave.exp   |  1 +
>  gdb/testsuite/gdb.reverse/step-reverse.c      |  9 ++++++
>  gdb/testsuite/gdb.reverse/step-reverse.exp    |  1 +
>  6 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> index 2a204748d98..94bcf41dc67 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> @@ -19,6 +19,27 @@
>  # 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
>  }
> @@ -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
>  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/finish-reverse.c
> b/gdb/testsuite/gdb.reverse/finish-reverse.c
> index 316d6f6aa7e..676d960ca9c 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse.c
> @@ -89,6 +89,7 @@ int main (int argc, char **argv)
>    float float_resultval;
>    double double_resultval;
>    int i;
> +  void (*funp) (void) = void_func;
> 
>    /* A "test load" that will insure that the function really
> returns 
>       a ${type} (as opposed to just a truncated or part of a
> ${type}).  */
> @@ -123,6 +124,9 @@ int main (int argc, char **argv)
>    testval.double_testval = 3.14159265358979323846; /*
> float_checkpoint */
>    double_resultval    = double_func ();		
>    main_test = 1;				/* double_checkpoint */
> +
> +  /* This call is used with finish-reverse-bkpt.exp.  */
> +  funp();                              /* FUNCTION PTR call to
> void_func */
>    return 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..d78c6ac490e 100644
> --- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
> +++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
> @@ -22,6 +22,25 @@
>  # 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
> @@ -42,8 +61,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 +76,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"
> diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp
> b/gdb/testsuite/gdb.reverse/step-precsave.exp
> index 3279b6ce879..602dd7e6976 100644
> --- a/gdb/testsuite/gdb.reverse/step-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
> @@ -76,6 +76,7 @@ gdb_test "record restore $precsave" \
> 
>  # plain vanilla step/next (no count)
> 
> +gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
>  gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
>  gdb_test "step" ".*STEP TEST 1.*" "step test 1"
> 
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c
> b/gdb/testsuite/gdb.reverse/step-reverse.c
> index 809c7d16dc9..984fd336510 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -54,6 +54,7 @@ large_struct_by_value (struct rhomboidal r)
>  int main () {
>     int w,x,y,z;
>     int a[10], b[10];
> +   int (*funp) (void) = callee;
> 
>     /* Test "next" and "step" */
>     w = 0;	/* BREAK AT MAIN */
> @@ -90,6 +91,14 @@ int main () {
>       large_struct_by_value (r);  /* step-test.exp: large struct by
> value */
>     }
> 
> +   /* 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 */
> +
>     exit (0); /* end of main */
>  }
> 
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp
> b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index d2975cffb5c..bce137a97ad 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -40,6 +40,7 @@ if [supports_process_record] {
> 
>  # plain vanilla step/next (no count)
> 
> +gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
>  gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
>  gdb_test "step" ".*STEP TEST 1.*" "step test 1"
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-14 21:05   ` [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp Carl Love
  2022-11-21 16:36     ` [PING] " Carl Love
@ 2022-11-22  9:42     ` Bruno Larsen
  2022-11-22 16:53       ` Carl Love
  1 sibling, 1 reply; 17+ messages in thread
From: Bruno Larsen @ 2022-11-22  9:42 UTC (permalink / raw)
  To: Carl Love, gdb-patches; +Cc: Ulrich Weigand, Will Schmidt

On 14/11/2022 22:05, Carl Love wrote:
> Bruno, GDB maintainers:
>
> 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, 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.
>
> The tests were modified to call the function with a function pointer so
> the test 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.
>
> 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,

This was a great approach to solve the problem, but the location of the 
funcp() call in step-reverse.c is generating regressions on my machine.

Currently, GDB can't record memset calls in x86_64 (at least using 
gcc-12.2.1), due to the instruction 0xc5.

Moving the funcp and callee calls above the block that contains memset 
fixes the regression and should still fix the PowerPC issue.

Also, style nits inlined.

> ---
>   .../gdb.reverse/finish-reverse-bkpt.exp       | 30 +++++++++++++++++++
>   gdb/testsuite/gdb.reverse/finish-reverse.c    |  4 +++
>   .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 29 ++++++++++++++++--
>   gdb/testsuite/gdb.reverse/step-precsave.exp   |  1 +
>   gdb/testsuite/gdb.reverse/step-reverse.c      |  9 ++++++
>   gdb/testsuite/gdb.reverse/step-reverse.exp    |  1 +
>   6 files changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> index 2a204748d98..94bcf41dc67 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
> @@ -19,6 +19,27 @@
>   # 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
>   }
> @@ -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
>   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/finish-reverse.c b/gdb/testsuite/gdb.reverse/finish-reverse.c
> index 316d6f6aa7e..676d960ca9c 100644
> --- a/gdb/testsuite/gdb.reverse/finish-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/finish-reverse.c
> @@ -89,6 +89,7 @@ int main (int argc, char **argv)
>     float float_resultval;
>     double double_resultval;
>     int i;
> +  void (*funp) (void) = void_func;
>   
>     /* A "test load" that will insure that the function really returns
>        a ${type} (as opposed to just a truncated or part of a ${type}).  */
> @@ -123,6 +124,9 @@ int main (int argc, char **argv)
>     testval.double_testval = 3.14159265358979323846; /* float_checkpoint */
>     double_resultval    = double_func ();		
>     main_test = 1;				/* double_checkpoint */
> +
> +  /* This call is used with finish-reverse-bkpt.exp.  */
> +  funp();                              /* FUNCTION PTR call to void_func */
Missing space before (
>     return 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..d78c6ac490e 100644
> --- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
> +++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
> @@ -22,6 +22,25 @@
>   # 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
> @@ -42,8 +61,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 +76,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"
> diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
> index 3279b6ce879..602dd7e6976 100644
> --- a/gdb/testsuite/gdb.reverse/step-precsave.exp
> +++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
> @@ -76,6 +76,7 @@ gdb_test "record restore $precsave" \
>   
>   # plain vanilla step/next (no count)
>   
> +gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
>   gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
>   gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>   
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
> index 809c7d16dc9..984fd336510 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.c
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.c
> @@ -54,6 +54,7 @@ large_struct_by_value (struct rhomboidal r)
>   int main () {
>      int w,x,y,z;
>      int a[10], b[10];
> +   int (*funp) (void) = callee;
>   
>      /* Test "next" and "step" */
>      w = 0;	/* BREAK AT MAIN */
> @@ -90,6 +91,14 @@ int main () {
>        large_struct_by_value (r);  /* step-test.exp: large struct by value */
>      }
>   
> +   /* 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 */
again here
> +
> +   /* Test that "step" doesn't */
> +   callee();	/* STEP INTO CALL AFTER FUNP CALL */
and here.
> +
>      exit (0); /* end of main */
>   }
>   
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index d2975cffb5c..bce137a97ad 100644
> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp
> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
> @@ -40,6 +40,7 @@ if [supports_process_record] {
>   
>   # plain vanilla step/next (no count)
>   
> +gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
>   gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
>   gdb_test "step" ".*STEP TEST 1.*" "step test 1"
>   


-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-22  9:42     ` Bruno Larsen
@ 2022-11-22 16:53       ` Carl Love
  2022-11-23  8:44         ` Bruno Larsen
  2022-11-23 17:56         ` Ulrich Weigand
  0 siblings, 2 replies; 17+ messages in thread
From: Carl Love @ 2022-11-22 16:53 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches, cel; +Cc: Ulrich Weigand, Will Schmidt

GDB maintainers, Bruno:

On Tue, 2022-11-22 at 10:42 +0100, Bruno Larsen wrote:
> On 14/11/2022 22:05, Carl Love wrote:
> > Bruno, GDB maintainers:
> > 
> > 
<snip>
> > 
> > Patch tested on PowerPC and Intel X86-64 with no regressions.
> 
> Hi Carl,
> 
> This was a great approach to solve the problem, but the location of
> the 
> funcp() call in step-reverse.c is generating regressions on my
> machine.
> 
> Currently, GDB can't record memset calls in x86_64 (at least using 
> gcc-12.2.1), due to the instruction 0xc5.

OK, my x86_64 box has gcc version 11.3.0 which would explain why I
didn't see an issue.

> 
> Moving the funcp and callee calls above the block that contains
> memset 
> fixes the regression and should still fix the PowerPC issue.

I moved the calls up as described.  So, the latest version should now
work on your system.  
> 
> Also, style nits inlined.

Fixed the three nits.
> 

I have updated the patch, below.  If you could take a quick look and
make sure it works on your system that would be great.  Assuming it is
OK, hopefully we can get a maintainer to bless it and we will get the
patch committed.

Thanks.

                        Carl 
----------------------------------------------------------------------
PowerPC, 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.

The tests were modified to call the function with a function pointer so
the test 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.

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.
---
 .../gdb.reverse/finish-reverse-bkpt.exp       | 30 +++++++++++++++++++
 gdb/testsuite/gdb.reverse/finish-reverse.c    |  4 +++
 .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 29 ++++++++++++++++--
 gdb/testsuite/gdb.reverse/step-precsave.exp   |  1 +
 gdb/testsuite/gdb.reverse/step-reverse.c      |  9 ++++++
 gdb/testsuite/gdb.reverse/step-reverse.exp    |  1 +
 6 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
index 2a204748d98..94bcf41dc67 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
@@ -19,6 +19,27 @@
 # 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
 }
@@ -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
 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/finish-reverse.c b/gdb/testsuite/gdb.reverse/finish-reverse.c
index 316d6f6aa7e..163a028f394 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse.c
+++ b/gdb/testsuite/gdb.reverse/finish-reverse.c
@@ -89,6 +89,7 @@ int main (int argc, char **argv)
   float float_resultval;
   double double_resultval;
   int i;
+  void (*funp) (void) = void_func;
 
   /* A "test load" that will insure that the function really returns 
      a ${type} (as opposed to just a truncated or part of a ${type}).  */
@@ -123,6 +124,9 @@ int main (int argc, char **argv)
   testval.double_testval = 3.14159265358979323846; /* float_checkpoint */
   double_resultval    = double_func ();		
   main_test = 1;				/* double_checkpoint */
+
+  /* This call is used with finish-reverse-bkpt.exp.  */
+  funp ();                              /* FUNCTION PTR call to void_func */
   return 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..d78c6ac490e 100644
--- a/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
+++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.exp
@@ -22,6 +22,25 @@
 # 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
@@ -42,8 +61,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 +76,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"
diff --git a/gdb/testsuite/gdb.reverse/step-precsave.exp b/gdb/testsuite/gdb.reverse/step-precsave.exp
index 3279b6ce879..602dd7e6976 100644
--- a/gdb/testsuite/gdb.reverse/step-precsave.exp
+++ b/gdb/testsuite/gdb.reverse/step-precsave.exp
@@ -76,6 +76,7 @@ gdb_test "record restore $precsave" \
 
 # plain vanilla step/next (no count)
 
+gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
 gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
 gdb_test "step" ".*STEP TEST 1.*" "step test 1"
 
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.c b/gdb/testsuite/gdb.reverse/step-reverse.c
index 809c7d16dc9..83704b42b6b 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.c
+++ b/gdb/testsuite/gdb.reverse/step-reverse.c
@@ -54,6 +54,7 @@ large_struct_by_value (struct rhomboidal r)
 int main () {
    int w,x,y,z;
    int a[10], b[10];
+   int (*funp) (void) = callee;
 
    /* Test "next" and "step" */
    w = 0;	/* BREAK AT MAIN */
@@ -83,6 +84,14 @@ int main () {
 
    y = w + z;
 
+   /* 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 */
+
    {
      struct rhomboidal r;
      memset (r.rather_large, 0, sizeof (r.rather_large));
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index d2975cffb5c..bce137a97ad 100644
--- a/gdb/testsuite/gdb.reverse/step-reverse.exp
+++ b/gdb/testsuite/gdb.reverse/step-reverse.exp
@@ -40,6 +40,7 @@ if [supports_process_record] {
 
 # plain vanilla step/next (no count)
 
+gdb_test "next" ".*BREAK AT MAIN.*" "next past funp declaration"
 gdb_test "next" ".*NEXT TEST 1.*" "next test 1"
 gdb_test "step" ".*STEP TEST 1.*" "step test 1"
 
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-22 16:53       ` Carl Love
@ 2022-11-23  8:44         ` Bruno Larsen
  2022-11-23 17:56         ` Ulrich Weigand
  1 sibling, 0 replies; 17+ messages in thread
From: Bruno Larsen @ 2022-11-23  8:44 UTC (permalink / raw)
  To: Carl Love, gdb-patches; +Cc: Ulrich Weigand, Will Schmidt

On 22/11/2022 17:53, Carl Love wrote:
> PowerPC, 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.
>
> The tests were modified to call the function with a function pointer so
> the test 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.
>
> 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.

Regressions have been fixed and style looks good.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Let's hope a maintainer shows up soon :)

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-22 16:53       ` Carl Love
  2022-11-23  8:44         ` Bruno Larsen
@ 2022-11-23 17:56         ` Ulrich Weigand
  2022-11-23 23:33           ` Carl Love
  2022-11-28 18:52           ` Carl Love
  1 sibling, 2 replies; 17+ messages in thread
From: Ulrich Weigand @ 2022-11-23 17:56 UTC (permalink / raw)
  To: gdb-patches, blarsen, cel; +Cc: will_schmidt

Carl Love <cel@us.ibm.com> wrote:

>I have updated the patch, below.  If you could take a quick look and
>make sure it works on your system that would be great.  Assuming it is
>OK, hopefully we can get a maintainer to bless it and we will get the
>patch committed.

This approach makes sense to me.  The one thing that may not be
quite optimal is that having finish-reverse-bkpt.exp use the same
source file as finish-reverse.exp starts looking to be more trouble
than it may be worth.

Would it make sense to create a new finish-reverse-bkpt.c file
that contains just the stripped-down test case (using the function
pointer) required to do the "break *callee" test?

Same for next-reverse-bpkt-over-sr.exp vs. step-reverse.c.

Bye,
Ulrich


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-23 17:56         ` Ulrich Weigand
@ 2022-11-23 23:33           ` Carl Love
  2022-11-28 18:52           ` Carl Love
  1 sibling, 0 replies; 17+ messages in thread
From: Carl Love @ 2022-11-23 23:33 UTC (permalink / raw)
  To: Ulrich Weigand, gdb-patches, blarsen; +Cc: will_schmidt, cel

On Wed, 2022-11-23 at 17:56 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > I have updated the patch, below.  If you could take a quick look
> > and
> > make sure it works on your system that would be great.  Assuming it
> > is
> > OK, hopefully we can get a maintainer to bless it and we will get
> > the
> > patch committed.
> 
> This approach makes sense to me.  The one thing that may not be
> quite optimal is that having finish-reverse-bkpt.exp use the same
> source file as finish-reverse.exp starts looking to be more trouble
> than it may be worth.
> 
> Would it make sense to create a new finish-reverse-bkpt.c file
> that contains just the stripped-down test case (using the function
> pointer) required to do the "break *callee" test?
> 
> Same for next-reverse-bpkt-over-sr.exp vs. step-reverse.c.

Splitting the tests off to their own source file would be fine.  I will
work on that.  Thanks for the review.

                       Carl 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-23 17:56         ` Ulrich Weigand
  2022-11-23 23:33           ` Carl Love
@ 2022-11-28 18:52           ` Carl Love
  2022-11-29  8:52             ` Bruno Larsen
  1 sibling, 1 reply; 17+ messages in thread
From: Carl Love @ 2022-11-28 18:52 UTC (permalink / raw)
  To: Ulrich Weigand, gdb-patches, blarsen; +Cc: will_schmidt, cel

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.
---
 .../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 ()
+{
+  void_test = 1;		/* VOID FUNC */
+}
+
+int main (int argc, char **argv)
+{
+  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
 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 */
+  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 */
+
+   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"
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-28 18:52           ` Carl Love
@ 2022-11-29  8:52             ` Bruno Larsen
  2022-11-29 16:50               ` Carl Love
  2022-11-29 16:57               ` [PATCH V4] " Carl Love
  0 siblings, 2 replies; 17+ messages in thread
From: Bruno Larsen @ 2022-11-29  8:52 UTC (permalink / raw)
  To: Carl Love, Ulrich Weigand, gdb-patches; +Cc: will_schmidt

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"


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-29  8:52             ` Bruno Larsen
@ 2022-11-29 16:50               ` Carl Love
  2022-11-29 16:57               ` [PATCH V4] " Carl Love
  1 sibling, 0 replies; 17+ messages in thread
From: Carl Love @ 2022-11-29 16:50 UTC (permalink / raw)
  To: Bruno Larsen, Ulrich Weigand, gdb-patches; +Cc: will_schmidt

Bruno:

On Tue, 2022-11-29 at 09:52 +0100, Bruno Larsen wrote:

<snip> 
> 
> > 
> > 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>
> 
> > ---
> > 
<snip>

> > +   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 ()
> {
Fixed
> 
> 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.

Yup, I know better.  Because I am used to doing it in line, it doesn't
jump out at me a wrong.  I have to actually stop and think about it to
see it.  Sigh hard to teach an "old dog" new tricks.  :-).  I fixed up
the four occurrences of the mistake.


> > +{
> > +  void_test = 1;		/* VOID FUNC */
> > +}
> > +
> > +int main (int argc, char **argv)
> same here.

Fixed. 
> > +{
> > +  int i;
> > +  void (*funp) (void) = void_func;
> > +
> > 
<snip>
> > set breakpoint on funp"
> > +gdb_continue_to_breakpoint "funp call" ".*$srcfile:$breakloc.*"
> > +
> > +# Start the test
> missing period at the end of the comment.

Yup, fixed.

> >   set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
> >   gdb_test "tbreak void_func" \
> > 
<snip>

> > t myglob = 0;
> > +
> > +int callee() {		/* ENTER CALLEE */
> Same comment about C coding style.

Fixed.

> > +  return myglob++;	/* ARRIVED IN CALLEE */
> > +}			/* RETURN FROM CALLEE */
> > +
> > +int main () {

Fixed

> > +   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.

True, it is only used as a reference point to break on.  The exit (0)
statement could be used instead.  My first thought was to let leave it
as it was.  But after thinking about it for awhile, I decided it was
best to remove it since the comment "STEP INTO CALL AFTER FUNP CALL" is
technically misleading as the test doesn't actually step into callee().
So, in the end I decided to remove the callee (); line and fix the
expect file to break on the following exit (0) call.

Retested both test cases on Power to make sure I didn't have any typos.

> As I said before, these are all minor style comments, so I don't
> think a 
> new version is needed for these comments :)

Given the above functional change, I need to post the updated version. 
I did go ahead and add the reviewed by tag.

                        Carl 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH V4] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-29  8:52             ` Bruno Larsen
  2022-11-29 16:50               ` Carl Love
@ 2022-11-29 16:57               ` Carl Love
  2022-11-30 11:30                 ` Ulrich Weigand
  1 sibling, 1 reply; 17+ messages in thread
From: Carl Love @ 2022-11-29 16:57 UTC (permalink / raw)
  To: Bruno Larsen, Ulrich Weigand, gdb-patches; +Cc: will_schmidt, cel

Bruno, GDB maintainers:

Version 4, fixed the style comments on function declarations, missing
period mentioned by Bruno.  Also, removed the callee () call in next-
reverse-bkpt-over-sr.c since it isn't really needed and the comment is
misleading since the test doesn't actually step into callee (). 
Retested on PowerPC to make sure there were no typos.

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 4, 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.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
---
 .../gdb.reverse/finish-reverse-bkpt.c         | 39 +++++++++++++++++
 .../gdb.reverse/finish-reverse-bkpt.exp       | 32 +++++++++++++-
 .../gdb.reverse/next-reverse-bkpt-over-sr.c   | 43 +++++++++++++++++++
 .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 33 +++++++++++---
 4 files changed, 140 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..5dacd2c9be3
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c
@@ -0,0 +1,39 @@
+/* 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 ()
+{
+  void_test = 1;		/* VOID FUNC */
+}
+
+int
+main (int argc, char **argv)
+{
+  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..98549dce859 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.
 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..4f96825dd8b
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c
@@ -0,0 +1,43 @@
+/* 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 */
+  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 */
+
+   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..fa02a5faca9 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 "end of main"]
+gdb_test "advance $lineno" ".*end of main.*" \
+    "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"
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH V4] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-29 16:57               ` [PATCH V4] " Carl Love
@ 2022-11-30 11:30                 ` Ulrich Weigand
  2022-12-01  1:33                   ` Carl Love
  0 siblings, 1 reply; 17+ messages in thread
From: Ulrich Weigand @ 2022-11-30 11:30 UTC (permalink / raw)
  To: gdb-patches, blarsen, cel; +Cc: will_schmidt

Carl Love <cel@us.ibm.com> wrote:

>+# 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.*"

Do we still need this?  In the new source file, there is only
a single call to void_func (via function pointer), so the
tbreak below should already stop there:

> set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
> gdb_test "tbreak void_func" \
>     "Temporary breakpoint $decimal at .*$srcfile, line $breakloc\." \


>+int
>+callee() {		/* ENTER CALLEE */
>+  return myglob++;	/* ARRIVED IN CALLEE */
>+}			/* RETURN FROM CALLEE */

These comment markers seem to be nowhere used.

+   exit (0);      /* end of main */

Comment marker should be all-upper-case like the others.

>+# 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.

This comment seems confusing.

Bye,
Ulrich


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH V4] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-11-30 11:30                 ` Ulrich Weigand
@ 2022-12-01  1:33                   ` Carl Love
  2022-12-01 15:50                     ` [PATCH V5] " Carl Love
  0 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2022-12-01  1:33 UTC (permalink / raw)
  To: Ulrich Weigand, gdb-patches, blarsen; +Cc: will_schmidt

On Wed, 2022-11-30 at 11:30 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > +# 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.*"
> 
> Do we still need this?  In the new source file, there is only
> a single call to void_func (via function pointer), so the
> tbreak below should already stop there:

Yes, we can just use the tbreak since we have removed all the stuff
from the source file used by the other tests.  Removed the break on
FUNCTION_PTR.  The label in the source file is also no longer needed,
removed.

> 
> > set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
> > gdb_test "tbreak void_func" \
> >     "Temporary breakpoint $decimal at .*$srcfile, line $breakloc\."
> > \
> > +int
> > +callee() {		/* ENTER CALLEE */
> > +  return myglob++;	/* ARRIVED IN CALLEE */
> > +}			/* RETURN FROM CALLEE */
> 
> These comment markers seem to be nowhere used.

Yes, they are left overs in the original source file and are now not
needed.  Removed.
> 
> +   exit (0);      /* end of main */
> 
> Comment marker should be all-upper-case like the others.

Fixed. 

> 
> > +# 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.
> 
> This comment seems confusing.

Fixed the comment.  

Will repost the patch.

Thanks for the review.

                     Carl


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH V5] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-12-01  1:33                   ` Carl Love
@ 2022-12-01 15:50                     ` Carl Love
  2022-12-01 16:02                       ` Ulrich Weigand
  0 siblings, 1 reply; 17+ messages in thread
From: Carl Love @ 2022-12-01 15:50 UTC (permalink / raw)
  To: Ulrich Weigand, gdb-patches, blarsen; +Cc: will_schmidt

Bruno, GDB maintainers:

Version 5, removed unnecessary break, removed unused comment markers,
make comment marker all caps and fixed comment per Ulrich's feedback.

Version 4, fixed the style comments on function declarations, missing
period mentioned by Bruno.  Also, removed the callee () call in next-
reverse-bkpt-over-sr.c since it isn't really needed and the comment is
misleading since the test doesn't actually step into callee (). 
Retested on PowerPC to make sure there were no typos.

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, 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.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
---
 .../gdb.reverse/finish-reverse-bkpt.c         | 39 +++++++++++++++++
 .../gdb.reverse/finish-reverse-bkpt.exp       | 24 ++++++++++-
 .../gdb.reverse/next-reverse-bkpt-over-sr.c   | 43 +++++++++++++++++++
 .../gdb.reverse/next-reverse-bkpt-over-sr.exp | 31 ++++++++++---
 4 files changed, 130 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..7a954179234
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.c
@@ -0,0 +1,39 @@
+/* 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 ()
+{
+  void_test = 1;		/* VOID FUNC */
+}
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  void (*funp) (void) = void_func;
+
+  funp ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp b/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
index 2a204748d98..722f0206e09 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,7 @@ if [supports_process_record] {
     gdb_test_no_output "record" "turn on process record"
 }
 
+# Start the test.
 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..6d0ed521fc3
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/next-reverse-bkpt-over-sr.c
@@ -0,0 +1,43 @@
+/* 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() {
+  return myglob++;
+}
+
+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 */
+
+   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..f6e1d8b9b2f 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,10 @@ 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"
+# Stop after the function pointer call to test the reverse-next command.
+set lineno [gdb_get_line_number "END OF MAIN"]
+gdb_test "advance $lineno" ".*END OF MAIN.*" \
+    "get past callee call"
 
 gdb_test "b \*callee" "" "set breakpoint at callee's entry"
 
@@ -53,5 +72,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"
-- 
2.37.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH V5] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp
  2022-12-01 15:50                     ` [PATCH V5] " Carl Love
@ 2022-12-01 16:02                       ` Ulrich Weigand
  0 siblings, 0 replies; 17+ messages in thread
From: Ulrich Weigand @ 2022-12-01 16:02 UTC (permalink / raw)
  To: gdb-patches, blarsen, cel; +Cc: will_schmidt

Carl Love <cel@us.ibm.com> wrote:

>PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp

This version is OK.

Thanks,
Ulrich



^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-12-01 16:03 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 16:14 [PATCH] Fix test next-reverse-bkpt-over-sr.exp Carl Love
2022-09-28  7:35 ` Bruno Larsen
2022-11-14 21:05   ` [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp Carl Love
2022-11-21 16:36     ` [PING] " Carl Love
2022-11-22  9:42     ` Bruno Larsen
2022-11-22 16:53       ` Carl Love
2022-11-23  8:44         ` Bruno Larsen
2022-11-23 17:56         ` Ulrich Weigand
2022-11-23 23:33           ` Carl Love
2022-11-28 18:52           ` Carl Love
2022-11-29  8:52             ` Bruno Larsen
2022-11-29 16:50               ` Carl Love
2022-11-29 16:57               ` [PATCH V4] " Carl Love
2022-11-30 11:30                 ` Ulrich Weigand
2022-12-01  1:33                   ` Carl Love
2022-12-01 15:50                     ` [PATCH V5] " Carl Love
2022-12-01 16:02                       ` Ulrich Weigand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).