This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Pedro, Sorry to disturb you. Could you please help me on it? :) Thanks, Hui ---------- Forwarded message ---------- From: teawater <teawater@gmail.com> Date: Mon, Feb 2, 2009 at 17:04 Subject: Re: [RFA] Submit process record and replay third time, 3/9 To: Pedro Alves <pedro@codesourcery.com> Cc: gdb-patches@sourceware.org Hi Pedro, According to your mail, I make a new patch. Could you please help me review it? 1. Change all if (foo) { bar (); } to if (foo) bar (); 2. Change some paddr_nz to host_address_to_string. 3. Change sigaction to signal like remote.c to make it not os special. 4. Remove normal_stop from record.c 5. Make all files don't have any line exceeding 80 columns. Do you think this patch is OK? Thanks, Hui On Thu, Jan 22, 2009 at 21:24, Pedro Alves <pedro@codesourcery.com> 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 >
Attachment:
3-record_target.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |