From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11508 invoked by alias); 22 Jan 2009 13:23:14 -0000 Received: (qmail 11498 invoked by uid 22791); 22 Jan 2009 13:23:13 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Jan 2009 13:23:06 +0000 Received: (qmail 18588 invoked from network); 22 Jan 2009 13:23:04 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Jan 2009 13:23:04 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] Submit process record and replay third time, 3/9 Date: Thu, 22 Jan 2009 13:23:00 -0000 User-Agent: KMail/1.9.10 Cc: teawater References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200901221324.02225.pedro@codesourcery.com> 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: 2009-01/txt/msg00446.txt.bz2 Hi Hui, I've just skimmed through this patch, comments below. On Thursday 08 January 2009 05:46:17, teawater wrote: > This patch add the process record and replay target. This is the core > part of process record and replay. You still haven't addressed several past comments. :-( 1) Nit-picky nature, but I've warned you about it several months ago, so I'm escalating it. :-) You have several formatting things you need to clean up. Please change instances of: if (foo) { bar (); } To: if (foo) bar (); Please make sure you don't have any line exceeding 80 columns. Please remove redundant ()'s, like in `return (0)'; 2) This bit, +/* The real beneath function pointers. */ +void (*record_beneath_to_resume) (ptid_t, int, enum target_signal); +ptid_t (*record_beneath_to_wait) (ptid_t, struct target_waitstatus *); +void (*record_beneath_to_store_registers) (struct regcache *, int regno); +LONGEST (*record_beneath_to_xfer_partial) (struct target_ops * ops, + enum target_object object, + const char *annex, + gdb_byte * readbuf, + const gdb_byte * writebuf, + ULONGEST offset, LONGEST len); +int (*record_beneath_to_insert_breakpoint) (struct bp_target_info *); +int (*record_beneath_to_remove_breakpoint) (struct bp_target_info *); And the corresponding bit in target.c that sets these function pointers, and the RECORD_IS_REPLAY and TARGET_IS_PROCESS_RECORD macros, add coupling between the record target, and the core of gdb, that sounds unnecessary if we're adding record as a target stratum. I'd really like to see those function pointers go away, and mentioned adding new target vector entries for the properties of record target you want checked in common code. I've suggested how before, did you try it? 2.1) Related to coupling as well. You've added record.c to the list of files that are built on all hosts, but I don't think that record.c is currently buildable on all hosts. E.g., you're using sigaction unconditionally. I didn't spot any call to a function defined in a *-nat.c file in this patch, but if you have any, you'll need to either remove/rewrite it (ideal, I expect), or build record.c on native linux hosts only. 3) This is a new one: I'd prefer we don't add calls to normal_stop outside core inferior control code. There's only one left in go32-nat.c, and that has been on my list to eliminate. -- Pedro Alves