[RFA] Reverse Debugging, 3/5
Michael Snyder
msnyder@vmware.com
Mon Oct 6 20:54:00 GMT 2008
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.
More information about the Gdb-patches
mailing list