From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28419 invoked by alias); 2 Oct 2008 21:42:35 -0000 Received: (qmail 28411 invoked by uid 22791); 2 Oct 2008 21:42:34 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 02 Oct 2008 21:41:59 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 2A9E939073; Thu, 2 Oct 2008 14:41:57 -0700 (PDT) Received: from [10.20.92.59] (promb-2s-dhcp59.eng.vmware.com [10.20.92.59]) by mailhost4.vmware.com (Postfix) with ESMTP id E6EF3C9A8D; Thu, 2 Oct 2008 14:41:56 -0700 (PDT) Message-ID: <48E53FE3.8090306@vmware.com> Date: Thu, 02 Oct 2008 21:42:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Eli Zaretskii CC: "gdb-patches@sourceware.org" , "drow@false.org" , "pedro@codesourcery.com" , "teawater@gmail.com" Subject: Re: [RFA] Reverse Debugging, 5/5 References: <48E3CD66.9020600@vmware.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; 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/msg00065.txt.bz2 Eli Zaretskii wrote: >> Date: Wed, 01 Oct 2008 12:20:06 -0700 >> From: Michael Snyder >> CC: Daniel Jacobowitz , Pedro Alves , teawater >> >> +static void >> +set_exec_direction_func (char *args, int from_tty, >> + struct cmd_list_element *cmd) >> +{ >> + if (target_get_execution_direction () != EXEC_ERROR) >> + { >> + enum exec_direction_kind dir = EXEC_ERROR; >> + >> + if (!strcmp (exec_direction, exec_forward)) >> + dir = EXEC_FORWARD; >> + else if (!strcmp (exec_direction, exec_reverse)) >> + dir = EXEC_REVERSE; >> + >> + if (target_set_execution_direction (dir) != EXEC_ERROR) >> + return; >> + } >> +} > > I'm not sure I get your intent with the last two lines: if > target_set_execution_direction returns anything but EXEC_ERROR, you > return, but if not, you ... return? What am I missing? Well, this is the implementation of a "set" command. If target_get_execution_direction returns EXEC_ERROR, it means that the target doesn't support reverse, in which case there's nothing to set, so I just return. Otherwise, I pass the request to the target. Possibly I should output something, like "target does not support this comand", but since it's a "set" command, it isn't expected to output anything and it doesn't receive a struct ui_file among its arguments. Originally I had it call error() in that case, but I found that caused a problem in the testsuite, default.exp. Apparently doing an "info set" causes every registered "set" function to be called with no arguments? >> + case EXEC_ERROR: >> + default: >> + fprintf_filtered, (out, >> + _("Target `%s' does not support execution-direction."), >> + target_shortname); >> + break; > > Why print an error message? isn't it better to say the direction is > "forward" (which is documented as the default in your patch for the > manual)? Maybe. I'm open to discussion. The context is, the user says "show exec-direction" with a target that doesn't support reverse. Is it better to just say "Forward", with no comment, or is it better to let the user know that the question is not applicable? Or both? >> + if (dir == EXEC_REVERSE) >> + error (_("Already in reverse mode. Use '%s' or 'set exec-dir forward'."), >> + cmd); > > Isn't it better to silently do the equivalent of "cmd"? Possibly. Again, I'm open to discussion. But I think we've discussed this one before, and I hope we don't get bogged down again... Here the context is that the user has set a sticky mode bit ("reverse"), but then has used one of the non-sticky commands such as "reverse-step". What should we do? A reverse-reverse-step, ie. forward? That's logical, but is it what the user wanted? The problem is compounded by the fact that it won't always be easy for the user to know whether we in fact went forward or back. If s/he realizes that we have gone the 'wrong' way, s/he can correct it; but if not... >> + add_com ("reverse-step", class_run, reverse_step, _("\ >> +Step program backward until it reaches the beginning of another source line.\n\ >> +Argument N means do this N times (or till program stops for another reason).") > > This sounds as if you are single-stepping the program until it reaches > the previous line (as opposed to running uninterrupted until you hit > previous line). Are you? Of course I am single-stepping until it reaches the previous line. It's the same way we do forward stepping. We don't and can't implement step by using a breakpoint, because we don't know whether the source line contains jumps or calls. We only use the stepping-breakpoint to get back after we have single-stepped thru a call (or something similar). I'm using the same step-range-start / step-range-end logic that we use to step forward (with minor but essential modifications). >> + add_com ("reverse-next", class_run, reverse_next, _("\ >> +Step program backward, proceeding through subroutine calls.\n\ > ^ > Won't commands like "apropos" stop at the first comma when they > display the short descriptions of commands? Yep, you're right! I should omit the comma, huh? >> +@node Reverse Execution >> +@chapter Running programs backward > > Please add a @cindex entry here. OK. >> +program was executing normally. Variables, registers etc. should > > Please put a @: after "etc.", to prevent the period being interpreted > by TeX as an end of a sentence. OK. >> After executing >> +a piece of source code in reverse, all side effects of that code >> +should be ``undone'' > > ALL side effects? I thought some of them cannot be undone, > un-outputting to the various I/O devices etc. That depends on the implementation on the target side. For instance, VMware's implementation can undo device I/O, so long as the device is virtual, but gdb-freeplay cannot. GDB has no way of knowing this, but perhaps I should say something more explicit about it in the doc? >> +Assuming you are debugging in a target environment that supports > > "IF you are debugging in a target ..." sounds better, I think. Agreed. >> Starting from >> +the first line of a function, @code{reverse-next} will take you back >> +to the caller of that function, @emph{before} the function was called. > > Shouldn't we have some kind of caveat here regarding function prologue > and epilogue? Like what? If I've done my job right, prologues and epilogues should be handled transparently, just like they are when stepping forward. And that seems to be reflected by the step-reverse.exp test. >> +@item set exec-direction > > There should be a @kindex entry here for this command. OK. Thanks a lot for your comments, Eli.