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

teawater
2009-01-23T14:56:11Z

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
>