Re: [RFA] Resubmit reverse debugging [4/5]

[Michael Snyder <msnyder at vmware dot com>]

Pedro Alves wrote:
> A Thursday 09 October 2008 02:18:08, Michael Snyder wrote:
> > Sorry, it's an artifact of the fact that I've been on a
> > fork for so long.  When I copied this code from finish_command,
> > the code that I copied had a similar call to internal_error.
> >
> > In fact, finish_command_continuation still does.
>
> Yeah, the continuation has a check for `function != NULL',
> though.
>
> > In fact, it's the same call that used to be in "finish_command".
> >
> > So what should it be?  Just "error"?
>
> Ah, I think I see what's going on.  "finish" is not meaningful
> in the outermost frame, so, you'd get an error before reaching
> here, if you had no symbols.
>
>  (gdb) finish
>  "finish" not meaningful in the outermost frame.
>  (gdb) reverse-finish
>  "finish" not meaningful in the outermost frame.
>
> Is it possible to be at frame != #0 and not find a function?

I have no idea, I was just checking the return value
on general principals.

[sucking out "frame 0" discussion for separate reply]

> > I think I understand that you think it would be more "local"
> > to put the error here -- but is it worth it if it makes us
> > add complexity?
> >
> > finish_command already tests a number of things, including
> > whether we are async and (now) whether we are reverse, and
> > contains a number of error calls already.
>
> No biggie with me.  Just thought you had signed up to do
> the function split.  ;-)

Well I didn't, but I will.  How does this look to you
(as an add-on to the present patch)?

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.212.2.7
diff -u -p -r1.212.2.7 infcmd.c
--- infcmd.c	8 Oct 2008 00:26:28 -0000	1.212.2.7
+++ infcmd.c	9 Oct 2008 02:32:33 -0000
@@ -1370,9 +1370,10 @@ finish_command_continuation_free_arg (vo
 /* finish_backward -- helper function for finish_command.  */
 
 static void
-finish_backward (struct symbol *function, struct thread_info *tp)
+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 func_addr;
@@ -1385,8 +1386,6 @@ finish_backward (struct symbol *function
 
   sal = find_pc_line (func_addr, 0);
 
-  /* TODO: Let's not worry about async until later.  */
-
   /* We don't need a return value.  */
   tp->proceed_to_finish = 0;
   /* Special case: if we're sitting at the function entry point,
@@ -1421,26 +1420,56 @@ finish_backward (struct symbol *function
       /* 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.  */
-      /* (Kludgy way of letting wait_for_inferior know...) */
       tp->step_range_start = tp->step_range_end = 1;
       proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
     }
   return;
 }
 
+/* finish_forward -- helper function for finish_command.  */
+
+static void
+finish_forward (struct symbol *function, struct frame_info *frame)
+{
+  struct symtab_and_line sal;
+  struct thread_info *tp = inferior_thread ();
+  struct breakpoint *breakpoint;
+  struct cleanup *old_chain;
+  struct finish_command_continuation_args *cargs;
+
+  sal = find_pc_line (get_frame_pc (frame), 0);
+  sal.pc = get_frame_pc (frame);
+
+  breakpoint = set_momentary_breakpoint (sal, get_frame_id (frame),
+                                         bp_finish);
+
+  old_chain = make_cleanup_delete_breakpoint (breakpoint);
+
+  tp->proceed_to_finish = 1;    /* We want stop_registers, please...  */
+  make_cleanup_restore_integer (&suppress_stop_observer);
+  suppress_stop_observer = 1;
+  proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
+
+  cargs = xmalloc (sizeof (*cargs));
+
+  cargs->breakpoint = breakpoint;
+  cargs->function = function;
+  add_continuation (tp, finish_command_continuation, cargs,
+                    finish_command_continuation_free_arg);
+
+  discard_cleanups (old_chain);
+  if (!target_can_async_p ())
+    do_all_continuations ();
+}
+
 /* "finish": Set a temporary breakpoint at the place the selected
    frame will return to, then continue.  */
 
 static void
 finish_command (char *arg, int from_tty)
 {
-  struct symtab_and_line sal;
   struct frame_info *frame;
   struct symbol *function;
-  struct breakpoint *breakpoint;
-  struct cleanup *old_chain;
-  struct finish_command_continuation_args *cargs;
-  struct thread_info *tp;
 
   int async_exec = 0;
 
@@ -1474,8 +1503,6 @@ finish_command (char *arg, int from_tty)
   if (frame == 0)
     error (_("\"finish\" not meaningful in the outermost frame."));
 
-  tp = inferior_thread ();
-
   clear_proceed_status ();
 
   /* Find the function we will return from.  */
@@ -1495,35 +1522,9 @@ finish_command (char *arg, int from_tty)
     }
 
   if (execution_direction == EXEC_REVERSE)
-    {
-      /* Split off at this point.  */
-      finish_backward (function, tp);
-      return;
-    }
-
-  sal = find_pc_line (get_frame_pc (frame), 0);
-  sal.pc = get_frame_pc (frame);
-
-  breakpoint = set_momentary_breakpoint (sal, get_frame_id (frame), 
-					 bp_finish);
-
-  old_chain = make_cleanup_delete_breakpoint (breakpoint);
-
-  tp->proceed_to_finish = 1;	/* We want stop_registers, please...  */
-  make_cleanup_restore_integer (&suppress_stop_observer);
-  suppress_stop_observer = 1;
-  proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
-
-  cargs = xmalloc (sizeof (*cargs));
-
-  cargs->breakpoint = breakpoint;
-  cargs->function = function;
-  add_continuation (tp, finish_command_continuation, cargs,
-		    finish_command_continuation_free_arg);
-
-  discard_cleanups (old_chain);
-  if (!target_can_async_p ())
-    do_all_continuations ();
+    finish_backward (function);
+  else
+    finish_forward (function, frame);
 }