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

teawater
2009-03-10T23:37:48Z

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