Re: [RFA] Submit process record and replay third time, 3/9

[Pedro Alves <pedro@codesourcery.com>]

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