This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]