From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25406 invoked by alias); 12 Mar 2009 23:57:26 -0000 Received: (qmail 25395 invoked by uid 22791); 12 Mar 2009 23:57:25 -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.189) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Mar 2009 23:57:20 +0000 Received: by ti-out-0910.google.com with SMTP id y8so1226831tia.12 for ; Thu, 12 Mar 2009 16:57:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.110.8.5 with SMTP id 5mr786475tih.10.1236902235328; Thu, 12 Mar 2009 16:57:15 -0700 (PDT) In-Reply-To: References: <200903092034.57367.pedro@codesourcery.com> <200903101800.08633.pedro@codesourcery.com> Date: Fri, 13 Mar 2009 00:27: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/msg00180.txt.bz2 Hi Pedro, Sorry to disturb you. Could you please help me review it? Thanks, Hui On Wed, Mar 11, 2009 at 07:37, teawater wrote: > 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. =A0T= his is >>> > not the right way to do this. =A0You need to add a new target_ops met= hod or >>> > property that the core of GDB checks on. =A0It is not correct that ma= ke >>> > 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_RE= CORD, >>> > 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_str= atum) >>> 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 suppor= ts >> 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. =A0I 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) > { > =A0return (((can_use_displaced_stepping =3D=3D can_use_displaced_stepping= _auto > =A0 =A0 =A0 =A0 =A0 =A0&& non_stop) > =A0 =A0 =A0 =A0 =A0 || can_use_displaced_stepping =3D=3D can_use_displace= d_stepping_on) > =A0 =A0 =A0 =A0 =A0&& gdbarch_displaced_step_copy_insn_p (gdbarch) > =A0 =A0 =A0 =A0 =A0&& !TARGET_IS_PROCESS_RECORD); > } > > This place use "TARGET_IS_PROCESS_RECORD" because process record > target can't work with displaced stepping. > > =A0 =A0 =A0if (TARGET_IS_PROCESS_RECORD) > =A0 =A0 =A0 =A0old_cleanups =3D record_gdb_operation_disable_set (); > > =A0 =A0 =A0if (singlestep_breakpoints_inserted_p > =A0 =A0 =A0 =A0 =A0|| !ptid_equal (ecs->ptid, inferior_ptid) > =A0 =A0 =A0 =A0 =A0|| !currently_stepping (ecs->event_thread) > =A0 =A0 =A0 =A0 =A0|| ecs->event_thread->prev_pc =3D=3D breakpoint_pc) > =A0 =A0 =A0 =A0regcache_write_pc (regcache, breakpoint_pc); > > =A0 =A0 =A0if (TARGET_IS_PROCESS_RECORD) > =A0 =A0 =A0 =A0do_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. =A0I agree. > > But "#define target_is_process_record (target) (target->to_stratum =3D=3D > record_stratum)". is better. > record_stratum is in enum strata > =A0{ > =A0 =A0dummy_stratum, =A0 =A0 =A0 =A0 =A0 =A0 =A0/* The lowest of the low= */ > =A0 =A0file_stratum, =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Executable files, etc= */ > =A0 =A0core_stratum, =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Core dump files */ > =A0 =A0process_stratum, =A0 =A0 =A0 =A0 =A0 =A0/* Executing processes */ > =A0 =A0thread_stratum, =A0 =A0 =A0 =A0 =A0 =A0 /* Executing threads */ > =A0 =A0record_stratum =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Support record debugg= ing */ > =A0}; > in target.h. > It's used to make process record on the top of all other target. =A0Just > process record use it. > I think it friendly to both global code and process record target. > > > Thanks, > Hui >