RFC: reverse-finish

RFC: reverse-finish

[Michael Snyder]

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

Following on to my patch of 9/7 implementing reverse-step, reverse-next
and reverse-continue, this patch is sufficient to implement
reverse-finish.  This is meant for discussion, not approval (I don't
plan to check anything in right away).

Again, this is based on a modular approach that assumes only a
simple interface "get_exec_direction()".  I haven't taken it
down to the architecture level, but have simply assumed that
every function has a single entry point (at its label), and
that it is sufficient to continue backwards to that point,
and (assuming the correct frame) do one more single-step to
reach the call.

Here's the patch:


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

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.123
diff -p -r1.123 infcmd.c
*** infcmd.c	13 Sep 2004 18:26:28 -0000	1.123
--- infcmd.c	4 Sep 2005 23:08:10 -0000
*************** finish_command_continuation (struct cont
*** 1181,1186 ****
--- 1181,1188 ----
  /* "finish": Set a temporary breakpoint at the place the selected
     frame will return to, then continue.  */
  
+ static void finish_backwards (struct symbol *);
+ 
  static void
  finish_command (char *arg, int from_tty)
  {
*************** finish_command (char *arg, int from_tty)
*** 1223,1238 ****
  
    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);
- 
-   if (!target_can_async_p ())
-     old_chain = make_cleanup_delete_breakpoint (breakpoint);
-   else
-     old_chain = make_exec_cleanup_delete_breakpoint (breakpoint);
- 
    /* Find the function we will return from.  */
  
    function = find_pc_function (get_frame_pc (deprecated_selected_frame));
--- 1225,1230 ----
*************** finish_command (char *arg, int from_tty)
*** 1241,1250 ****
       source.  */
    if (from_tty)
      {
!       printf_filtered ("Run till exit from ");
        print_stack_frame (get_selected_frame (), 1, LOCATION);
      }
  
    /* If running asynchronously and the target support asynchronous
       execution, set things up for the rest of the finish command to be
       completed later on, when gdb has detected that the target has
--- 1233,1263 ----
       source.  */
    if (from_tty)
      {
!       if (get_exec_direction () == EXEC_REVERSE)
! 	printf_filtered ("Run back to call of ");
!       else
! 	printf_filtered ("Run till exit from ");
! 
        print_stack_frame (get_selected_frame (), 1, LOCATION);
      }
  
+   if (get_exec_direction () == EXEC_REVERSE)
+     {
+       /* Split off at this point.  */
+       finish_backwards (function);
+       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);
+ 
+   if (!target_can_async_p ())
+     old_chain = make_cleanup_delete_breakpoint (breakpoint);
+   else
+     old_chain = make_exec_cleanup_delete_breakpoint (breakpoint);
+ 
    /* If running asynchronously and the target support asynchronous
       execution, set things up for the rest of the finish command to be
       completed later on, when gdb has detected that the target has
*************** finish_command (char *arg, int from_tty)
*** 1301,1306 ****
--- 1314,1375 ----
        do_cleanups (old_chain);
      }
  }
+ 
+ static void
+ finish_backwards (struct symbol *function)
+ {
+   struct symtab_and_line sal;
+   struct breakpoint *breakpoint;
+   struct cleanup *old_chain;
+   CORE_ADDR func_addr;
+ 
+   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);
+ 
+   /* Let's cheat and not worry about async until later.  */
+ 
+   /* We don't need a return value.  */
+   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 ()),
+ 				  bp_breakpoint);
+       /* Tell the breakpoint to keep quiet.  */
+       breakpoint_muzzle (breakpoint);
+       old_chain = make_cleanup_delete_breakpoint (breakpoint);
+       proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
+       /* We will be stopped when proceed returns.  */
+       if (bpstat_find_breakpoint (stop_bpstat, breakpoint) == NULL)
+ 	{
+ 	  /* Didn't hit our breakpoint.  */
+ 	  internal_error (__FILE__, __LINE__, 
+ 			  "Finish backwards missed its breakpoint.");
+ 	}
+       do_cleanups (old_chain);
+     }
+   /* We're almost there; just back up one step.  */
+   /* (Kludgy way of letting wait_for_inferior know...) */
+   step_range_start = step_range_end = 1;
+   proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
+   return;
+ }
+ 
  \f
  
  static void

Re: RFC: reverse-finish

[Daniel Jacobowitz]

On Wed, Sep 21, 2005 at 11:17:51AM -0700, Michael Snyder wrote:
> Following on to my patch of 9/7 implementing reverse-step, reverse-next
> and reverse-continue, this patch is sufficient to implement
> reverse-finish.  This is meant for discussion, not approval (I don't
> plan to check anything in right away).

As with the previous patch: looks plausible to me, kind of ugly, so is
anything in infrun.

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

This shouldn't be an internal error; it's quite easy to provoke it from
the command line, so it should be at worst an error().  And please use
_() for the message eventually :-)

> +   sal = find_pc_line (func_addr, 0);

I think it's not a good idea to use the line table for this.  You want
a breakpoint at, literally, the first instruction of the function.  So
just create one.  Avoids problems with broken line information, et
cetera.

> +       /* Tell the breakpoint to keep quiet.  */
> +       breakpoint_muzzle (breakpoint);

Not sure what this patch is against, but we don't have
breakpoint_muzzle :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: RFC: reverse-finish

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Wed, Sep 21, 2005 at 11:17:51AM -0700, Michael Snyder wrote:
> 
>>+   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.");
> 
> 
> This shouldn't be an internal error; it's quite easy to provoke it from
> the command line, so it should be at worst an error().  And please use
> _() for the message eventually :-)

Sounds good.

>>+   sal = find_pc_line (func_addr, 0);
> 
> 
> I think it's not a good idea to use the line table for this.  You want
> a breakpoint at, literally, the first instruction of the function.  So
> just create one.  Avoids problems with broken line information, et
> cetera.

Ah.  You're right, I think I cut'n'pasted this from
step_into_function.  In this context, we want to step
*out of* the function.

>>+       /* Tell the breakpoint to keep quiet.  */
>>+       breakpoint_muzzle (breakpoint);
> 
> 
> Not sure what this patch is against, but we don't have
> breakpoint_muzzle :-)

Oh, sorry -- just a hack to keep the breakpoint
from being "mentioned".  I'm sure there's a better
way of doing it.