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

[teawater <teawater@gmail.com>]

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 <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
>