RE: [PATCH 2/2 ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

[Carl Love <cel@us.ibm.com>]

Bruno:

On Fri, 2023-07-21 at 09:24 +0200, Bruno Larsen wrote:
> > I think what Bruno is again asking is to have
> > control.step_range_start
> > set to the real start range initially, i.e. what Simon asked
> > about.  I
> > think to do that, we would need to make significant changes to how
> > reverse execution works to allow us to make that change.  It didn't
> > appear to be a straight forward fix to me.  I may be wrong.  Maybe
> > someone sees a good way to make that work that I am missing.  So it
> > looks like this patch fixes most issues but not all of the problems
> > with reverse-step and reverse-next.  It might be good to try and
> > fix
> > this additional scenario in a separate patch, not sure???
> 
> I just tested with the newest version of the patch relating to
> reverse 
> stepping over contiguous lines, but that didn't make a difference.
> GCC 
> continued broken. I think the problem is that GCC doesn't have 
> contiguous ranges, it has a single range that doesn't contain the PC
> 
> My (probably naive) thinking is to set step_range_end to the starting
> PC 
> of the following line, and step_range_start to be the real start of
> the 
> current line at the moment the command is parsed. This sounds like
> it 
> would solve the GCC problem, but I assume I'm missing something. 
> Unfortunately, I don't have the time to test this theory myself, but 
> I'll happily test any patches you submit :). If you want to repeat
> my 
> test, I want the behavior of gdb.reverse/finish-reverse-next from gcc
> to 
> match clang's (on x86 machines), that is, if you reverse finish from 
> function1, a single "rn" command would land you on the b=5 line.

So, I took the example of the error from your previous patch where you
demonstrated the scenario where you have stopped at the call to the
function, then do the reverse-next which stops at the beginning of the
same line and put it into a new test case for discussion and for
debugging the issue.  Nice to have a simple example of the issue.  The
new test is bruno_7_20_2023.exp.  So, in this test, when we are stopped
at the call, as you showed on Intel

(gdb) maint info line-table
>              (snip)
> 15     81     0x0000000000401185 0x0000000000401185 Y
> 16     83     0x000000000040118c 0x000000000040118c Y
> 17     86     0x000000000040119b 0x000000000040119b Y
> (gdb) disas /s
> Dump of assembler code for function main:
>        (snip)
> 83        function1 (a, b);   // CALL VIA LEP
>     0x000000000040118c <+37>:    mov    -0x10(%rbp),%edx
>     0x000000000040118f <+40>:    mov    -0xc(%rbp),%eax
>     0x0000000000401192 <+43>:    mov    %edx,%esi
>     0x0000000000401194 <+45>:    mov    %eax,%edi
> => 0x0000000000401196 <+47>:    call   0x40112c <function1>
> 
> We can see that we have stopped at the right instruction, but it
> isn't 
> mapped to any line number directly. If we reverse-next from there:

the tp->control.step_range_start is set to 0x40118c in function
prepare_one_step() in gdb/infcmd.c.  The PC in your case is at
0x401196.  Of course both of the addresses are in the same line.  We
are executing backwards so we need step_range_start to be set to a
value in the previous line.  In general, we have no idea what the
previous line was as we may have arrived on this line via a jump or
branch in theory.  Also, note, when setting the range we need to have
the PC in the range or GDB will give us an assert error.

So really the issue is that we want the step_range_start to be in the
line where we came from.  So, if we see the line number for PC and
step_range_start are the same, we can set step_range_start to one less
then the address of the beginning of the current line when we parse the
instruction and setup the stepping range.  The goal is to have GDB stop
in the previous line and not at the beginning of the line and thus
match the behavior of gdb on clang.

I implemented the check and adjustment for step_range_start as just
described and tested it with your example, bruno_7_20_2023.exp and it
does seem to work as desired.

The existing testcases expect to have to do two reverse step/next
instructions to reach the previous line.  So, we need to fix the
existing tests gdb.reverse/finish-reverse-next.exp and gdb.mi/mi-
reverse.exp to remove the extra reverse step/next command.

Anyway, I put the gdb code fix, the bruno_7_20_2023.exp and test fixes
into the patch below for discussion purposes.  If everyone is happy
with the gdb change and test changes (we can drop the Bruno test) I can
merge these changes into the patch. Hopefully keeping these changes in
a separate patch for the moment will make it easier to see what is
being changed.

If you were to apply the following patch on top of the currently
proposed patch, does gdb then execute as you expect on your system?  I
have tested the change on IBM Power 10.  It would be good to make sure
it works on your X86 system as well.  I also ran the full gdb
regression tests on Power 10 with no additional regression failures.
Thanks for looking at the patch, proposed fix and the additional
testing.

                          Carl 
---------------------------------------------------------------
[PATCH 2/2] finish-reverse-next new test case from bruno.

GDB fix for Bruno test, updated other tests for the gdb fix.
---
 gdb/infcmd.c                                  |  9 ++
 gdb/testsuite/gdb.mi/mi-reverse.exp           |  6 +-
 gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp | 85 +++++++++++++++++++
 .../gdb.reverse/finish-reverse-next.exp       | 44 +++++++---
 4 files changed, 133 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b8134665f3f..c09b3edad3d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -981,6 +981,15 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
 	  find_pc_line_pc_range (pc,
 				 &tp->control.step_range_start,
 				 &tp->control.step_range_end);
+	  symtab_and_line sal = find_pc_line (pc, 0);
+	  symtab_and_line sal_start
+	    = find_pc_line (tp->control.step_range_start, 0);
+
+	  if (sal.line == sal_start.line)
+	    /* The step_range_start address is in the same line.  We want to
+	       stop in the previous line so move step_range_start one
+	       instruction earlier.  */
+	    tp->control.step_range_start--;
 
 	  /* There's a problem in gcc (PR gcc/98780) that causes missing line
 	     table entries, which results in a too large stepping range.
diff --git a/gdb/testsuite/gdb.mi/mi-reverse.exp b/gdb/testsuite/gdb.mi/mi-reverse.exp
index baa53a495d7..997309cfb71 100644
--- a/gdb/testsuite/gdb.mi/mi-reverse.exp
+++ b/gdb/testsuite/gdb.mi/mi-reverse.exp
@@ -99,8 +99,12 @@ proc test_controlled_execution_reverse {} {
     #   It takes two steps to get back to the previous line,
     #   as the first step moves us to the start of the current line,
     #   and the one after that moves back to the previous line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
 
-    mi_execute_to "exec-next --reverse 2" \
+    mi_execute_to "exec-next --reverse" \
  	"end-stepping-range" "main" "" \
  	"basics.c" $line_main_hello "" \
  	"reverse next to get over the call to do_nothing"
diff --git a/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp
new file mode 100644
index 00000000000..0f19a959043
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/bruno_7_20_2023.exp
@@ -0,0 +1,85 @@
+# Copyright 2008-2023 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/>.  */
+
+# This file is part of the GDB testsuite.  It tests reverse stepping.
+# Lots of code borrowed from "step-test.exp".
+
+# The reverse finish command should return from a function and stop on
+# the first instruction of the source line where the function call is made.
+# Specifically, the behavior should match doing a reverse next from the
+# first instruction in the function.  GDB should only take one reverse step
+# or next statement to reach the previous source code line.
+
+# This testcase verifies the reverse-finish command stops at the first
+# instruction in the source code line where the function was called.  There
+# are two scenarios that must be checked:
+#   1) gdb is at the entry point instruction for the function
+#   2) gdb is in the body of the function.
+
+# This test verifies the fix for gdb bugzilla:
+#   https://sourceware.org/bugzilla/show_bug.cgi?id=29927
+
+# PowerPC supports two entry points to a function.  The normal entry point
+# is called the local entry point (LEP).  The alternate entry point is called
+# the global entry point (GEP).  A function call via a function pointer
+# will entry via the GEP.  A normal function call will enter via the LEP.
+#
+# This test has been expanded to include tests to verify the reverse-finish
+# command works properly if the function is called via the GEP.  The original
+# test only verified the reverse-finish command for a normal call that used
+# the LEP.
+
+if ![supports_reverse] {
+    return
+}
+
+standard_testfile finish-reverse-next.c
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+runto_main
+set target_remote [gdb_is_target_remote]
+
+if [supports_process_record] {
+    # Activate process record/replay.
+    gdb_test_no_output "record" "turn on process record for test1"
+}
+
+
+# until 83
+# Set breakpoint at call to function1 in main.
+set bp_LEP_test [gdb_get_line_number "CALL VIA LEP" $srcfile]
+gdb_breakpoint $srcfile:$bp_LEP_test temporary
+
+# Continue to break point at function1 call in main.
+gdb_continue_to_breakpoint \
+    "stopped at function1 entry point instruction to stepi into function" \
+    ".*$srcfile:$bp_LEP_test\r\n.*"
+
+gdb_test "next" ".*a = 10;.*" \
+    "next from function body function1 LEP to a = 10;"
+
+gdb_test "reverse-step" ".*70.*}.*" \
+    "reverse step from a = 10, stops at last inst in function1 call"
+
+gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+    "reverse-finish should stop at last inst in line for function1"
+
+gdb_test "reverse-next" ".*b = 5;.*" \
+    "reverse-finish should stop at b = 5, not first inst in line for function1"
+
+
diff --git a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
index 1f53b649a7d..921d0051233 100644
--- a/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
+++ b/gdb/testsuite/gdb.reverse/finish-reverse-next.exp
@@ -80,10 +80,15 @@ repeat_cmd_until "stepi" "CALL VIA LEP" "{" "stepi into function1 call" "100"
 # instruction should then stop at the first instruction in the same source
 # code line.  Another revers-next instruction stops at the previous source
 # code line.
+
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
 gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
     "reverse-finish function1 LEP call from LEP "
-gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP" \
-    "reverse next 1 LEP entry point function call from LEP"
+##gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP" \
+##    "reverse next 1 LEP entry point function call from LEP"
 gdb_test "reverse-next" ".*b = 5;.*" "reverse next 2, at b = 5, call from LEP"
 
 
@@ -113,10 +118,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 1"
 # instruction should then stop at the first instruction in the same source
 # code line.  Another revers-next instruction stops at the previous source
 # code line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
 gdb_test "reverse-finish" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
     "reverse-finish function1 LEP call from function body"
-gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
-    "reverse next 1 LEP from function body"
+## gdb_test "reverse-next" ".*function1 \\(a, b\\);   // CALL VIA LEP.*" \
+##     "reverse next 1 LEP from function body"
 gdb_test "reverse-next" ".*b = 5;.*" \
     "reverse next 2 at b = 5, from function body"
 
@@ -148,10 +157,15 @@ repeat_cmd_until "stepi" "CALL VIA GEP" "{" "stepi into funp call"
 # instruction should then stop at the first instruction in the same source
 # code line.  Another revers-next instruction stops at the previous source
 # code line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
+
 gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
     "function1 GEP call call from GEP"
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
-    "reverse next 1 GEP entry point function call from GEP"
+## gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+##    "reverse next 1 GEP entry point function call from GEP"
 gdb_test "reverse-next" ".*b = 50;.*" "reverse next 2 at b = 50, call from GEP"
 
 gdb_test "reverse-continue" ".*" "setup for test 4"
@@ -184,10 +198,15 @@ gdb_test "stepi" "{" "stepi to between GEP and LEP"
 # instruction should then stop at the first instruction in the same source
 # code line.  Another revers-next instruction stops at the previous source
 # code line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
+
 gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
     "function1 GEP call call from GEP again"
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
-    "reverse next 1 GEP entry point function call from GEP again"
+##gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+##    "reverse next 1 GEP entry point function call from GEP again"
 gdb_test "reverse-next" ".*b = 50;.*" \
     "reverse next 2 at b = 50, call from GEP again"
 
@@ -216,9 +235,14 @@ gdb_test "step" ".*int ret = 0;.*" "step test 2"
 # instruction should then stop at the first instruction in the same source
 # code line.  Another revers-next instruction stops at the previous source
 # code line.
+## I BELIEVE Bruno is arguing this part of the test is wrong.  When we are
+## stopped at the call, which is the last instruction in the line, the
+## reverse next should take us directly to the previous line, not stop at
+## the first instruction in the same line.
+
 gdb_test "reverse-finish" ".*funp \\(a, b\\);.*" \
     "reverse-finish function1 GEP call, from function body  "
-gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
-    "reverse next 1 GEP call from function body"
+## gdb_test "reverse-next" ".*funp \\(a, b\\);.*" \
+##    "reverse next 1 GEP call from function body"
 gdb_test "reverse-next" ".*b = 50;.*" \
     "reverse next 2 at b = 50 from function body"
-- 
2.37.2