From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23119 invoked by alias); 6 Oct 2008 22:18:34 -0000 Received: (qmail 23111 invoked by uid 22791); 6 Oct 2008 22:18:33 -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; Mon, 06 Oct 2008 22:17:54 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 5F7E04A007; Mon, 6 Oct 2008 15:17:50 -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 529768E5CD; Mon, 6 Oct 2008 15:17:50 -0700 (PDT) Message-ID: <48EA8E20.9090306@vmware.com> Date: Mon, 06 Oct 2008 22:18:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Joel Brobecker CC: "gdb-patches@sourceware.org" , Daniel Jacobowitz , Pedro Alves , teawater Subject: Re: [RFA] Reverse Debugging, 4/5 References: <48E3CD40.3070206@vmware.com> <20081006215637.GE21853@adacore.com> In-Reply-To: <20081006215637.GE21853@adacore.com> 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/msg00183.txt.bz2 Joel Brobecker wrote: >> * breakpoint.c (breakpoint_silence): New function. >> * infcmd.c (finish_command): Check for reverse exec direction. > > You're going to hate me, now :). This is another instance where > I think we can break the code a little differently: > > 1. finish_command_backwards (I would have prefered > "reverse_finish_command but :-P) If this were the function that directly implemented the reverse-finish command, I would have named it like that. There's actually another function that implements the reverse-finish command. This function is a helper, one that just does part of the job. > > 2. finish_command_forward > > 3. finish_command: > { > [do all the stuff about checking for args, etc] > if (target_get_exec_dir () == EXEC_REVERSE) > finish_command_backwards () > else > finish_command_forward () > } > > That way, we just split the finish_command code into two parts > without moving some of the code, and it's clear that the two paths > are completely distinct. The "branch-off" approach (that we used > for Ada but that I'm trying to avoid like the plague now) does obscure > the structure of your program. OK, I'm not totally opposed to the idea. ;-) >> +void >> +breakpoint_silence (struct breakpoint *b) >> +{ >> + /* Silence the breakpoint. */ >> + b->silent = 1; > > Minor nit: This name brings little meaning when I see it being called. > Can we change it to "make_breakpoint_silent"? That way, the comment > in the body becomes useless and can be removed. Yeah, ok. >> + if (find_pc_partial_function (get_frame_pc (get_current_frame ()), >> + NULL, &func_addr, NULL) == 0) >> + internal_error (__FILE__, __LINE__, >> + "Finish: couldn't find function."); > > Internal error? I understand that it should probably never happen > in this context, but how about making it a simple error instead. > If we trip this check, it's true that something went wrong, but > let's just abort the command and let the user try to continue > rather than asking the user whether we should abort the whole > session. > >> + /* TODO: Let's not worry about async until later. */ > > Should we add a check now, though, and error out if async was requested? Yeah. Will do. >> + /* (Kludgy way of letting wait_for_inferior know...) */ >> + tp->step_range_start = tp->step_range_end = 1; > > AARGH! More special meaning to these addresses. We really ought to > clean these up and put some specific flags in the structure, one day. > I don't know why we're trying so hard to resume these fields. OK, in this instance, I did not add the special meaning. I was only following what practice was already there. ;-)