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

[teawater <teawater at gmail dot com>]

Thanks Pedro,

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.  :-(

Sorry for it.  It must be my gmail lost them.  :-(
I will try my best with them.

>
> 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)';

I will fix it.


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

This part in there because:
Process record and replay target need handler the debug behavior such
as resume, wait and insert breakpoint. And GDB will call the target
function that "to_stratum" is biggest one (If this one is NULL, GDB
will call the function that lower than this one). So add a
"record_stratum" to enum strata and set "to_stratum" of process record
to "record_stratum". Then, when GDB call debug function, it will call
the function in process record target.

When GDB in record mode, we need to call the really debug function in
low strata target because process record and replay target need call
this function to control the inferior.

Struct target_ops already has a pointer "beneath" point to low strata
target, but process record and replay target doesn't use it. Because
if low strata target doesn't set some function pointers, process
record and replay target will need to call the function pointers of
the target that is low strata target of this target. If this target
doesn't set them too, it will need to call anothers. So use "beneath"
is not a good choice and "multi-thread" target that need function
pointers of low strata target doesn't use "beneath" too.

The process record and replay target has 6 function pointers
record_beneath_to_resume, record_beneath_to_wait,
record_beneath_to_prepare_to_store, record_beneath_to_xfer_partial,
record_beneath_to_insert_breakpoint and
record_beneath_to_remove_breakpoint.
They are set in function "update_current_target". They are always
point to the function of low strata target.

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

OK. I will put it close to linux-record.c.  Wish it can be back in the future.


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

OK. I will deal with it.


Thanks,
Hui