fix reverse-finish bugs, make it async-ready

[Pedro Alves <pedro at codesourcery dot com>]

Looking to turn async mode on by default, I realized I need to
make reverse debugging work in reverse too (gdb doesn't allow it
currently, just throwing a bailing out error if you try it).

Most of the common code is ready to support it actually,
but reverse-finish needs some fixing first.

It does

 - set momentary breakpoint at function entry
 - call "proceed".
 - if momentary breakpoint was hit, issue another
   step, using "proceed".

This has several problems.  

- if there's a user breakpoint at the same address as
  the momentary breakpoint, it is ignored, and the second
  proceed happens anyway.

- make it async, we'd have to split the second proceed
  into a continuation.

along with other things a full `proceed' does behind the
scenes that aren't appropriate for an intermediate step.

To fix these, instead of a special momentary breakpoint,
implement it similarly to when reverse-next detects a
reverse step into a function call.  Set a step-resume
at the function entry (the same place where the momentary
is currently placed), but let infrun.c:handle_inferior_event
handle the final single-step.

(and it was writing this that I noticed the user breakpoints
on top of step-resume breakpoints getting ignored anyway,
leading to my previous patch).

Since there's only one proceed call, and there's really nothing
else to do afterwards, there's be no need for a continuation
for async mode.

Tested on x86_64-linux (with precord) and applied.

-- 
Pedro Alves

2011-05-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* infcmd.c (finish_backward): Set a step-resume breakpoint at the
	function's entry point instead of a manually managed momentary
	breakpoint, and only ever issue one proceed call.
	* infrun.c (handle_inferior_event) <BPSTAT_WHAT_STEP_RESUME>: If
	doing a reverse-finish, switch to stepi mode, to do another step.
	(insert_step_resume_breakpoint_at_sal): Make public.
	(normal_stop): No need to save function value return registers if
	going reverse.
	* inferior.h (insert_step_resume_breakpoint_at_sal): Declare.

	gdb/testsuite/
	* gdb.reverse/finish-reverse-bkpt.exp: New test.

---
 gdb/infcmd.c                                      |   36 ++++---------
 gdb/inferior.h                                    |    4 +
 gdb/infrun.c                                      |   23 ++++++--
 gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp |   57 ++++++++++++++++++++++
 4 files changed, 90 insertions(+), 30 deletions(-)

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2011-05-26 16:16:29.811255041 +0100
+++ src/gdb/infcmd.c	2011-05-26 16:22:44.151254911 +0100
@@ -1528,11 +1528,8 @@ finish_backward (struct symbol *function
 {
   struct symtab_and_line sal;
   struct thread_info *tp = inferior_thread ();
-  struct breakpoint *breakpoint;
-  struct cleanup *old_chain;
   CORE_ADDR pc;
   CORE_ADDR func_addr;
-  int back_up;
 
   pc = get_frame_pc (get_current_frame ());
 
@@ -1542,8 +1539,7 @@ finish_backward (struct symbol *function
 
   sal = find_pc_line (func_addr, 0);
 
-  /* We don't need a return value.  */
-  tp->control.proceed_to_finish = 0;
+  tp->control.proceed_to_finish = 1;
   /* Special case: if we're sitting at the function entry point,
      then all we need to do is take a reverse singlestep.  We
      don't need to set a breakpoint, and indeed it would do us
@@ -1557,33 +1553,25 @@ finish_backward (struct symbol *function
     {
       struct frame_info *frame = get_selected_frame (NULL);
       struct gdbarch *gdbarch = get_frame_arch (frame);
+      struct symtab_and_line sr_sal;
+
+      /* Set a step-resume at the function's entry point.  Once that's
+	 hit, we'll do one more step backwards.  */
+      init_sal (&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);
 
-      /* Set breakpoint and continue.  */
-      breakpoint =
-	set_momentary_breakpoint (gdbarch, sal,
-				  get_stack_frame_id (frame),
-				  bp_breakpoint);
-      /* Tell the breakpoint to keep quiet.  We won't be done
-         until we've done another reverse single-step.  */
-      breakpoint_set_silent (breakpoint, 1);
-      old_chain = make_cleanup_delete_breakpoint (breakpoint);
       proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
-      /* We will be stopped when proceed returns.  */
-      back_up = (bpstat_find_breakpoint (tp->control.stop_bpstat, breakpoint)
-		 != NULL);
-      do_cleanups (old_chain);
     }
   else
-    back_up = 1;
-  if (back_up)
     {
-      /* If in fact we hit the step-resume breakpoint (and not
-	 some other breakpoint), then we're almost there --
-	 we just need to back up by one more single-step.  */
+      /* 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, TARGET_SIGNAL_DEFAULT, 1);
     }
-  return;
 }
 
 /* finish_forward -- helper function for finish_command.  */
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2011-05-26 16:16:29.811255041 +0100
+++ src/gdb/infrun.c	2011-05-26 16:17:21.581255023 +0100
@@ -103,10 +103,6 @@ static void insert_hp_step_resume_breakp
 
 static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
 
-static void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
-						  struct symtab_and_line ,
-						  struct frame_id);
-
 static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
 
 /* When set, stop the 'step' command if we enter a function which has
@@ -4354,6 +4350,20 @@ process_event_stop_test:
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_STEP_RESUME\n");
 
 	delete_step_resume_breakpoint (ecs->event_thread);
+	if (ecs->event_thread->control.proceed_to_finish
+	    && execution_direction == EXEC_REVERSE)
+	  {
+	    struct thread_info *tp = ecs->event_thread;
+
+	    /* We are finishing a function in reverse, and just hit
+	       the step-resume breakpoint at the start address of the
+	       function, and we're almost there -- just need to back
+	       up by one more single-step, which should take us back
+	       to the function call.  */
+	    tp->control.step_range_start = tp->control.step_range_end = 1;
+	    keep_going (ecs);
+	    return;
+	  }
 	if (stop_pc == ecs->stop_func_start
 	    && execution_direction == EXEC_REVERSE)
 	  {
@@ -5235,7 +5245,7 @@ insert_step_resume_breakpoint_at_sal_1 (
     = set_momentary_breakpoint (gdbarch, sr_sal, sr_id, sr_type);
 }
 
-static void
+void
 insert_step_resume_breakpoint_at_sal (struct gdbarch *gdbarch,
 				      struct symtab_and_line sr_sal,
 				      struct frame_id sr_id)
@@ -5859,7 +5869,8 @@ normal_stop (void)
 
   /* Save the function value return registers, if we care.
      We might be about to restore their previous contents.  */
-  if (inferior_thread ()->control.proceed_to_finish)
+  if (inferior_thread ()->control.proceed_to_finish
+      && execution_direction != EXEC_REVERSE)
     {
       /* This should not be necessary.  */
       if (stop_registers)
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h	2011-05-26 16:16:29.811255041 +0100
+++ src/gdb/inferior.h	2011-05-26 16:17:21.591255023 +0100
@@ -190,6 +190,10 @@ extern void resume (int, enum target_sig
 
 extern ptid_t user_visible_resume_ptid (int step);
 
+extern void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
+						  struct symtab_and_line ,
+						  struct frame_id);
+
 /* From misc files */
 
 extern void default_print_registers_info (struct gdbarch *gdbarch,
Index: src/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.reverse/finish-reverse-bkpt.exp	2011-05-26 16:17:21.591255023 +0100
@@ -0,0 +1,57 @@
+# Copyright 2008, 2009, 2010, 2011 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.
+#
+# 'reverse-finish' used to have a bug where user breakpoints set at
+# the functions entry would be ignored.  Make sure the bug doesn't
+# reappear.
+
+if ![target_info exists gdb,can_reverse] {
+    return
+}
+
+set testfile "finish-reverse-bkpt"
+set srcfile finish-reverse.c
+
+if { [prepare_for_testing $testfile.exp "$testfile" $srcfile] } {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+if [target_info exists gdb,use_precord] {
+    # Activate process record/replay
+    gdb_test_no_output "record" "Turn on process record"
+}
+
+set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
+gdb_test "break void_func" \
+    "Breakpoint $decimal at .*/$srcfile, line $breakloc\." \
+    "set breakpoint on void_func"
+gdb_continue_to_breakpoint "void_func" ".*/$srcfile:$breakloc.*"
+
+gdb_test "break \*void_func" \
+    "Breakpoint $decimal at .*" \
+    "set breakpoint at void_func's entry"
+
+gdb_test "reverse-finish" \
+    ".*Breakpoint .*, void_func.*" \
+    "reverse-finish from void_func trips breakpoint at entry"
+
+gdb_test "frame" "#0  void_func.*" "no spurious proceed after breakpoint stop"