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

[teawater <teawater at gmail dot com>]

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