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

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

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

This looks generally OK to me, except for two minor issues
in the code base, and one problem in the test suite:


>diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>index 77206fcbfe8..a65cc700fc9 100644
>--- a/gdb/infcmd.c
>+++ b/gdb/infcmd.c
>@@ -1728,28 +1728,41 @@ finish_backward (struct finish_command_fsm *sm)
>      no way that a function up the stack can have a return address
>      that's equal to its entry point.  */
> 
>-  if (sal.pc != pc)
>-    {
>-      frame_info_ptr frame = get_selected_frame (nullptr);
>-      struct gdbarch *gdbarch = get_frame_arch (frame);
>+  CORE_ADDR alt_entry_point = sal.pc;
>+  CORE_ADDR entry_point = alt_entry_point;
>+  frame_info_ptr frame = get_selected_frame (nullptr);
>+  struct gdbarch *gdbarch = get_frame_arch (frame);
> 
>-      /* Set a step-resume at the function's entry point.  Once that's
>-	 hit, we'll do one more step backwards.  */
>-      symtab_and_line sr_sal;
>-      sr_sal.pc = sal.pc;
>-      sr_sal.pspace = get_frame_program_space (frame);
>-      insert_step_resume_breakpoint_at_sal (gdbarch,
>-					    sr_sal, null_frame_id);
>+  if (gdbarch_skip_entrypoint_p (gdbarch))
>+    /* Some architectures, like PowerPC use local and global entry points.
>+       There is only one Entry Point (GEP = LEP) for other architectures.
>+       The GEP is an alternate entry point.  The LEP is the normal entry point.
>+       The value of entry_point was initialized to the alternate entry point
>+       (GEP).  It will be adjusted to the normal entry point if the function
>+       has two entry points.  */
>+    entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc);
>+
>+  if  ((pc >= alt_entry_point) && (pc <= entry_point))
>+    /* We are either at one of the entry points or between the entry points.
>+       If we are not at the alt_entry point, go back to the alt_entry_point
>+       If we at the normal entry point step back one instruction, when we
>+       stop we will determine if we entered via the entry point or the
>+       alternate entry point.  If we are at the alternate entry point,
>+       single step back to the function call.  */
>+    tp->control.step_range_start = tp->control.step_range_end = 1;
> 
>-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>-    }
>   else
>     {
>-      /* We're almost there -- we just need to back up by one more
>-	 single-step.  */
>-      tp->control.step_range_start = tp->control.step_range_end = 1;
>-      proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>+      /* We are in the body of the function.  Set a breakpoint to backup to
>+	 the normal entry point.  */
>+      symtab_and_line sr_sal;
>+      sr_sal.pc = entry_point;
>+      sr_sal.pspace = get_frame_program_space (frame);
>+      insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
>+					    null_frame_id);
>     }
>+
>+  proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> }

This change looks much larger than it actually is, because you've
swapped the order of the if vs. else blocks (the orignal code sets
the breakpoint in the if path and the single-step range in the
else path, while you've swapped this).  Can you swap this back to
shorten the diff?

>+  if (execution_direction == EXEC_REVERSE
>+      && ecs->event_thread->control.proceed_to_finish)
>+    {
>+      /* We are executing the reverse-finish command.  */
>+      if (ecs->event_thread->stop_pc () >= ecs->stop_func_alt_start
>+	  && ecs->event_thread->stop_pc () < ecs->stop_func_start
>+	  && ecs->stop_func_alt_start != ecs->stop_func_start)

This third condition seems redundant: if stop_pc is >= func_alt_start
*and* < func_start, then func_alt_start cannot be equal func_start.


>diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>index 94292d5eb9b..dc5cf097511 100644
>--- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>+++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
>@@ -36,39 +36,6 @@ if { ![runto_main] } {
>     return -1
> }
> 
>-# Do repeated stepping COMMANDs in order to reach TARGET from CURRENT
>-#
>-#  COMMAND is a stepping command
>-#  CURRENT is a string matching the current location
>-#  TARGET  is a string matching the target location
>-#  TEST    is the test name
>-#
>-# The function issues repeated COMMANDs as long as the location matches
>-# CURRENT up to a maximum of 100 steps.
>-#
>-# TEST passes if the resulting location matches TARGET and fails
>-# otherwise.
>-#
>-proc step_until { command current target test } {
>-    global gdb_prompt
>-
>-    set count 0
>-    gdb_test_multiple "$command" "$test" {
>-        -re "$current.*$gdb_prompt $" {
>-            incr count
>-            if { $count < 100 } {
>-                send_gdb "$command\n"
>-                exp_continue
>-            } else {
>-                fail "$test"
>-            }
>-        }
>-        -re "$target.*$gdb_prompt $" {
>-            pass "$test"
>-        }
>-    }
>-}
>-
> gdb_test_no_output "record"
> gdb_test "next" ".*" "record trace"


How is it OK to just remove this procedure?  This is still used
in the rest of the step-indirect-call-thunk.exp test case ...

I get why you may want to reduce duplication, but then you'd
have to update current users of "step_until" as well.

Also, talking about duplication, should the (already separate)
gdb_step_until command in lib/gdb.exp now just be a variant
of the new repeat_cmd_until ?


Bye,
Ulrich