From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3695 invoked by alias); 23 Jan 2009 14:56:11 -0000 Received: (qmail 3680 invoked by uid 22791); 23 Jan 2009 14:56:09 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.189) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 23 Jan 2009 14:56:05 +0000 Received: by ti-out-0910.google.com with SMTP id d10so3297663tib.12 for ; Fri, 23 Jan 2009 06:56:03 -0800 (PST) MIME-Version: 1.0 Received: by 10.110.43.18 with SMTP id q18mr1316446tiq.18.1232722563256; Fri, 23 Jan 2009 06:56:03 -0800 (PST) In-Reply-To: <200901221324.02225.pedro@codesourcery.com> References: <200901221324.02225.pedro@codesourcery.com> Date: Fri, 23 Jan 2009 14:56:00 -0000 Message-ID: Subject: Re: [RFA] Submit process record and replay third time, 3/9 From: teawater To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 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: 2009-01/txt/msg00464.txt.bz2 Hi Pedro, Do you think I need change: if (record_debug > 1) { fprintf_unfiltered (gdb_stdlog, "Process record: record_arch_list_add 0x%s.\n", paddr_nz ((CORE_ADDR)rec)); } to: if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, "Process record: record_arch_list_add 0x%s.\n", paddr_nz ((CORE_ADDR)rec)); Thanks, Hui On Thu, Jan 22, 2009 at 21:24, Pedro Alves wrote: > 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 >