Re: [RFA] Reverse Debugging, 3/5

[Pedro Alves <pedro at codesourcery dot 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