From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8478 invoked by alias); 20 Apr 2006 19:42:06 -0000 Received: (qmail 8464 invoked by uid 22791); 20 Apr 2006 19:42:05 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 20 Apr 2006 19:42:02 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.12.11.20060308/8.12.11) with ESMTP id k3KJg0hH023432; Thu, 20 Apr 2006 15:42:00 -0400 Received: from potter.sfbay.redhat.com (potter.sfbay.redhat.com [172.16.27.15]) by int-mx1.corp.redhat.com (8.12.11.20060308/8.11.6) with ESMTP id k3KJg0Px021340; Thu, 20 Apr 2006 15:42:00 -0400 Received: from [172.16.24.50] (bluegiant.sfbay.redhat.com [172.16.24.50]) by potter.sfbay.redhat.com (8.12.8/8.12.8) with ESMTP id k3KJfwTq017907; Thu, 20 Apr 2006 15:41:59 -0400 Message-ID: <4447E406.5050006@redhat.com> Date: Thu, 20 Apr 2006 19:42:00 -0000 From: Michael Snyder User-Agent: Mozilla Thunderbird 1.0.7-1.4.1 (X11/20050929) MIME-Version: 1.0 To: Daniel Jacobowitz CC: GDB Patches Subject: Re: [RFA] Reverse debugging, part 1/3: target interface References: <442DAA70.5070203@redhat.com> <20060420135832.GD11710@nevyn.them.org> In-Reply-To: <20060420135832.GD11710@nevyn.them.org> 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-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-04/txt/msg00293.txt.bz2 Daniel Jacobowitz wrote: > > Need "set debug target 1" support for the new target ops. Oh yeah. Thanks, will do. > Also, it > might be nice to use the new style rather than using INHERIT (search > the vector at call time; see the "do not inherit" comments); that makes > the debugging output nicer, too. OK, I think I know what you mean (the ->beneath business) -- but why is this better? >>+ /* Set execution direction (forward/reverse). */ >>+ int (*to_set_execdir) (enum exec_direction_kind); >>+ /* Get execution direction (forward/reverse). */ >>+ enum exec_direction_kind (*to_get_execdir) (void); >>+ > > We've got these execdir methods... > >>+ /* Forward/reverse execution direction. >>+ These will only be implemented by a target that supports reverse execution. >>+ */ > > (comment formatting; wrap before the */ please?) OK > >>+ #define target_get_execution_direction() \ >>+ (current_target.to_get_execdir ? \ >>+ (*current_target.to_get_execdir) () : EXEC_ERROR) > > > ... but these _execution_direction methods. Please choose one or the > other; it makes it simpler to find the uses and implementations at the > same time. ... are you referring to the choices of names? I made the names of the macros more verbose because they're more visible, and used in a context where any aid to understanding is desireable. For the struct members themselves, I used shorter names because they're *not* all that visible, are used in a context where it's not all that critical to understand precisely what they do, and longer names are cumbersome. For instance, they would make the declarations in target.h really long. Do you still think I should change it? > You're below this bit: > > /* The vCont packet doesn't need to specify threads via Hc. */ > if (remote_vcont_resume (ptid, step, siggnal)) > return; > > So if the remote target supports vCont, you'll go forwards by accident > instead of reverse! Talk about a traffic accident. Oops, thanks for the catch. I'm not that familiar with vCont. > I think that's a pretty big FIXME. Acknowledged, discussed in another sub-thread. >>+ if (dir == EXEC_FORWARD || dir == EXEC_REVERSE) >>+ return (remote_execdir = dir); > > > And please don't do that - no wonder Eli was confused by this > function. If you want an assignment, put it on its own line :-) Oh, alright... ;-) >>+ remote_ops.to_get_execdir = remote_get_execdir; >>+ remote_ops.to_set_execdir = remote_set_execdir; > > > What about the other remote ops vectors? At least one isn't copied > from here. Right, well, I figured get it accepted in one vector, then worry about extending it into others. Basically, as of this patch, it will work with "target remote" only (and I guess only in non-async mode).