[RFA] Reverse Debugging, 3/5

[RFA] Reverse Debugging, 3/5

[Michael Snyder]

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



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


This patch (#3) adds to the infrun event loop so that it 
knows what to do differently if execution-direction is
reverse.  This effectively makes reverse step, next, 
and continue work properly, except that there is no way
for them to be invoked (that comes in a later patch).

This patch can go in on top of #1 and #2 without any
user-visible changes.  Tested under RHEL native with 
no change in results.


2008-09-30  Michael Snyder  <msnyder@vmware.com>
	Event handling interface for reverse debugging.
	* 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: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.322
retrieving revision 1.322.2.2
diff -u -p -r1.322 -r1.322.2.2
--- infrun.c	22 Sep 2008 15:26:53 -0000	1.322
+++ infrun.c	30 Sep 2008 23:50:51 -0000	1.322.2.2
@@ -1193,11 +1193,17 @@ proceed (CORE_ADDR addr, enum target_sig
 
   if (addr == (CORE_ADDR) -1)
     {
-      if (pc == stop_pc && breakpoint_here_p (pc))
+      if (pc == stop_pc && breakpoint_here_p (pc) 
+	  && target_get_execution_direction () != EXEC_REVERSE)
 	/* There is a breakpoint at the address we will resume at,
 	   step one instruction before inserting breakpoints so that
 	   we do not stop right away (and report a second hit at this
-	   breakpoint).  */
+	   breakpoint).
+
+	   Note, we don't do this in reverse, because we won't
+	   actually be executing the breakpoint insn anyway.
+	   We'll be (un-)executing the previous instruction.  */
+
 	oneproc = 1;
       else if (gdbarch_single_step_through_delay_p (gdbarch)
 	       && gdbarch_single_step_through_delay (gdbarch,
@@ -1426,7 +1432,9 @@ enum inferior_stop_reason
   /* 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
 };
 
 /* The PTID we'll do a target_wait on.*/
@@ -2141,6 +2149,12 @@ handle_inferior_event (struct execution_
       ecs->event_thread->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.
@@ -2861,6 +2875,17 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 	    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->event_thread->stepping_over_breakpoint = 1;
+	    keep_going (ecs);
+	    return;
+	  }
 	break;
 
       case BPSTAT_WHAT_CHECK_SHLIBS:
@@ -3026,10 +3051,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
       && stop_pc < ecs->event_thread->step_range_end)
     {
       if (debug_infrun)
-	 fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
+	fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
 			    paddr_nz (ecs->event_thread->step_range_start),
 			    paddr_nz (ecs->event_thread->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 == ecs->event_thread->step_range_start &&
+	  stop_pc != ecs->stop_func_start &&
+	  target_get_execution_direction () == EXEC_REVERSE)
+	{
+	  ecs->event_thread->stop_step = 1;
+	  print_stop_reason (END_STEPPING_RANGE, 0);
+	  stop_stepping (ecs);
+	}
+      else
+	{
+	  keep_going (ecs);
+	}
       return;
     }
 
@@ -3116,10 +3156,28 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 
       if (ecs->event_thread->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_caller (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)
+	    {
+	      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_caller (get_current_frame ());
+
 	  keep_going (ecs);
 	  return;
 	}
@@ -3176,9 +3234,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 	  return;
 	}
 
-      /* Set a breakpoint at callee's return address (the address at
-         which the caller will resume).  */
-      insert_step_resume_breakpoint_at_caller (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.  */
+	  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_caller (get_current_frame ());
+	}
       keep_going (ecs);
       return;
     }
@@ -3344,6 +3414,28 @@ step_into_function (struct execution_con
     ecs->stop_func_start = gdbarch_skip_prologue
 			     (current_gdbarch, ecs->stop_func_start);
 
+  if (target_get_execution_direction () == EXEC_REVERSE)
+    {
+      stop_func_sal = find_pc_line (stop_pc, 0);
+
+      /* OK, we're just gonna keep stepping here.  */
+      if (stop_func_sal.pc == stop_pc)
+	{
+	  /* We're there already.  Just stop stepping now.  */
+	  ecs->event_thread->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.  */
+      ecs->event_thread->step_range_start = stop_func_sal.pc;
+      ecs->event_thread->step_range_end   = stop_func_sal.end;
+      keep_going (ecs);
+      return;
+    }
+  /* else... */
   stop_func_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
@@ -3712,6 +3804,10 @@ print_stop_reason (enum inferior_stop_re
       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, 3/5

[Pedro Alves]

Hi Michael,

Haven't read the other patches yet, but I'll go ahead and give
some comments on this one.

On Wednesday 01 October 2008 20:18:35, Michael Snyder wrote:
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.322
> retrieving revision 1.322.2.2
> diff -u -p -r1.322 -r1.322.2.2
> --- infrun.c    22 Sep 2008 15:26:53 -0000      1.322
> +++ infrun.c    30 Sep 2008 23:50:51 -0000      1.322.2.2
> @@ -1193,11 +1193,17 @@ proceed (CORE_ADDR addr, enum target_sig
>  

>    if (addr == (CORE_ADDR) -1)
>      {
> -      if (pc == stop_pc && breakpoint_here_p (pc))
> +      if (pc == stop_pc && breakpoint_here_p (pc) 
> +         && target_get_execution_direction () != EXEC_REVERSE)

Hmmm, so EXEC_ERROR is accepted here.  What exactly is
EXEC_ERROR useful for?  Will there be a target that can't go
either direction?  :-)  Shouldn't failing to find ones
direction always be an error (hence an error call from inside
target_get_execution_direction, or something alike).

>  /* The PTID we'll do a target_wait on.*/
> @@ -2141,6 +2149,12 @@ handle_inferior_event (struct execution_
>        ecs->event_thread->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.
> @@ -2861,6 +2875,17 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>             keep_going (ecs);
>             return;
>           }

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

Split new line before the operator, not after:

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

Here and elsewhere.

>        case BPSTAT_WHAT_CHECK_SHLIBS:
> @@ -3026,10 +3051,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>        && stop_pc < ecs->event_thread->step_range_end)
>      {
>        if (debug_infrun)
> -        fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
> +       fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
>                             paddr_nz (ecs->event_thread->step_range_start),
>                             paddr_nz (ecs->event_thread->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

unless

> +        keep going back to the call point).  */
> +      if (stop_pc == ecs->event_thread->step_range_start &&
> +         stop_pc != ecs->stop_func_start &&
> +         target_get_execution_direction () == EXEC_REVERSE)
> +       {
> +         ecs->event_thread->stop_step = 1;
> +         print_stop_reason (END_STEPPING_RANGE, 0);
> +         stop_stepping (ecs);
> +       }

> +      else
> +       {
> +         keep_going (ecs);
> +       }

Unneeded braces.

>        return;
>      }
>  
> @@ -3116,10 +3156,28 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>  
>        if (ecs->event_thread->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_caller (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)
> +           {
> +             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_caller (get_current_frame ());
> +
>           keep_going (ecs);
>           return;
>         }
> @@ -3176,9 +3234,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>           return;
>         }
>  
> -      /* Set a breakpoint at callee's return address (the address at
> -         which the caller will resume).  */
> -      insert_step_resume_breakpoint_at_caller (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.  */
> +         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_caller (get_current_frame ());
> +       }

Unneeded braces.

>        keep_going (ecs);
>        return;
>      }
> @@ -3344,6 +3414,28 @@ step_into_function (struct execution_con
>      ecs->stop_func_start = gdbarch_skip_prologue
>                              (current_gdbarch, ecs->stop_func_start);
>  
> +  if (target_get_execution_direction () == EXEC_REVERSE)
> +    {
> +      stop_func_sal = find_pc_line (stop_pc, 0);
> +
> +      /* OK, we're just gonna keep stepping here.  */
> +      if (stop_func_sal.pc == stop_pc)
> +       {
> +         /* We're there already.  Just stop stepping now.  */
> +         ecs->event_thread->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.  */
> +      ecs->event_thread->step_range_start = stop_func_sal.pc;
> +      ecs->event_thread->step_range_end   = stop_func_sal.end;

Somethings fishy with the whitespace.     ^


> +      keep_going (ecs);
> +      return;
> +    }
> +  /* else... */
>    stop_func_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
> @@ -3712,6 +3804,10 @@ print_stop_reason (enum inferior_stop_re
>        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"));

Otherwise, I can't see anything wrong with it...

-- 
Pedro Alves

Re: [RFA] Reverse Debugging, 3/5

[Michael Snyder]

Pedro Alves wrote:
> Hi Michael,
> 
> Haven't read the other patches yet, but I'll go ahead and give
> some comments on this one.
> 
> On Wednesday 01 October 2008 20:18:35, Michael Snyder wrote:
>> Index: infrun.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/infrun.c,v
>> retrieving revision 1.322
>> retrieving revision 1.322.2.2
>> diff -u -p -r1.322 -r1.322.2.2
>> --- infrun.c    22 Sep 2008 15:26:53 -0000      1.322
>> +++ infrun.c    30 Sep 2008 23:50:51 -0000      1.322.2.2
>> @@ -1193,11 +1193,17 @@ proceed (CORE_ADDR addr, enum target_sig
>>
> 
>>    if (addr == (CORE_ADDR) -1)
>>      {
>> -      if (pc == stop_pc && breakpoint_here_p (pc))
>> +      if (pc == stop_pc && breakpoint_here_p (pc)
>> +         && target_get_execution_direction () != EXEC_REVERSE)
> 
> Hmmm, so EXEC_ERROR is accepted here.  What exactly is
> EXEC_ERROR useful for?  Will there be a target that can't go
> either direction?  :-) 

No, silly...   ;-)

> Shouldn't failing to find ones
> direction always be an error (hence an error call from inside
> target_get_execution_direction, or something alike).

Targets that don't implement reverse return EXEC_ERROR,
rather than EXEC_FORWARD.  It was an early interface
design decision, and I'm not sure if I can remember the
justification after over 2 years, but I made it
consciously -- it seemed to simplify things.


>>  /* The PTID we'll do a target_wait on.*/
>> @@ -2141,6 +2149,12 @@ handle_inferior_event (struct execution_
>>        ecs->event_thread->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.
>> @@ -2861,6 +2875,17 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>>             keep_going (ecs);
>>             return;
>>           }
> 
>> +       if (stop_pc == ecs->stop_func_start &&
>> +           target_get_execution_direction () == EXEC_REVERSE)
> 
> Split new line before the operator, not after:

OK

>>        case BPSTAT_WHAT_CHECK_SHLIBS:
>> @@ -3026,10 +3051,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>>        && stop_pc < ecs->event_thread->step_range_end)
>>      {
>>        if (debug_infrun)
>> -        fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
>> +       fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
>>                             paddr_nz (ecs->event_thread->step_range_start),
>>                             paddr_nz (ecs->event_thread->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
> 
> unless

OK

>> +        keep going back to the call point).  */
>> +      if (stop_pc == ecs->event_thread->step_range_start &&
>> +         stop_pc != ecs->stop_func_start &&
>> +         target_get_execution_direction () == EXEC_REVERSE)
>> +       {
>> +         ecs->event_thread->stop_step = 1;
>> +         print_stop_reason (END_STEPPING_RANGE, 0);
>> +         stop_stepping (ecs);
>> +       }
> 
>> +      else
>> +       {
>> +         keep_going (ecs);
>> +       }
> 
> Unneeded braces.

Don't you think it's more readable if the if block
and the else block match?

>>        return;
>>      }
>>
>> @@ -3116,10 +3156,28 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>>
>>        if (ecs->event_thread->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_caller (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)
>> +           {
>> +             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_caller (get_current_frame ());
>> +
>>           keep_going (ecs);
>>           return;
>>         }
>> @@ -3176,9 +3234,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
>>           return;
>>         }
>>
>> -      /* Set a breakpoint at callee's return address (the address at
>> -         which the caller will resume).  */
>> -      insert_step_resume_breakpoint_at_caller (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.  */
>> +         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_caller (get_current_frame ());
>> +       }
> 
> Unneeded braces.

Oh come on -- I know they're syntactic null, but
they serve to keep the comment together with the
code it refers to.

>>        keep_going (ecs);
>>        return;
>>      }
>> @@ -3344,6 +3414,28 @@ step_into_function (struct execution_con
>>      ecs->stop_func_start = gdbarch_skip_prologue
>>                              (current_gdbarch, ecs->stop_func_start);
>>
>> +  if (target_get_execution_direction () == EXEC_REVERSE)
>> +    {
>> +      stop_func_sal = find_pc_line (stop_pc, 0);
>> +
>> +      /* OK, we're just gonna keep stepping here.  */
>> +      if (stop_func_sal.pc == stop_pc)
>> +       {
>> +         /* We're there already.  Just stop stepping now.  */
>> +         ecs->event_thread->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.  */
>> +      ecs->event_thread->step_range_start = stop_func_sal.pc;
>> +      ecs->event_thread->step_range_end   = stop_func_sal.end;
> 
> Somethings fishy with the whitespace.     ^

I just like things to line up when possible!
;-)


>> +      keep_going (ecs);
>> +      return;
>> +    }
>> +  /* else... */
>>    stop_func_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
>> @@ -3712,6 +3804,10 @@ print_stop_reason (enum inferior_stop_re
>>        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"));
> 
> Otherwise, I can't see anything wrong with it...

Thanks for reviewing.

Re: [RFA] Reverse Debugging, 3/5

[Pedro Alves]

On Monday 06 October 2008 21:51:39, Michael Snyder wrote:
> Pedro Alves wrote:
> > Hi Michael,
> > 
> > Shouldn't failing to find ones
> > direction always be an error (hence an error call from inside
> > target_get_execution_direction, or something alike).
> 
> Targets that don't implement reverse return EXEC_ERROR,
> rather than EXEC_FORWARD.  It was an early interface
> design decision, and I'm not sure if I can remember the
> justification after over 2 years, but I made it
> consciously -- it seemed to simplify things.
> 

...  Okay.  If nobody else remembers why, and I throw a
later patch at you to change this, please don't be mad
at me.  :-)

> 
> >> +        keep going back to the call point).  */
> >> +      if (stop_pc == ecs->event_thread->step_range_start &&
> >> +         stop_pc != ecs->stop_func_start &&
> >> +         target_get_execution_direction () == EXEC_REVERSE)
> >> +       {
> >> +         ecs->event_thread->stop_step = 1;
> >> +         print_stop_reason (END_STEPPING_RANGE, 0);
> >> +         stop_stepping (ecs);
> >> +       }
> > 
> >> +      else
> >> +       {
> >> +         keep_going (ecs);
> >> +       }
> > 
> > Unneeded braces.
> 
> Don't you think it's more readable if the if block
> and the else block match?

Not really, and it's what the GDB/GNU coding standards prefer...

"
For the body of the function, our recommended style looks like this:

     if (x < foo (y, z))
       haha = bar[4] + 5;
     else
       {
         while (z)
           {
             haha += foo (z, z);
             z--;
           }
         return ++x + bar ();
       }
"


> >> +       }
> >> +      else
> >> +       {
> >> +         /* Set a breakpoint at callee's return address (the address
> >> +            at which the caller will resume).  */
> >> +         insert_step_resume_breakpoint_at_caller (get_current_frame ());
> >> +       }
> > 
> > Unneeded braces.
> 
> Oh come on -- I know they're syntactic null, but
> they serve to keep the comment together with the
> code it refers to.
> 

I'm not going to argue about these issues, but, personally, and to
stick with the standard, I do things like this a lot:

      if (foo)
        {
          /* Set a breakpoint at callee's return address (the address
             at which the caller will resume).  */
          goo ();
          bar ();
        }
      else
        /* Set a breakpoint at callee's return address (the address
           at which the caller will resume).  */
        insert_step_resume_breakpoint_at_caller (get_current_frame ());

> >> +      /* 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.  */
> >> +      ecs->event_thread->step_range_start = stop_func_sal.pc;
> >> +      ecs->event_thread->step_range_end   = stop_func_sal.end;
> > 
> > Somethings fishy with the whitespace.     ^
> 
> I just like things to line up when possible!
> ;-)

To me, visual vertical aligment is more distracting than good.  It
distract me from the right -> left assignment flow.  But, that's
just me.  I'm not going to make a bid deal out of it.

-- 
Pedro Alves

Re: [RFA] Reverse Debugging, 3/5

[Michael Snyder]

Pedro Alves wrote:
> On Monday 06 October 2008 21:51:39, Michael Snyder wrote:
>> Pedro Alves wrote:
>>> Hi Michael,
>>>
>>> Shouldn't failing to find ones
>>> direction always be an error (hence an error call from inside
>>> target_get_execution_direction, or something alike).
>> Targets that don't implement reverse return EXEC_ERROR,
>> rather than EXEC_FORWARD.  It was an early interface
>> design decision, and I'm not sure if I can remember the
>> justification after over 2 years, but I made it
>> consciously -- it seemed to simplify things.
>>
> 
> ...  Okay.  If nobody else remembers why, and I throw a
> later patch at you to change this, please don't be mad
> at me.  :-)

So long as you can argue that it makes things better.
;-)


>>>> +        keep going back to the call point).  */
>>>> +      if (stop_pc == ecs->event_thread->step_range_start &&
>>>> +         stop_pc != ecs->stop_func_start &&
>>>> +         target_get_execution_direction () == EXEC_REVERSE)
>>>> +       {
>>>> +         ecs->event_thread->stop_step = 1;
>>>> +         print_stop_reason (END_STEPPING_RANGE, 0);
>>>> +         stop_stepping (ecs);
>>>> +       }
>>>> +      else
>>>> +       {
>>>> +         keep_going (ecs);
>>>> +       }
>>> Unneeded braces.
>> Don't you think it's more readable if the if block
>> and the else block match?
> 
> Not really, and it's what the GDB/GNU coding standards prefer...

OK, good enough for me.

  > I'm not going to argue about these issues, but, personally, and to
> stick with the standard, I do things like this a lot:

You got it.

>>>> +      ecs->event_thread->step_range_start = stop_func_sal.pc;
>>>> +      ecs->event_thread->step_range_end   = stop_func_sal.end;
>>> Somethings fishy with the whitespace.     ^
>> I just like things to line up when possible!
>> ;-)
> 
> To me, visual vertical aligment is more distracting than good.  It
> distract me from the right -> left assignment flow.  But, that's
> just me.  I'm not going to make a bid deal out of it.

How about if I give you two out of three?   ;-)

Re: [RFA] Reverse Debugging, 3/5

[Joel Brobecker]

> To me, visual vertical aligment is more distracting than good.  It
> distract me from the right -> left assignment flow.  But, that's
> just me.  I'm not going to make a bid deal out of it.

Neither am I, but normally, we're supposed to follow the style
that gdb_indent.sh generates... (but I agree that visual vertical
alignment often creates distraction, the only exception is when
you have perhaps 20 lines in a row to align, but then where is
the limit?).

-- 
Joel

Re: [RFA] Reverse Debugging, 3/5

[Michael Snyder]

Joel Brobecker wrote:
>> To me, visual vertical aligment is more distracting than good.  It
>> distract me from the right -> left assignment flow.  But, that's
>> just me.  I'm not going to make a bid deal out of it.
> 
> Neither am I, but normally, we're supposed to follow the style
> that gdb_indent.sh generates... (but I agree that visual vertical
> alignment often creates distraction, the only exception is when
> you have perhaps 20 lines in a row to align, but then where is
> the limit?).

OK, consider me out-voted.   ;-)

Re: [RFA] Reverse Debugging, 3/5

[Joel Brobecker]

I had some comments, too, but Pedro made most of them.  Here are mine
that don't overlap with his...

> +      /* OK, we're just gonna keep stepping here.  */

I would prefer if we kept slang out of the comments. Can we use
"going to" instead?

> +      if (stop_func_sal.pc == stop_pc)
> +	{
> +	  /* We're there already.  Just stop stepping now.  */
> +	  ecs->event_thread->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.  */
> +      ecs->event_thread->step_range_start = stop_func_sal.pc;
> +      ecs->event_thread->step_range_end   = stop_func_sal.end;
> +      keep_going (ecs);
> +      return;
> +    }

Regarding this hunk, it really took me a long time to understand
what we're supposed to do when execution is reverse.  I think it
was a combination of the function name together with the fact that
we're not trying to undo what the function does, but instead implement
whatever command the user has issued which caused us to call this
function. I am thinking, for the sake of clarity, to implement this
in a separate function and call the appropriate function from
handle_inferior_event.  The function name can then be used to explain
at a glance what it's supposed to do...

-- 
Joel

Re: [RFA] Reverse Debugging, 3/5

[Joel Brobecker]

> But on the other hand, this is exactly what we are doing here.
> We are stepping into a function.  Only we're doing it in
> reverse, so we're coming in thru a return, not thru a call.

I think part of the issue is that, to me, "step_into_function" is
a misleading name for that function, as it implies that we haven't
stepped into the function yet.  So, what the function does is,
now that we've stepped into the function, see if we need to continue
somewhere a little farther or not. So, to me, doing the reverse of
"step_into_function" meant going back to the calling site...

> You still think I should split them up?

At the very least, I think that a comment explaining what the context
and what we need to do would be very useful.  But I also think that
putting the reverse part in its own function would be even clearer.
Your choice, though.

-- 
Joel

Re: [RFA] Reverse Debugging, 3/5

[Michael Snyder]

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

Joel Brobecker wrote:
>> But on the other hand, this is exactly what we are doing here.
>> We are stepping into a function.  Only we're doing it in
>> reverse, so we're coming in thru a return, not thru a call.
> 
> I think part of the issue is that, to me, "step_into_function" is
> a misleading name for that function, as it implies that we haven't
> stepped into the function yet.  So, what the function does is,
> now that we've stepped into the function, see if we need to continue
> somewhere a little farther or not. So, to me, doing the reverse of
> "step_into_function" meant going back to the calling site...
> 
>> You still think I should split them up?
> 
> At the very least, I think that a comment explaining what the context
> and what we need to do would be very useful.  But I also think that
> putting the reverse part in its own function would be even clearer.
> Your choice, though.

All right, how do you like this change (as a diff from the previous change)?

Don't worry, I'll post a new revised patch when we're at or near 
convergence.



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


2008-10-06  Michael Snyder  <msnyder@vmware.com>
 
	* infrun.c (step_into_function): Rename to stepped_into_function.
	Split into two versions (normal (forward), and reverse).
	(handle_inferior_event): Call stepped_into_function or 
	stepped_into_function_backward, depending on exec_direction.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.322.2.7
diff -u -p -r1.322.2.7 infrun.c
--- infrun.c	6 Oct 2008 23:24:00 -0000	1.322.2.7
+++ infrun.c	6 Oct 2008 23:44:06 -0000
@@ -1474,7 +1474,8 @@ void init_execution_control_state (struc
 
 void handle_inferior_event (struct execution_control_state *ecs);
 
-static void step_into_function (struct execution_control_state *ecs);
+static void stepped_into_function (struct execution_control_state *ecs);
+static void stepped_into_function_backward (struct execution_control_state *ecs);
 static void insert_step_resume_breakpoint_at_frame (struct frame_info *step_frame);
 static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
 static void insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
@@ -3226,7 +3227,13 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0)
 	  {
-	    step_into_function (ecs);
+	    /* Find start of appropriate source line (either first or
+	       last line in callee, depending on execution
+	       direction).  */	      
+	    if (target_get_execution_direction () == EXEC_REVERSE)
+	      stepped_into_function_backward (ecs);
+	    else
+	      stepped_into_function (ecs);
 	    return;
 	  }
       }
@@ -3408,42 +3415,21 @@ currently_stepping (struct thread_info *
 	  || bpstat_should_step ());
 }
 
-/* Subroutine call with source code we should not step over.  Do step
-   to the first line of code in it.  */
+/* Inferior has stepped into a subroutine call with source code that
+   we should not step over.  Do step to the first line of code in
+   it.  */
 
 static void
-step_into_function (struct execution_control_state *ecs)
+stepped_into_function (struct execution_control_state *ecs)
 {
   struct symtab *s;
   struct symtab_and_line stop_func_sal, sr_sal;
 
   s = find_pc_symtab (stop_pc);
   if (s && s->language != language_asm)
-    ecs->stop_func_start = gdbarch_skip_prologue
-			     (current_gdbarch, ecs->stop_func_start);
+    ecs->stop_func_start = gdbarch_skip_prologue (current_gdbarch, 
+						  ecs->stop_func_start);
 
-  if (target_get_execution_direction () == EXEC_REVERSE)
-    {
-      stop_func_sal = find_pc_line (stop_pc, 0);
-
-      /* OK, we're just going to keep stepping here.  */
-      if (stop_func_sal.pc == stop_pc)
-	{
-	  /* We're there already.  Just stop stepping now.  */
-	  ecs->event_thread->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.  */
-      ecs->event_thread->step_range_start = stop_func_sal.pc;
-      ecs->event_thread->step_range_end = stop_func_sal.end;
-      keep_going (ecs);
-      return;
-    }
-  /* else... */
   stop_func_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
@@ -3505,6 +3491,43 @@ step_into_function (struct execution_con
   keep_going (ecs);
 }
 
+/* Inferior has stepped backward into a subroutine call with source
+   code that we should not step over.  Do step to the beginning of the
+   last line of code in it.  */
+
+static void
+stepped_into_function_backward (struct execution_control_state *ecs)
+{
+  struct symtab *s;
+  struct symtab_and_line stop_func_sal, sr_sal;
+
+  s = find_pc_symtab (stop_pc);
+  if (s && s->language != language_asm)
+    ecs->stop_func_start = gdbarch_skip_prologue (current_gdbarch, 
+						  ecs->stop_func_start);
+
+  stop_func_sal = find_pc_line (stop_pc, 0);
+
+  /* OK, we're just going to keep stepping here.  */
+  if (stop_func_sal.pc == stop_pc)
+    {
+      /* We're there already.  Just stop stepping now.  */
+      ecs->event_thread->stop_step = 1;
+      print_stop_reason (END_STEPPING_RANGE, 0);
+      stop_stepping (ecs);
+    }
+  else
+    {
+      /* 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.  */
+      ecs->event_thread->step_range_start = stop_func_sal.pc;
+      ecs->event_thread->step_range_end = stop_func_sal.end;
+      keep_going (ecs);
+    }
+  return;
+}
+
 /* Insert a "step-resume breakpoint" at SR_SAL with frame ID SR_ID.
    This is used to both functions and to skip over code.  */
 

Re: [RFA] Reverse Debugging, 3/5

[Joel Brobecker]

> >I think part of the issue is that, to me, "step_into_function" is
> >a misleading name for that function, as it implies that we haven't
> >stepped into the function yet.  So, what the function does is,
> >now that we've stepped into the function, see if we need to continue
> >somewhere a little farther or not. So, to me, doing the reverse of
> >"step_into_function" meant going back to the calling site...

When I wrote that, I wasn't asking you to change the name. I hope
you didn't feel forced to do it...

> 	* infrun.c (step_into_function): Rename to stepped_into_function.

Can I suggest: handle_stepped_into_function.  This follows the
"handle_inferior_event" style while explaining that we're handling
the situation we're dealing with.

Same for handle_stepped_into_function_backwards.

> +	    /* Find start of appropriate source line (either first or
> +	       last line in callee, depending on execution
> +	       direction).  */	      
> +	    if (target_get_execution_direction () == EXEC_REVERSE)
> +	      stepped_into_function_backward (ecs);
> +	    else
> +	      stepped_into_function (ecs);
>  	    return;

I think that the comment is redundant in this case. Hopefully the
function name should give a good clue of what it is supposed to do
(handle the case of just having stepped into a function, either
forward or backwards depending on the execution direction).  The
actual details of how they do it do not matter at this level,
and keeping a copy here introduces some chances that it might
become out of date.

> +/* Inferior has stepped into a subroutine call with source code that
     ^ The inferior...
> +   we should not step over.  Do step to the first line of code in
> +   it.  */

> +/* Inferior has stepped backward into a subroutine call with source
Same here: "The inferior..."
> +   code that we should not step over.  Do step to the beginning of the
> +   last line of code in it.  */

I am sorry - perhaps this is because it's getting late, but I'm getting
confused again. What does it mean to step backward into a subroutine
call?  My understanding was completely wrong. Could you give maybe
an example at the user-interface level?  Maybe it's a typo in the
function description, but the function implementation doesn't seem
to be doing what you say it is. "Do step to the beginning of the
current line of code"? (I'm still interested in my example)

> +static void
> +stepped_into_function_backward (struct execution_control_state *ecs)

English question: Are "backward" and "backwards" interchangeable?
If these two words are strictly equivalent, I'd like for us to remain
consistent. But if now, can you explain to me what the difference is?

> +  if (s && s->language != language_asm)
> +    ecs->stop_func_start = gdbarch_skip_prologue (current_gdbarch, 
> +						  ecs->stop_func_start);

Depending of your answer to my question above, I'm wondering if
we should actually do a prologue skip...

-- 
Joel

Re: [RFA] Reverse Debugging, 3/5

[Michael Snyder]

Joel Brobecker wrote:
>>> I think part of the issue is that, to me, "step_into_function" is
>>> a misleading name for that function, as it implies that we haven't
>>> stepped into the function yet.  So, what the function does is,
>>> now that we've stepped into the function, see if we need to continue
>>> somewhere a little farther or not. So, to me, doing the reverse of
>>> "step_into_function" meant going back to the calling site...
> 
> When I wrote that, I wasn't asking you to change the name. I hope
> you didn't feel forced to do it...
> 
>>       * infrun.c (step_into_function): Rename to stepped_into_function.
> 
> Can I suggest: handle_stepped_into_function.  This follows the
> "handle_inferior_event" style while explaining that we're handling
> the situation we're dealing with.
> 
> Same for handle_stepped_into_function_backwards.
> 
>> +         /* Find start of appropriate source line (either first or
>> +            last line in callee, depending on execution
>> +            direction).  */
>> +         if (target_get_execution_direction () == EXEC_REVERSE)
>> +           stepped_into_function_backward (ecs);
>> +         else
>> +           stepped_into_function (ecs);
>>           return;
> 
> I think that the comment is redundant in this case. Hopefully the
> function name should give a good clue of what it is supposed to do
> (handle the case of just having stepped into a function, either
> forward or backwards depending on the execution direction).  The
> actual details of how they do it do not matter at this level,
> and keeping a copy here introduces some chances that it might
> become out of date.

OK, I'll add "handle_" to the name, and remove the
redundant comment.


>> +/* Inferior has stepped into a subroutine call with source code that
>      ^ The inferior...
>> +   we should not step over.  Do step to the first line of code in
>> +   it.  */
> 
>> +/* Inferior has stepped backward into a subroutine call with source
> Same here: "The inferior..."
>> +   code that we should not step over.  Do step to the beginning of the
>> +   last line of code in it.  */
> 
> I am sorry - perhaps this is because it's getting late, but I'm getting
> confused again. What does it mean to step backward into a subroutine
> call?  My understanding was completely wrong. Could you give maybe
> an example at the user-interface level?  Maybe it's a typo in the
> function description, but the function implementation doesn't seem
> to be doing what you say it is. "Do step to the beginning of the
> current line of code"? (I'm still interested in my example)

Sure -- you're right, it is confusing.  I should remove
the word "call" in the comment above.

Here's your example:

18: i = i + 1;
19: foo ();
20: j = j + 1;

Now if I am at line 18, and I step forward, I will
step into function foo, thru the call and foo's prologue.
I will expect to stop at the beginning of the first source
line in foo, after the prologue.

OTOH, if I am at line 20 and I step backward, I will
also step into function foo -- thru the return instruction
and foo's epilogue.  In this case, I will expect to stop
at the beginning of the LAST (executed) source line in
foo, before the epilogue.

In each case it is accomplished by single-stepping and
using line information.  There is a difference in that
functions usually have a single entry but may have multiple
returns, but the single-stepping makes this transparent.


>> +static void
>> +stepped_into_function_backward (struct execution_control_state *ecs)
> 
> English question: Are "backward" and "backwards" interchangeable?
> If these two words are strictly equivalent, I'd like for us to remain
> consistent. But if now, can you explain to me what the difference is?

My non-professional, layman's interpretation is that "backwards"
is more slangy.  I will switch to using "backward" consistantly.


>> +  if (s && s->language != language_asm)
>> +    ecs->stop_func_start = gdbarch_skip_prologue (current_gdbarch,
>> +                                               ecs->stop_func_start);
> 
> Depending of your answer to my question above, I'm wondering if
> we should actually do a prologue skip...

Hmmm.  Well, the question is, do we need to update stop_func_start.
We don't use it locally, so perhaps we don't need to do it.  But
I don't really know if we're doing it because someone else will
maybe need it later.

It might be an artifact.  But it's not "incorrect", in that
skipping the prologue is the right thing to do IF you want
the function start location.

Re: [RFA] Reverse Debugging, 3/5

[Joel Brobecker]

HaaaAAAAA, thank you so much for the example, it makes complete sense now.
If you don't mind, and that's only a suggestion, perhaps I would suggest
handle_stepped_backward_into_function for the function name.

> Hmmm.  Well, the question is, do we need to update stop_func_start.
> We don't use it locally, so perhaps we don't need to do it.  But
> I don't really know if we're doing it because someone else will
> maybe need it later.
> 
> It might be an artifact.  But it's not "incorrect", in that
> skipping the prologue is the right thing to do IF you want
> the function start location.

Hmm, you might be right. I'm young and foolish, so I tend to delete code
I'm not sure I need, and only put it back when things do actually break.
I looked at the "handle_inferior_event" code, and it looks like this
field is not used before being recomputed, but it's hard to say for
sure, as you have to also visit the functions being called from there...
Anyway, I was just wondering...

-- 
Joel