From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32035 invoked by alias); 6 Oct 2008 20:54:06 -0000 Received: (qmail 32025 invoked by uid 22791); 6 Oct 2008 20:54:05 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.113.40.141) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 06 Oct 2008 20:53:30 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 003DB67EE; Mon, 6 Oct 2008 13:53:27 -0700 (PDT) Received: from [10.20.92.59] (promb-2s-dhcp59.eng.vmware.com [10.20.92.59]) by mailhost2.vmware.com (Postfix) with ESMTP id 796948E5CA; Mon, 6 Oct 2008 13:53:28 -0700 (PDT) Message-ID: <48EA7A5B.4040701@vmware.com> Date: Mon, 06 Oct 2008 20:54:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" , Daniel Jacobowitz , teawater Subject: Re: [RFA] Reverse Debugging, 3/5 References: <48E3CD0B.8020003@vmware.com> <200810062109.16785.pedro@codesourcery.com> In-Reply-To: <200810062109.16785.pedro@codesourcery.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-10/txt/msg00160.txt.bz2 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.