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

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 10 March 2009 17:02:46, teawater wrote:
> >> +#define TARGET_IS_PROCESS_RECORD   \
> >> +     (current_target.beneath == &record_ops)
> >
> > Sorry, but I repeat the request I've made several times already.  This is
> > not the right way to do this.  You need to add a new target_ops method or
> > property that the core of GDB checks on.  It is not correct that make
> > the core of GDB reference record_ops directly.  To come up with
> > the target callback's name, at each call site of TARGET_IS_PROCESS_RECORD,
> > consider what is the property of the current target that GDB needs to
> > know about the current target.  Is it something like:
> >
> >  target_is_recording () ?
> >  target_is_replaying () ?
> >  target_is_read_only () ?
> >
> > etc.
> >
> 
> I forget a process record has special strata "record_stratum".
> 
> What about delete "TARGET_IS_PROCESS_RECORD" and add
> #define target_is_record (target) (target->to_stratum == record_stratum)
> to target.h?

No, that's not a new callback...

If we in some hypothetical future end up layering yet another target
on top of record, then that check will fail.

What I'm saying is, TARGET_IS_PROCESS_RECORD is just as wrong
as TARGET_IS_LINUX_NAT_C, or TARGET_IS_WINDOWS_NAT_C.

E.g., in places in the common code that we want to check if the
current target has execution, we call target_has_execution, doesn't
matter which target it is, as long as it has execution.  If we want to
check that the target is in asynchronous more, we check for
target_is_async_p, again, doesn't matter which target it is.

In your case, imagine that you implemented all of record.c in
gdbserver instead of on GDB.  Say, imagine that there's no record.c in
GDB at all.  Then, when you connected to gdbserver, and told it to
start recording, the topmost pushed target on the GDB side would still be
the remote target (process stratum).  GDB wouldn't know how gdbserver
was implementing the recording feature, only that the remote side supports
the recording feature.  Now, see, here's a property we'd possibly
want to expose through a target method --- e.g., target_can_record_p().

If you look at the places in the core of GDB where you are
currently checking for TARGET_IS_PROCESS_RECORD, and you wanted those
checks to also return true in the case of precord being implemented
on the remote server, clearly, you'd need to check for some
target property other than the target name, or its stratum.

Why is it that you need to call TARGET_IS_PROCESS_RECORD in the
first place?   That is the key here.  Again, is it because the
target is recording, and you can't do some things in that case?
Is it because the target is replaying?  Or perhaps it's a more
fundamental state --- is the target in a read only state?  When you
find out which *state(s)* you're interesting in checking, we can
easily add target method(s) for it/them, and make the record
target implement them (say, returning true), and making the
default return false.  Or, we may even come to the conclusion
that is not a target method we want --- we may end up with
a global variable, similar to `execution_direction'.

-- 
Pedro Alves