Re: [RFA] Reverse Debugging, 3/5

[Pedro Alves <pedro@codesourcery.com>]

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