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

[teawater <teawater@gmail.com>]

Hi Pedro,

Sorry to disturb you.
Could you please help me review it?

Thanks,
Hui

On Wed, Mar 11, 2009 at 07:37, teawater <teawater@gmail.com> wrote:
> Hi Pedro,
>
> On Wed, Mar 11, 2009 at 02:00, Pedro Alves <pedro@codesourcery.com> wrote:
>> 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'.
>>
>
> TARGET_IS_PROCESS_RECORD is not very well.  I agree with it.
> Let's talk about "#define target_is_record (target)
> (target->to_stratum == record_stratum)".
>
> If add a new callback, it's mean that every target can be a process
> record or not.
> But process record is not a function, it's a target.
> For example:
> static int
> use_displaced_stepping (struct gdbarch *gdbarch)
> {
>  return (((can_use_displaced_stepping == can_use_displaced_stepping_auto
>            && non_stop)
>           || can_use_displaced_stepping == can_use_displaced_stepping_on)
>          && gdbarch_displaced_step_copy_insn_p (gdbarch)
>          && !TARGET_IS_PROCESS_RECORD);
> }
>
> This place use "TARGET_IS_PROCESS_RECORD" because process record
> target can't work with displaced stepping.
>
>      if (TARGET_IS_PROCESS_RECORD)
>        old_cleanups = record_gdb_operation_disable_set ();
>
>      if (singlestep_breakpoints_inserted_p
>          || !ptid_equal (ecs->ptid, inferior_ptid)
>          || !currently_stepping (ecs->event_thread)
>          || ecs->event_thread->prev_pc == breakpoint_pc)
>        regcache_write_pc (regcache, breakpoint_pc);
>
>      if (TARGET_IS_PROCESS_RECORD)
>        do_cleanups (old_cleanups);
>
> This place use "TARGET_IS_PROCESS_RECORD" because process record don't
> want record this pc change.
>
> Off course, in global code call a macro in record.h is ugly.  I agree.
>
> But "#define target_is_process_record (target) (target->to_stratum ==
> record_stratum)". is better.
> record_stratum is in enum strata
>  {
>    dummy_stratum,              /* The lowest of the low */
>    file_stratum,               /* Executable files, etc */
>    core_stratum,               /* Core dump files */
>    process_stratum,            /* Executing processes */
>    thread_stratum,             /* Executing threads */
>    record_stratum              /* Support record debugging */
>  };
> in target.h.
> It's used to make process record on the top of all other target.  Just
> process record use it.
> I think it friendly to both global code and process record target.
>
>
> Thanks,
> Hui
>