From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8155 invoked by alias); 20 Apr 2006 13:58:38 -0000 Received: (qmail 8143 invoked by uid 22791); 20 Apr 2006 13:58:37 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Thu, 20 Apr 2006 13:58:34 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1FWZg0-0003Vy-1z; Thu, 20 Apr 2006 09:58:32 -0400 Date: Thu, 20 Apr 2006 13:58:00 -0000 From: Daniel Jacobowitz To: Michael Snyder Cc: GDB Patches Subject: Re: [RFA] Reverse debugging, part 1/3: target interface Message-ID: <20060420135832.GD11710@nevyn.them.org> Mail-Followup-To: Michael Snyder , GDB Patches References: <442DAA70.5070203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <442DAA70.5070203@redhat.com> User-Agent: Mutt/1.5.8i 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/msg00271.txt.bz2 About the patch itself... On Fri, Mar 31, 2006 at 02:17:20PM -0800, Michael Snyder wrote: > Index: target.c > =================================================================== > RCS file: /cvs/src/src/gdb/target.c,v > retrieving revision 1.117 > diff -p -r1.117 target.c > *** target.c 17 Mar 2006 00:30:34 -0000 1.117 > --- target.c 31 Mar 2006 21:46:54 -0000 > *************** update_current_target (void) > *** 457,462 **** > --- 457,464 ---- > INHERIT (to_find_memory_regions, t); > INHERIT (to_make_corefile_notes, t); > INHERIT (to_get_thread_local_address, t); > + INHERIT (to_get_execdir, t); > + INHERIT (to_set_execdir, t); > INHERIT (to_magic, t); > } > #undef INHERIT Need "set debug target 1" support for the new target ops. 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. > + /* 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?) > + #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. > *************** remote_resume (ptid_t ptid, int step, en > *** 2423,2429 **** > else > set_thread (pid, 0); /* Run this thread. */ > > ! if (siggnal != TARGET_SIGNAL_0) > { > buf[0] = step ? 'S' : 'C'; > buf[1] = tohex (((int) siggnal >> 4) & 0xf); > --- 2423,2437 ---- > else > set_thread (pid, 0); /* Run this thread. */ > > ! if (target_get_execution_direction () == EXEC_REVERSE) > ! { > ! /* We don't pass signals to the target in reverse exec mode. */ > ! if (info_verbose && siggnal != TARGET_SIGNAL_0) > ! warning (" - Can't pass signal %d to target in reverse: ignored.\n", > ! siggnal); > ! strcpy (buf, step ? "bs" : "bc"); > ! } > ! else if (siggnal != TARGET_SIGNAL_0) > { > buf[0] = step ? 'S' : 'C'; > buf[1] = tohex (((int) siggnal >> 4) & 0xf); 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. > + static int remote_set_execdir (enum exec_direction_kind dir) > + { > + if (remote_debug && info_verbose) > + printf_filtered ("Set remote execdir: %s\n", > + dir == EXEC_FORWARD ? "forward" : > + dir == EXEC_REVERSE ? "reverse" : > + "bad direction"); > + > + /* FIXME: check target for capability. */ I think that's a pretty big FIXME. > + 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 :-) > + 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. -- Daniel Jacobowitz CodeSourcery