From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5991 invoked by alias); 10 Mar 2009 23:37:48 -0000 Received: (qmail 5953 invoked by uid 22791); 10 Mar 2009 23:37:47 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.190) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Mar 2009 23:37:40 +0000 Received: by ti-out-0910.google.com with SMTP id y8so1431062tia.12 for ; Tue, 10 Mar 2009 16:37:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.110.57.5 with SMTP id f5mr11749561tia.49.1236728257484; Tue, 10 Mar 2009 16:37:37 -0700 (PDT) In-Reply-To: <200903101800.08633.pedro@codesourcery.com> References: <200903092034.57367.pedro@codesourcery.com> <200903101800.08633.pedro@codesourcery.com> Date: Wed, 11 Mar 2009 01:15:00 -0000 Message-ID: Subject: Re: [RFA] Submit process record and replay third time, 3/9 From: teawater To: Pedro Alves Cc: gdb-patches@sourceware.org, Michael Snyder Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-03/txt/msg00147.txt.bz2 Hi Pedro, On Wed, Mar 11, 2009 at 02:00, Pedro Alves wrote: > On Tuesday 10 March 2009 17:02:46, teawater wrote: >> >> +#define TARGET_IS_PROCESS_RECORD =A0 \ >> >> + =A0 =A0 (current_target.beneath =3D=3D &record_ops) >> > >> > Sorry, but I repeat the request I've made several times already. =A0Th= is is >> > not the right way to do this. =A0You need to add a new target_ops meth= od or >> > property that the core of GDB checks on. =A0It is not correct that make >> > the core of GDB reference record_ops directly. =A0To come up with >> > the target callback's name, at each call site of TARGET_IS_PROCESS_REC= ORD, >> > consider what is the property of the current target that GDB needs to >> > know about the current target. =A0Is it something like: >> > >> > =A0target_is_recording () ? >> > =A0target_is_replaying () ? >> > =A0target_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 =3D=3D record_stra= tum) >> 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. =A0If 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. =A0Say, imagine that there's no record.c in > GDB at all. =A0Then, 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). =A0GDB wouldn't know how gdbserver > was implementing the recording feature, only that the remote side supports > the recording feature. =A0Now, 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? =A0 That is the key here. =A0Again, is it because the > target is recording, and you can't do some things in that case? > Is it because the target is replaying? =A0Or perhaps it's a more > fundamental state --- is the target in a read only state? =A0When 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. =A0Or, 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 =3D=3D 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 =3D=3D can_use_displaced_stepping_au= to && non_stop) || can_use_displaced_stepping =3D=3D 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 =3D 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 =3D=3D 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 =3D=3D 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