[RFA] Resubmit reverse debugging [4/5]

[RFA] Resubmit reverse debugging [4/5]

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 36 bytes --]

The infcmd and breakpoint changes:


[-- Attachment #2: infcmd.txt --]
[-- Type: text/plain, Size: 5908 bytes --]

2008-10-07  Michael Snyder  <msnyder@vmware.com>

	* breakpoint.c (make_breakpoint_silent): New function.
	* breakpoint.h (make_breakpoint_silent): Export.
	* infcmd.c (finish_command): Check for reverse exec direction.
	(finish_backward): New function, handle finish cmd in reverse.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.352
diff -u -p -r1.352 breakpoint.c
--- breakpoint.c	16 Sep 2008 18:55:01 -0000	1.352
+++ breakpoint.c	8 Oct 2008 01:25:26 -0000
@@ -7741,6 +7741,13 @@ breakpoint_clear_ignore_counts (void)
     b->ignore_count = 0;
 }
 
+void
+make_breakpoint_silent (struct breakpoint *b)
+{
+  /* Silence the breakpoint.  */
+  b->silent = 1;
+}
+
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.79
diff -u -p -r1.79 breakpoint.h
--- breakpoint.h	22 Sep 2008 15:26:53 -0000	1.79
+++ breakpoint.h	8 Oct 2008 01:25:26 -0000
@@ -884,4 +884,7 @@ extern int breakpoints_always_inserted_m
    in our opinion won't ever trigger.  */
 extern void breakpoint_retire_moribund (void);
 
+/* Tell a breakpoint to be quiet.  */
+extern void make_breakpoint_silent (struct breakpoint *);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.212
diff -u -p -r1.212 infcmd.c
--- infcmd.c	22 Sep 2008 15:20:08 -0000	1.212
+++ infcmd.c	8 Oct 2008 01:25:26 -0000
@@ -1366,6 +1366,67 @@ finish_command_continuation_free_arg (vo
   xfree (arg);
 }
 
+/* finish_backward -- helper function for finish_command.  */
+
+static void
+finish_backward (struct symbol *function, struct thread_info *tp)
+{
+  struct symtab_and_line sal;
+  struct breakpoint *breakpoint;
+  struct cleanup *old_chain;
+  CORE_ADDR func_addr;
+  int back_up;
+
+  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
+				NULL, &func_addr, NULL) == 0)
+    internal_error (__FILE__, __LINE__,
+		    _("Finish: couldn't find 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,
+     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
+     no good to do so.
+
+     Note that this can only happen at frame #0, since there's
+     no way that a function up the stack can have a return address
+     that's equal to its entry point.  */
+
+  if (sal.pc != read_pc ())
+    {
+      /* Set breakpoint and continue.  */
+      breakpoint =
+	set_momentary_breakpoint (sal,
+				  get_frame_id (get_selected_frame (NULL)),
+				  bp_breakpoint);
+      /* Tell the breakpoint to keep quiet.  We won't be done
+         until we've done another reverse single-step.  */
+      make_breakpoint_silent (breakpoint);
+      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->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.  */
+      /* (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": Set a temporary breakpoint at the place the selected
    frame will return to, then continue.  */
 
@@ -1391,6 +1452,10 @@ finish_command (char *arg, int from_tty)
   if (async_exec && !target_can_async_p ())
     error (_("Asynchronous execution not supported on this target."));
 
+  /* Don't try to async in reverse.  */
+  if (async_exec && execution_direction == EXEC_REVERSE)
+    error (_("Asynchronous 'finish' not supported in reverse."));
+
   /* If we are not asked to run in the bg, then prepare to run in the
      foreground, synchronously.  */
   if (!async_exec && target_can_async_p ())
@@ -1412,13 +1477,6 @@ finish_command (char *arg, int from_tty)
 
   clear_proceed_status ();
 
-  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);
-
   /* Find the function we will return from.  */
 
   function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
@@ -1427,10 +1485,29 @@ finish_command (char *arg, int from_tty)
      source.  */
   if (from_tty)
     {
-      printf_filtered (_("Run till exit from "));
+      if (execution_direction == EXEC_REVERSE)
+	printf_filtered (_("Run back to call of "));
+      else
+	printf_filtered (_("Run till exit from "));
+
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
     }
 
+  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;

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

[Pedro Alves]

On Wednesday 08 October 2008 03:19:37, Michael Snyder wrote:
> The infcmd and breakpoint changes:

Hmmm, isn't this the same patch as before?  I still see the same
issues.

-- 
Pedro Alves

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

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 08 October 2008 03:19:37, Michael Snyder wrote:
>> The infcmd and breakpoint changes:
> 
> Hmmm, isn't this the same patch as before? 

[checking...]  hmmm, no... The name of the function in
breakpoint.c has been changed, for instance.  Plus this
patch uses the global state variable "execution_direction",
instead of calling the target method to get it.

I've added the error for async in finish_backward.

Put finish_backward above finish_command, so it would not
need a prototype.

Made the i18n changes that you requested.

 > I still see the same issues.

<pout>

OK, like what?

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

[Pedro Alves]

A Wednesday 08 October 2008 03:19:37, Michael Snyder escreveu:
> 2008-10-07  Michael Snyder  <msnyder@vmware.com>
> 
>         * breakpoint.c (make_breakpoint_silent): New function.
>         * breakpoint.h (make_breakpoint_silent): Export.
>         * infcmd.c (finish_command): Check for reverse exec direction.
>         (finish_backward): New function, handle finish cmd in reverse.
> 
> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.352
> diff -u -p -r1.352 breakpoint.c
> --- breakpoint.c        16 Sep 2008 18:55:01 -0000      1.352
> +++ breakpoint.c        8 Oct 2008 01:25:26 -0000
> @@ -7741,6 +7741,13 @@ breakpoint_clear_ignore_counts (void)
>      b->ignore_count = 0;
>  }
>  
> +void
> +make_breakpoint_silent (struct breakpoint *b)
> +{
> +  /* Silence the breakpoint.  */
> +  b->silent = 1;
> +}
> +
>  /* Command to set ignore-count of breakpoint N to COUNT.  */
>  
>  static void
> Index: breakpoint.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.h,v
> retrieving revision 1.79
> diff -u -p -r1.79 breakpoint.h
> --- breakpoint.h        22 Sep 2008 15:26:53 -0000      1.79
> +++ breakpoint.h        8 Oct 2008 01:25:26 -0000
> @@ -884,4 +884,7 @@ extern int breakpoints_always_inserted_m
>     in our opinion won't ever trigger.  */
>  extern void breakpoint_retire_moribund (void);
>  
> +/* Tell a breakpoint to be quiet.  */
> +extern void make_breakpoint_silent (struct breakpoint *);
> +
>  #endif /* !defined (BREAKPOINT_H) */
> Index: infcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcmd.c,v
> retrieving revision 1.212
> diff -u -p -r1.212 infcmd.c
> --- infcmd.c    22 Sep 2008 15:20:08 -0000      1.212
> +++ infcmd.c    8 Oct 2008 01:25:26 -0000
> @@ -1366,6 +1366,67 @@ finish_command_continuation_free_arg (vo
>    xfree (arg);
>  }
>  
> +/* finish_backward -- helper function for finish_command.  */
> +
> +static void
> +finish_backward (struct symbol *function, struct thread_info *tp)
> +{
> +  struct symtab_and_line sal;
> +  struct breakpoint *breakpoint;
> +  struct cleanup *old_chain;
> +  CORE_ADDR func_addr;
> +  int back_up;
> +
> +  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
> +                               NULL, &func_addr, NULL) == 0)
> +    internal_error (__FILE__, __LINE__,
> +                   _("Finish: couldn't find function."));
> +

Still internal_error?

> +  sal = find_pc_line (func_addr, 0);
> +
> +  /* TODO: Let's not worry about async until later.  */
> +

Should be an error here instead of on finish_command ...
(keep reading)

> +  /* We don't need a return value.  */
> +  tp->proceed_to_finish = 0;
> +  /* 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
> +     no good to do so.
> +
> +     Note that this can only happen at frame #0, since there's
> +     no way that a function up the stack can have a return address
> +     that's equal to its entry point.  */
> +
> +  if (sal.pc != read_pc ())
> +    {
> +      /* Set breakpoint and continue.  */
> +      breakpoint =
> +       set_momentary_breakpoint (sal,
> +                                 get_frame_id (get_selected_frame (NULL)),
> +                                 bp_breakpoint);
> +      /* Tell the breakpoint to keep quiet.  We won't be done
> +         until we've done another reverse single-step.  */
> +      make_breakpoint_silent (breakpoint);
> +      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->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.  */
> +      /* (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);
> +    }

I guess calling step_once doesn't help then?  Otherwise, "kludgy" here
sounds unfair... This is, after all, the interface used
to request a single-step even in forward direction ...

> +  return;

Useless.

> +}
> +
>  /* "finish": Set a temporary breakpoint at the place the selected
>     frame will return to, then continue.  */
>  
> @@ -1391,6 +1452,10 @@ finish_command (char *arg, int from_tty)
>    if (async_exec && !target_can_async_p ())
>      error (_("Asynchronous execution not supported on this target."));
>  
> +  /* Don't try to async in reverse.  */
> +  if (async_exec && execution_direction == EXEC_REVERSE)
> +    error (_("Asynchronous 'finish' not supported in reverse."));
> +
>    /* If we are not asked to run in the bg, then prepare to run in the
>       foreground, synchronously.  */
>    if (!async_exec && target_can_async_p ())
> @@ -1412,13 +1477,6 @@ finish_command (char *arg, int from_tty)
>  
>    clear_proceed_status ();
>  
> -  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);
> -
>    /* Find the function we will return from.  */
>  
>    function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
> @@ -1427,10 +1485,29 @@ finish_command (char *arg, int from_tty)
>       source.  */
>    if (from_tty)
>      {
> -      printf_filtered (_("Run till exit from "));
> +      if (execution_direction == EXEC_REVERSE)
> +       printf_filtered (_("Run back to call of "));
> +      else
> +       printf_filtered (_("Run till exit from "));
> +
>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
>      }
>  
> +  if (execution_direction == EXEC_REVERSE)
> +    {
> +      /* Split off at this point.  */
> +      finish_backward (function, tp);
> +      return;
> +    }

Didn't Joel request you to not do this, but do,

finish_command
  if (execution_direction == EXEC_REVERSE)
   finish_backward ();
  else
   finish_forward ();

instead ?

(then, it makes sense to put the async + reverse error
inside finish_backward, IMHO.)

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

-- 
Pedro Alves

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

[Michael Snyder]

Pedro Alves wrote:

>> +/* finish_backward -- helper function for finish_command.  */
>> +
>> +static void
>> +finish_backward (struct symbol *function, struct thread_info *tp)
>> +{
>> +  struct symtab_and_line sal;
>> +  struct breakpoint *breakpoint;
>> +  struct cleanup *old_chain;
>> +  CORE_ADDR func_addr;
>> +  int back_up;
>> +
>> +  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
>> +                               NULL, &func_addr, NULL) == 0)
>> +    internal_error (__FILE__, __LINE__,
>> +                   _("Finish: couldn't find function."));
>> +
> 
> Still internal_error?

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.
In fact, it's the same call that used to be in "finish_command".

So what should it be?  Just "error"?

>> +  sal = find_pc_line (func_addr, 0);
>> +
>> +  /* TODO: Let's not worry about async until later.  */
>> +
> 
> Should be an error here instead of on finish_command ...
> (keep reading)

OK, I forgot to remove the comment...

But I put the error in finish_command, because that is
where all of the necessary information is available.

In order to put the error here, I would have to add
more function parameters and pass more information.

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.

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

[Pedro Alves]

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?

Wait, what does your remark about the frame #0 special
case mean then?

(I have to admit I got confused again by what finish in reverse
means.  I'd personaly trade it easily for "begin", and I'd
do the reverse step to go to the caller myself.  Then, you
could do begin at frame #0 too...)

> 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.  ;-)

-- 
Pedro Alves

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

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)?


[-- Attachment #2: finish_forward.txt --]
[-- Type: text/plain, Size: 4367 bytes --]

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);
 }
 \f
 

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

[Michael Snyder]

Pedro Alves wrote:

>> 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?
> 
> Wait, what does your remark about the frame #0 special
> case mean then?

This stuff makes your head spin, eh?

OK, so this is the inverse of the example that I posted for Joel
(about stepping into a function backward).  This is "what happens
if I say "finish" and I'm going backward?"

So consider the function:

18: int foo (int a)
19: {
20:    return a + 1;
21: }

Now if I'm at line 20 and I want to finish forward, I have to
find the caller, set a bp AFTER the call, run to it, then
extract the return value for printing.

If I want to "reverse-finish", I still find the caller, but
now I want to set a breakpoint BEFORE (or at) the call insn.
I'm going to execute backward thru the call, rather than
execute forward thru the return.

My "special case" comment had nothing to do with frame #0.
The special case is that, if I happen to be starting from
the first instruction of the callee (the label or entry point),
then there is no need to set a breakpoint.  I can just do
a singlestep, and that will take me to the caller.

Also, when I am going backward, there is no return value
for me to extract when I get back to the caller, since the
callee is not returning.

As for the command name, I'm still not gonna argue about that.  ;-)
I just figured that a logical starting place would be, we have to
know what we're going to do in the case of each existing command
if we need to do it backward.

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

[Pedro Alves]

On Thursday 09 October 2008 03:46:58, Michael Snyder wrote:
> Pedro Alves wrote:

> > Wait, what does your remark about the frame #0 special
> > case mean then?
> 
> This stuff makes your head spin, eh?

Yeah, spins in reverse.

> OK, so this is the inverse of the example that I posted for Joel
> (about stepping into a function backward).  This is "what happens
> if I say "finish" and I'm going backward?"
> 
> So consider the function:
> 
> 18: int foo (int a)
> 19: {
> 20:    return a + 1;
> 21: }
> 
> Now if I'm at line 20 and I want to finish forward, I have to
> find the caller, set a bp AFTER the call, run to it, then
> extract the return value for printing.
> 
> If I want to "reverse-finish", I still find the caller, but
> now I want to set a breakpoint BEFORE (or at) the call insn.
> I'm going to execute backward thru the call, rather than
> execute forward thru the return.

Thanks for the explanations.

Ok, I had understood that correctly then. 

> My "special case" comment had nothing to do with frame #0.

Ok, it's probably too late here to be thinking, but I found this
frame #0 reference confusing:

+  Note that this can only happen at frame #0, since there's
+  no way that a function up the stack can have a return address
+  that's equal to its entry point.  */

> The special case is that, if I happen to be starting from
> the first instruction of the callee (the label or entry point),
> then there is no need to set a breakpoint.  I can just do
> a singlestep, and that will take me to the caller.
> 

Yep, that I got too.  That's what I was refering to when
I mentioned a "begin" command.  A "begin" would do:

- execute backward until I'm at the start of the function
  (after prologue)
- If I want to go to the caller (like your finish), I do
  another reverse "step" or "next".

No need for the extra single-step special case,

> Also, when I am going backward, there is no return value
> for me to extract when I get back to the caller, since the
> callee is not returning.

Or this special case.

> As for the command name, I'm still not gonna argue about that.  ;-)

So, it wasn't just about the command name.  ;-)

> I just figured that a logical starting place would be, we have to
> know what we're going to do in the case of each existing command
> if we need to do it backward.

Ok.

Thanks again for the explanations.

-- 
Pedro Alves

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

[Pedro Alves]

On Thursday 09 October 2008 03:36:03, Michael Snyder wrote:

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

I'm happy with that.  Thanks.

-- 
Pedro Alves

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 26 bytes --]

Committed, as attached:



[-- Attachment #2: final4.txt --]
[-- Type: text/plain, Size: 7612 bytes --]

2008-10-17  Michael Snyder  <msnyder@vmware.com>

	* breakpoint.c (make_breakpoint_silent): New function.
	* breakpoint.h (make_breakpoint_silent): Export.
	* infcmd.c (finish_command): Check for reverse exec direction.
	(finish_backward): New function, handle finish cmd in reverse.
	(finish_forward): New function, abstracted from finish_command.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.355
diff -u -p -r1.355 breakpoint.c
--- breakpoint.c	16 Oct 2008 16:25:04 -0000	1.355
+++ breakpoint.c	17 Oct 2008 19:29:37 -0000
@@ -7863,6 +7863,13 @@ set_ignore_count (int bptnum, int count,
   error (_("No breakpoint number %d."), bptnum);
 }
 
+void
+make_breakpoint_silent (struct breakpoint *b)
+{
+  /* Silence the breakpoint.  */
+  b->silent = 1;
+}
+
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.82
diff -u -p -r1.82 breakpoint.h
--- breakpoint.h	16 Oct 2008 16:25:04 -0000	1.82
+++ breakpoint.h	17 Oct 2008 19:29:37 -0000
@@ -897,4 +897,7 @@ extern int breakpoints_always_inserted_m
    in our opinion won't ever trigger.  */
 extern void breakpoint_retire_moribund (void);
 
+/* Tell a breakpoint to be quiet.  */
+extern void make_breakpoint_silent (struct breakpoint *);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.212
diff -u -p -r1.212 infcmd.c
--- infcmd.c	22 Sep 2008 15:20:08 -0000	1.212
+++ infcmd.c	17 Oct 2008 19:29:37 -0000
@@ -1366,19 +1366,109 @@ finish_command_continuation_free_arg (vo
   xfree (arg);
 }
 
+/* finish_backward -- helper function for finish_command.  */
+
+static void
+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;
+  int back_up;
+
+  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
+				NULL, &func_addr, NULL) == 0)
+    internal_error (__FILE__, __LINE__,
+		    _("Finish: couldn't find function."));
+
+  sal = find_pc_line (func_addr, 0);
+
+  /* We don't need a return value.  */
+  tp->proceed_to_finish = 0;
+  /* 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
+     no good to do so.
+
+     Note that this can only happen at frame #0, since there's
+     no way that a function up the stack can have a return address
+     that's equal to its entry point.  */
+
+  if (sal.pc != read_pc ())
+    {
+      /* Set breakpoint and continue.  */
+      breakpoint =
+	set_momentary_breakpoint (sal,
+				  get_frame_id (get_selected_frame (NULL)),
+				  bp_breakpoint);
+      /* Tell the breakpoint to keep quiet.  We won't be done
+         until we've done another reverse single-step.  */
+      make_breakpoint_silent (breakpoint);
+      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->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.  */
+      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;
 
@@ -1391,6 +1481,10 @@ finish_command (char *arg, int from_tty)
   if (async_exec && !target_can_async_p ())
     error (_("Asynchronous execution not supported on this target."));
 
+  /* Don't try to async in reverse.  */
+  if (async_exec && execution_direction == EXEC_REVERSE)
+    error (_("Asynchronous 'finish' not supported in reverse."));
+
   /* If we are not asked to run in the bg, then prepare to run in the
      foreground, synchronously.  */
   if (!async_exec && target_can_async_p ())
@@ -1408,17 +1502,8 @@ finish_command (char *arg, int from_tty)
   if (frame == 0)
     error (_("\"finish\" not meaningful in the outermost frame."));
 
-  tp = inferior_thread ();
-
   clear_proceed_status ();
 
-  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);
-
   /* Find the function we will return from.  */
 
   function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
@@ -1427,25 +1512,18 @@ finish_command (char *arg, int from_tty)
      source.  */
   if (from_tty)
     {
-      printf_filtered (_("Run till exit from "));
+      if (execution_direction == EXEC_REVERSE)
+	printf_filtered (_("Run back to call of "));
+      else
+	printf_filtered (_("Run till exit from "));
+
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
     }
 
-  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 ();
+  if (execution_direction == EXEC_REVERSE)
+    finish_backward (function);
+  else
+    finish_forward (function, frame);
 }
 \f