Re: [RFA] Reverse debugging, part 2/3: core interface

Re: [RFA] Reverse debugging, part 2/3: core interface

[Eli Zaretskii]

> *************** finish_command (char *arg, int from_tty)
> *** 1311,1320 ****
>        source.  */
>     if (from_tty)
>       {
> !       printf_filtered (_("Run till exit from "));
>         print_stack_frame (get_selected_frame (NULL), 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
> --- 1303,1333 ----
>        source.  */
>     if (from_tty)
>       {
> !       if (target_get_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);
>       }

The original code was i18n-ready, but your code is not.  Please use
_() to wrap messages we display to the user.

In addition, I think "Run back to call of FOO" is not very clear.  I
wanted to suggest "Run to entry to FOO", but then I realized it would
be a lie: we do back up past the entry, to the instruction that
actually calls the function we are in, right?  Perhaps "Run back to
before the call to FOO" is better, even though it is wordier?

> ! 	  if (debug_infrun)
> ! 	    fprintf_unfiltered (gdb_stdlog, 
> ! 				"infrun: stepped to a different function\n");

_() is missing around the message string (yes, I know it was missing
in the original code as well, but...).

> +     case NO_HISTORY:
> +       /* Reverse execution: target ran out of history info.  */
> +       ui_out_text (uiout, "\nNo more reverse-execution history.\n");

_(), please.

Re: [RFA] Reverse debugging, part 2/3: core interface

[Daniel Jacobowitz]

On Sat, Apr 01, 2006 at 03:34:39PM +0300, Eli Zaretskii wrote:
> In addition, I think "Run back to call of FOO" is not very clear.  I
> wanted to suggest "Run to entry to FOO", but then I realized it would
> be a lie: we do back up past the entry, to the instruction that
> actually calls the function we are in, right?  Perhaps "Run back to
> before the call to FOO" is better, even though it is wordier?

How about "Run back to call site of FOO"?  That's a pretty clear term.

> > ! 	  if (debug_infrun)
> > ! 	    fprintf_unfiltered (gdb_stdlog, 
> > ! 				"infrun: stepped to a different function\n");
> 
> _() is missing around the message string (yes, I know it was missing
> in the original code as well, but...).

I thought we'd decided not to translate debug messages.

But I can't remember for sure, and I can't find the message now (just
spent twenty minutes searching for it).  I did find a thread saying
that we agreed to translate internal error messages, which I thought
we'd decided not to.  I think that the arguments given in that thread
imply that we should not translate debugging messages.

(That was here:
  http://sourceware.org/ml/gdb/2005-02/msg00083.html
)

Sounds to me like we need the Coding chapter of gdbint.texinfo to
record decisions about this :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 2/3: core interface

[Eli Zaretskii]

> Date: Sat, 1 Apr 2006 11:10:17 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Michael Snyder <msnyder@redhat.com>, gdb-patches@sources.redhat.com
> 
> On Sat, Apr 01, 2006 at 03:34:39PM +0300, Eli Zaretskii wrote:
> > In addition, I think "Run back to call of FOO" is not very clear.  I
> > wanted to suggest "Run to entry to FOO", but then I realized it would
> > be a lie: we do back up past the entry, to the instruction that
> > actually calls the function we are in, right?  Perhaps "Run back to
> > before the call to FOO" is better, even though it is wordier?
> 
> How about "Run back to call site of FOO"?  That's a pretty clear term.

Fine with me.

> > > ! 	  if (debug_infrun)
> > > ! 	    fprintf_unfiltered (gdb_stdlog, 
> > > ! 				"infrun: stepped to a different function\n");
> > 
> > _() is missing around the message string (yes, I know it was missing
> > in the original code as well, but...).
> 
> I thought we'd decided not to translate debug messages.
> 
> But I can't remember for sure

Neither do I.

> Sounds to me like we need the Coding chapter of gdbint.texinfo to
> record decisions about this :-)

Why not?  Feel free to write it up.

Re: [RFA] Reverse debugging, part 2/3: core interface

[Michael Snyder]

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

Hi Eli,

Please see revised patch, attached.
Is this ok with you now?

Michael

Eli Zaretskii wrote:
>>*************** finish_command (char *arg, int from_tty)
>>*** 1311,1320 ****
>>       source.  */
>>    if (from_tty)
>>      {
>>!       printf_filtered (_("Run till exit from "));
>>        print_stack_frame (get_selected_frame (NULL), 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
>>--- 1303,1333 ----
>>       source.  */
>>    if (from_tty)
>>      {
>>!       if (target_get_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);
>>      }
> 
> 
> The original code was i18n-ready, but your code is not.  Please use
> _() to wrap messages we display to the user.
> 
> In addition, I think "Run back to call of FOO" is not very clear.  I
> wanted to suggest "Run to entry to FOO", but then I realized it would
> be a lie: we do back up past the entry, to the instruction that
> actually calls the function we are in, right?  Perhaps "Run back to
> before the call to FOO" is better, even though it is wordier?
> 
> 
>>! 	  if (debug_infrun)
>>! 	    fprintf_unfiltered (gdb_stdlog, 
>>! 				"infrun: stepped to a different function\n");
> 
> 
> _() is missing around the message string (yes, I know it was missing
> in the original code as well, but...).
> 
> 
>>+     case NO_HISTORY:
>>+       /* Reverse execution: target ran out of history info.  */
>>+       ui_out_text (uiout, "\nNo more reverse-execution history.\n");
> 
> 
> _(), please.
> 


[-- Attachment #2: diff2 --]
[-- Type: text/plain, Size: 14374 bytes --]

2006-03-31  Michael Snyder  <msnyder@redhat.com>

	Execution interface for reverse execution.
	* breakpoint.c (breakpoint_silence): New function.
	* breakpoint.h (breakpoint_silence): Export.
	* infcmd.c (finish_command): Check for reverse exec direction.
	(finish_backward): New function, handle finish cmd in reverse.
	* infrun.c (enum inferior_stop_reason): Add NO_HISTORY reason.
	(handle_inferior_event): Handle TARGET_WAITKIND_NO_HISTORY.
	Handle stepping over a function call in reverse.
	Handle stepping thru a line range in reverse.
	Handle setting a step-resume breakpoint in reverse.
	Handle stepping into a function in reverse.
	Handle stepping between line ranges in reverse.
	(print_stop_reason): Print reason for NO_HISTORY.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.224
diff -u -r1.224 breakpoint.c
--- breakpoint.c	6 Apr 2006 18:33:05 -0000	1.224
+++ breakpoint.c	17 Apr 2006 23:39:14 -0000
@@ -7326,6 +7326,13 @@
     b->ignore_count = 0;
 }
 
+void
+breakpoint_silence (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.36
diff -u -r1.36 breakpoint.h
--- breakpoint.h	17 Dec 2005 22:33:59 -0000	1.36
+++ breakpoint.h	17 Apr 2006 23:39:14 -0000
@@ -1,6 +1,6 @@
 /* Data structures associated with breakpoints in GDB.
-   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001,
-   2002, 2003, 2004
+   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
+   2001, 2002, 2003, 2004, 2006
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -805,4 +805,7 @@
    reinitialized -- e.g. when program is re-run.  */
 extern int deprecated_exception_support_initialized;
 
+/* Tell a breakpoint to be quiet.  */
+extern void breakpoint_silence (struct breakpoint *);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.144
diff -u -r1.144 infcmd.c
--- infcmd.c	27 Jan 2006 20:05:27 -0000	1.144
+++ infcmd.c	17 Apr 2006 23:39:15 -0000
@@ -1251,6 +1251,8 @@
 /* "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)
 {
@@ -1293,16 +1295,6 @@
 
   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));
@@ -1311,10 +1303,31 @@
      source.  */
   if (from_tty)
     {
-      printf_filtered (_("Run till exit from "));
+      if (target_get_execution_direction () == EXEC_REVERSE)
+	printf_filtered (_("Run backward to before call of "));
+      else
+	printf_filtered (_("Run till exit from "));
+
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
     }
 
+  if (target_get_execution_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
@@ -1371,6 +1384,62 @@
       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 (NULL)),
+				  bp_breakpoint);
+      /* Tell the breakpoint to keep quiet.  We won't be done 
+         until we've done another reverse single-step.  */
+      breakpoint_silence (breakpoint);
+      old_chain = make_cleanup_delete_breakpoint (breakpoint);
+      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
+      /* We will be stopped when proceed returns.  */
+      do_cleanups (old_chain);
+    }
+  if (bpstat_find_breakpoint (stop_bpstat, breakpoint) != NULL)
+    {
+      /* 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...) */
+      step_range_start = step_range_end = 1;
+      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
+    }
+  return;
+}
+
 \f
 
 static void
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.210
diff -u -r1.210 infrun.c
--- infrun.c	30 Mar 2006 16:37:13 -0000	1.210
+++ infrun.c	17 Apr 2006 23:39:15 -0000
@@ -2,8 +2,8 @@
    process.
 
    Copyright (C) 1986, 1987, 1988, 1989, 1990, 1991, 1992, 1993, 1994,
-   1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004 Free
-   Software Foundation, Inc.
+   1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2006
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -905,7 +905,9 @@
   /* Inferior exited. */
   EXITED,
   /* Inferior received signal, and user asked to be notified. */
-  SIGNAL_RECEIVED
+  SIGNAL_RECEIVED,
+  /* Reverse execution -- target ran out of history info.  */
+  NO_HISTORY
 };
 
 /* This structure contains what used to be local variables in
@@ -1504,6 +1506,12 @@
       stop_signal = ecs->ws.value.sig;
       break;
 
+    case TARGET_WAITKIND_NO_HISTORY:
+      /* Reverse execution: target ran out of history info.  */
+      print_stop_reason (NO_HISTORY, 0);
+      stop_stepping (ecs);
+      return;
+
       /* We had an event in the inferior, but we are not interested
          in handling it at this level. The lower layers have already
          done what needs to be done, if anything.
@@ -2129,6 +2137,17 @@
 	    keep_going (ecs);
 	    return;
 	  }
+	if (stop_pc == ecs->stop_func_start &&
+	    target_get_execution_direction () == EXEC_REVERSE)
+	  {
+	    /* We are stepping over a function call in reverse, and
+	       just hit the step-resume breakpoint at the start
+	       address of the function.  Go back to single-stepping,
+	       which should take us back to the function call.  */
+	    ecs->another_trap = 1;
+	    keep_going (ecs);
+	    return;
+	  }
 	break;
 
       case BPSTAT_WHAT_THROUGH_SIGTRAMP:
@@ -2312,7 +2331,22 @@
 	 fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
 			    paddr_nz (step_range_start),
 			    paddr_nz (step_range_end));
-      keep_going (ecs);
+
+      /* When stepping backward, stop at beginning of line range
+	 (unles it's the function entry point, in which case 
+	 keep going back to the call point).  */
+      if (stop_pc == step_range_start &&
+	  stop_pc != ecs->stop_func_start &&
+	  target_get_execution_direction () == EXEC_REVERSE)
+	{
+	  stop_step = 1;
+	  print_stop_reason (END_STEPPING_RANGE, 0);
+	  stop_stepping (ecs);
+	}
+      else
+	{
+	  keep_going (ecs);
+	}
       return;
     }
 
@@ -2393,10 +2427,33 @@
 
       if (step_over_calls == STEP_OVER_ALL)
 	{
-	  /* We're doing a "next", set a breakpoint at callee's return
-	     address (the address at which the caller will
-	     resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  /* We're doing a "next".
+
+	     Normal (forward) execution: set a breakpoint at the
+	     callee's return address (the address at which the caller
+	     will resume).
+
+	     Reverse (backward) execution.  set the step-resume
+	     breakpoint at the start of the function that we just
+	     stepped into (backwards), and continue to there.  When we
+	     get there, we'll need to single-step back to the
+	     caller.  */
+
+	  if (target_get_execution_direction () == EXEC_REVERSE)
+	    {
+	      /* FIXME: I'm not sure if we've handled the frame for
+		 recursion.  */
+
+	      struct symtab_and_line sr_sal;
+	      init_sal (&sr_sal);
+	      sr_sal.pc = ecs->stop_func_start;
+	      insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
+	    }
+	  else
+	    {
+	      insert_step_resume_breakpoint_at_frame
+		(get_prev_frame (get_current_frame ()));
+	    }
 	  keep_going (ecs);
 	  return;
 	}
@@ -2457,9 +2514,23 @@
 	  return;
 	}
 
-      /* Set a breakpoint at callee's return address (the address at
-         which the caller will resume).  */
-      insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+      if (target_get_execution_direction () == EXEC_REVERSE)
+	{
+	  /* Set a breakpoint at callee's start address.
+	     From there we can step once and be back in the caller.  */
+	  /* FIXME: I'm not sure we've handled the frame for recursion.  */
+	  struct symtab_and_line sr_sal;
+	  init_sal (&sr_sal);
+	  sr_sal.pc = ecs->stop_func_start;
+	  insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
+	}
+      else
+	{
+	  /* Set a breakpoint at callee's return address (the address
+	     at which the caller will resume).  */
+	  insert_step_resume_breakpoint_at_frame
+	    (get_prev_frame (get_current_frame ()));
+	}
       keep_going (ecs);
       return;
     }
@@ -2585,17 +2656,38 @@
 
   if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)
     {
-      /* If this is the last line of the function, don't keep stepping
-         (it would probably step us out of the function).
-         This is particularly necessary for a one-line function,
-         in which after skipping the prologue we better stop even though
-         we will be in mid-line.  */
-      if (debug_infrun)
-	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped to a different function\n");
-      stop_step = 1;
-      print_stop_reason (END_STEPPING_RANGE, 0);
-      stop_stepping (ecs);
-      return;
+      if (target_get_execution_direction () != EXEC_REVERSE)
+	{
+	  /* If this is the last line of the function, don't keep
+	     stepping (it would probably step us out of the function).
+	     This is particularly necessary for a one-line function,
+	     in which after skipping the prologue we better stop even
+	     though we will be in mid-line.  */
+	  if (debug_infrun)
+	    fprintf_unfiltered (gdb_stdlog, 
+				"infrun: stepped to a different function\n");
+	  stop_step = 1;
+	  print_stop_reason (END_STEPPING_RANGE, 0);
+	  stop_stepping (ecs);
+	  return;
+	}
+      else
+	{
+	  /* If we stepped backward into the last line of a function,
+	     then we've presumably stepped thru a return.  We want to
+	     keep stepping backward until we reach the beginning of
+	     the new line.  */
+	  step_range_start = ecs->sal.pc;
+	  step_range_end = ecs->sal.end;
+	  step_frame_id = get_frame_id (get_current_frame ());
+	  ecs->current_line = ecs->sal.line;
+	  ecs->current_symtab = ecs->sal.symtab;
+	  /* Adjust for prologue, in case of a one-line function.  */
+	  if (in_prologue (step_range_start, ecs->stop_func_start))
+	    step_range_start = SKIP_PROLOGUE (step_range_start);
+	  keep_going (ecs);
+	  return;
+	}
     }
   step_range_start = ecs->sal.pc;
   step_range_end = ecs->sal.end;
@@ -2658,6 +2750,28 @@
   if (s && s->language != language_asm)
     ecs->stop_func_start = SKIP_PROLOGUE (ecs->stop_func_start);
 
+  if (target_get_execution_direction () == EXEC_REVERSE)
+    {
+      ecs->sal = find_pc_line (stop_pc, 0);
+
+      /* OK, we're just gonna keep stepping here.  */
+      if (ecs->sal.pc == stop_pc)
+	{
+	  /* We're there already.  Just stop stepping now.  */
+	  stop_step = 1;
+	  print_stop_reason (END_STEPPING_RANGE, 0);
+	  stop_stepping (ecs);
+	  return;
+	}
+      /* Else just reset the step range and keep going.
+	 No step-resume breakpoint, they don't work for
+	 epilogues, which can have multiple entry paths.  */
+      step_range_start = ecs->sal.pc;
+      step_range_end   = ecs->sal.end;
+      keep_going (ecs);
+      return;
+    }
+  /* else... */
   ecs->sal = find_pc_line (ecs->stop_func_start, 0);
   /* Use the step_resume_break to step until the end of the prologue,
      even if that involves jumps (as it seems to on the vax under
@@ -2960,6 +3074,10 @@
       annotate_signal_string_end ();
       ui_out_text (uiout, ".\n");
       break;
+    case NO_HISTORY:
+      /* Reverse execution: target ran out of history info.  */
+      ui_out_text (uiout, _("\nNo more reverse-execution history.\n"));
+      break;
     default:
       internal_error (__FILE__, __LINE__,
 		      _("print_stop_reason: unrecognized enum value"));

Re: [RFA] Reverse debugging, part 2/3: core interface

[Eli Zaretskii]

> Date: Mon, 17 Apr 2006 16:42:02 -0700
> From: Michael Snyder <msnyder@redhat.com>
> CC: gdb-patches@sources.redhat.com
> 
> Please see revised patch, attached.
> Is this ok with you now?

Yes, thanks.

Re: [RFA] Reverse debugging, part 2/3: core interface

[Daniel Jacobowitz]

On Mon, Apr 17, 2006 at 04:42:02PM -0700, Michael Snyder wrote:
> @@ -1371,6 +1384,62 @@
>        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.  */

:-(

What problems does it present?  Need to use completions?

Async operation has been getting a bit musty.  I don't think it's fair
to insist you straighten that out before this goes in.  But, it would
be nice to issue an error here, instead of doing something quite
possibly wrong.

> +
> +  /* 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 ())

Above you used get_frame_pc, here you use read_pc.  More importantly...

> +    {
> +      /* Set breakpoint and continue.  */
> +      breakpoint = 
> +	set_momentary_breakpoint (sal, 
> +				  get_frame_id (get_selected_frame (NULL)),
> +				  bp_breakpoint);

...above you used get_current_frame, here you used get_selected_frame.
Which is it?  I think that it should be the selected frame in all three
cases, by analogy with finish, and there should probably be a similar
error for cases without a caller.

> +  if (bpstat_find_breakpoint (stop_bpstat, breakpoint) != NULL)
> +    {
> +      /* 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...) */
> +      step_range_start = step_range_end = 1;
> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
> +    }

Does this do the right thing if there was already an active breakpoint
at the function entry point?

> +	if (stop_pc == ecs->stop_func_start &&
> +	    target_get_execution_direction () == EXEC_REVERSE)

&& at the beginning of lines.  (Same applies to '='; I saw that
somewhere else in these patches, don't remember where now.)

> +	  {
> +	    /* We are stepping over a function call in reverse, and
> +	       just hit the step-resume breakpoint at the start
> +	       address of the function.  Go back to single-stepping,
> +	       which should take us back to the function call.  */
> +	    ecs->another_trap = 1;
> +	    keep_going (ecs);
> +	    return;
> +	  }
>  	break;

If we've hit a step resume breakpoint, this says, then delete it,
continue, and expect another trap.  Where's the other trap come from,
i.e. what makes us go back to stepping?  I'm sure this bit works,
I just can't see why.

> @@ -2312,7 +2331,22 @@
>  	 fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
>  			    paddr_nz (step_range_start),
>  			    paddr_nz (step_range_end));
> -      keep_going (ecs);
> +
> +      /* When stepping backward, stop at beginning of line range
> +	 (unles it's the function entry point, in which case 
> +	 keep going back to the call point).  */
> +      if (stop_pc == step_range_start &&
> +	  stop_pc != ecs->stop_func_start &&
> +	  target_get_execution_direction () == EXEC_REVERSE)
> +	{
> +	  stop_step = 1;
> +	  print_stop_reason (END_STEPPING_RANGE, 0);
> +	  stop_stepping (ecs);
> +	}
> +      else
> +	{
> +	  keep_going (ecs);
> +	}
>        return;
>      }
>  

Why do we automatically step out of functions, again?  I know we
discussed this - is it because this is the equivalent of prologue
skipping, in reverse?  A better comment here might be nice.

This looks like it could be nasty with recursion; we might step out of
a bunch of recursive calls for a tiny function.  But it looks like we
have the same issue in both directions, so let's ignore for now.

> @@ -2393,10 +2427,33 @@
>  
>        if (step_over_calls == STEP_OVER_ALL)
>  	{
> -	  /* We're doing a "next", set a breakpoint at callee's return
> -	     address (the address at which the caller will
> -	     resume).  */
> -	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
> +	  /* We're doing a "next".
> +
> +	     Normal (forward) execution: set a breakpoint at the
> +	     callee's return address (the address at which the caller
> +	     will resume).
> +
> +	     Reverse (backward) execution.  set the step-resume

Typo; probably "execution: set..."

> +	     breakpoint at the start of the function that we just
> +	     stepped into (backwards), and continue to there.  When we
> +	     get there, we'll need to single-step back to the
> +	     caller.  */
> +
> +	  if (target_get_execution_direction () == EXEC_REVERSE)
> +	    {
> +	      /* FIXME: I'm not sure if we've handled the frame for
> +		 recursion.  */
> +
> +	      struct symtab_and_line sr_sal;
> +	      init_sal (&sr_sal);
> +	      sr_sal.pc = ecs->stop_func_start;
> +	      insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
> +	    }

This does seem like a reasonably important FIXME.  Any reason it
shouldn't be get_current_frame()?  Same in the other occurance of this
FIXME below.

> @@ -2585,17 +2656,38 @@
>  
>    if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)

This (pre-existing) check is pretty bogus... the epilogue doesn't need
to be at the end of the function any more.

>      {
> -      /* If this is the last line of the function, don't keep stepping
> -         (it would probably step us out of the function).
> -         This is particularly necessary for a one-line function,
> -         in which after skipping the prologue we better stop even though
> -         we will be in mid-line.  */
> -      if (debug_infrun)
> -	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped to a different function\n");
> -      stop_step = 1;
> -      print_stop_reason (END_STEPPING_RANGE, 0);
> -      stop_stepping (ecs);
> -      return;

So this makes some sense for the one-line function case...

> +      else
> +	{
> +	  /* If we stepped backward into the last line of a function,
> +	     then we've presumably stepped thru a return.  We want to
> +	     keep stepping backward until we reach the beginning of
> +	     the new line.  */
> +	  step_range_start = ecs->sal.pc;
> +	  step_range_end = ecs->sal.end;
> +	  step_frame_id = get_frame_id (get_current_frame ());
> +	  ecs->current_line = ecs->sal.line;
> +	  ecs->current_symtab = ecs->sal.symtab;
> +	  /* Adjust for prologue, in case of a one-line function.  */
> +	  if (in_prologue (step_range_start, ecs->stop_func_start))
> +	    step_range_start = SKIP_PROLOGUE (step_range_start);
> +	  keep_going (ecs);
> +	  return;
> +	}
>      }
>    step_range_start = ecs->sal.pc;
>    step_range_end = ecs->sal.end;

... but I'm much more skeptical of this.  I don't think it does what
you want it to; you could arrive backwards in a function at many
points.  Can't you check for "stepped backwards into a new function"
independently of "the last line"?  I'm thinking step_frame_id
here.  When that changes, either we've stepped backwards into a
function called by this one (which will show up as a function call in
the normal way), or backwards into the caller of this one.

I'm not entirely sure we want to step to the beginning of the line in
this case either.  A better analogue might be stepping forwards out of
a function; we don't continue to the next line of the calling function.
This is especially useful on lines with multiple function calls:

  foo (bar(), baz());

step takes you into bar, out of bar, into baz, out of baz, into foo.
Should reverse stepping do the same, and take you out of foo (stop),
into baz (stop), out of baz (stop), et cetera?

> @@ -2658,6 +2750,28 @@
>    if (s && s->language != language_asm)
>      ecs->stop_func_start = SKIP_PROLOGUE (ecs->stop_func_start);
>  
> +  if (target_get_execution_direction () == EXEC_REVERSE)
> +    {
> +      ecs->sal = find_pc_line (stop_pc, 0);
> +
> +      /* OK, we're just gonna keep stepping here.  */
> +      if (ecs->sal.pc == stop_pc)
> +	{
> +	  /* We're there already.  Just stop stepping now.  */
> +	  stop_step = 1;
> +	  print_stop_reason (END_STEPPING_RANGE, 0);
> +	  stop_stepping (ecs);
> +	  return;
> +	}
> +      /* Else just reset the step range and keep going.
> +	 No step-resume breakpoint, they don't work for
> +	 epilogues, which can have multiple entry paths.  */
> +      step_range_start = ecs->sal.pc;
> +      step_range_end   = ecs->sal.end;
> +      keep_going (ecs);
> +      return;
> +    }

[In step_into_function] Won't this stepping range move us all the way
through that line and to the previous line?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Reverse debugging, part 2/3: core interface

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Apr 17, 2006 at 04:42:02PM -0700, Michael Snyder wrote:
> 
>>@@ -1371,6 +1384,62 @@
>>       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.  */
> 
> 
> :-(
> > 
> What problems does it present?  Need to use completions?

I have no idea, that's why I have deferred thinking about it.

> Async operation has been getting a bit musty.  I don't think it's fair
> to insist you straighten that out before this goes in.  But, it would
> be nice to issue an error here, instead of doing something quite
> possibly wrong.

Yes, I agree.  I'll undertake to do it.

>>+  /* 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 ())
> 
> Above you used get_frame_pc, here you use read_pc. 

Good catch.  It should probably be get_frame_pc.
You agree?


> More importantly...
> 
> 
>>+    {
>>+      /* Set breakpoint and continue.  */
>>+      breakpoint = 
>>+	set_momentary_breakpoint (sal, 
>>+				  get_frame_id (get_selected_frame (NULL)),
>>+				  bp_breakpoint);
> 
> ...above you used get_current_frame, here you used get_selected_frame.
> Which is it?  I think that it should be the selected frame in all three
> cases, by analogy with finish, and there should probably be a similar
> error for cases without a caller.

Oh man, you're making my head hurt.  Which is, I suppose, a good thing.

What's the difference, now?  Is 'current_frame' the execution frame,
and 'selected_frame' the displayed one?  (ie. the "up/down" one?)

If that's the case, then I think you're right, it should be
the selected frame.

I have to break off and go meet someone.  This is all good
feedback, I'll pick up the rest of your email this afternoon.

Re: [RFA] Reverse debugging, part 2/3: core interface

[Daniel Jacobowitz]

On Thu, Apr 20, 2006 at 12:49:06PM -0700, Michael Snyder wrote:
> Good catch.  It should probably be get_frame_pc.
> You agree?

Yep.

> >>+    {
> >>+      /* Set breakpoint and continue.  */
> >>+      breakpoint = 
> >>+	set_momentary_breakpoint (sal, 
> >>+				  get_frame_id (get_selected_frame (NULL)),
> >>+				  bp_breakpoint);
> >
> >...above you used get_current_frame, here you used get_selected_frame.
> >Which is it?  I think that it should be the selected frame in all three
> >cases, by analogy with finish, and there should probably be a similar
> >error for cases without a caller.
> 
> Oh man, you're making my head hurt.  Which is, I suppose, a good thing.

Only fair, you made my brain ache this morning.

> What's the difference, now?  Is 'current_frame' the execution frame,
> and 'selected_frame' the displayed one?  (ie. the "up/down" one?)
> 
> If that's the case, then I think you're right, it should be
> the selected frame.

That's right.

-- 
Daniel Jacobowitz
CodeSourcery