From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2480 invoked by alias); 29 Jun 2009 20:45:45 -0000 Received: (qmail 2290 invoked by uid 22791); 29 Jun 2009 20:45:44 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 29 Jun 2009 20:45:37 +0000 Received: (qmail 5790 invoked from network); 29 Jun 2009 20:45:34 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Jun 2009 20:45:34 -0000 From: Pedro Alves To: Doug Evans Subject: Re: [RFA] i386/amd64 h/w watchpoints in gdbserver Date: Mon, 29 Jun 2009 20:45:00 -0000 User-Agent: KMail/1.9.10 Cc: Pierre Muller , gdb-patches@sourceware.org References: <20090430071853.99F5584890@localhost> <200906210055.06940.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200906292146.39899.pedro@codesourcery.com> 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-06/txt/msg00848.txt.bz2 On Tuesday 23 June 2009 08:37:14, Doug Evans wrote: > On Sat, Jun 20, 2009 at 4:55 PM, Pedro Alves wrot= e: > > On Tuesday 02 June 2009 16:36:05, Doug Evans wrote: > > > > I don't think none of these forward declarations is needed? >=20 > ok > [once upon a time, they were ok. the new rules haven't been locked in > memory yet] It's easy --- less redundant code to maintain is good. In this case, in ad= dition to the unnecessary function prototypes, even those large descriptions were duplicated. >=20 > >> + > >> +int > >> +i386_low_stopped_by_watchpoint (struct i386_debug_reg_state *state) > >> +{ > >> + =A0CORE_ADDR addr =3D 0; > >> + =A0/* NOTE: gdb version passes boolean found/not-found result from > >> + =A0 =A0 i386_stopped_data_address. =A0*/ > >> + =A0addr =3D i386_low_stopped_data_address (state); > >> + =A0return (addr !=3D 0); > >> +} > > > > Same as above. =A0You've probably thought about that too... > > > >> + > >> +/* Support for h/w breakpoints. > >> + =A0 This support is not currently used, kept for reference. =A0*/ > > > > Any reason for not using this currently? =A0If there's a good reason, > > than let's drop it. =A0But I'd prefer to have it working. =A0:-) >=20 > deleted. Now you left me curious as to what was missing. >=20 > >> Index: utils.c > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > +char * > > +paddr (CORE_ADDR addr) > > > > This isn't documented in neither server.h or here? >=20 > Just "going with the flow". The flow says: "look closer and you'll see that all functions in utils.c have description comments." >=20 > > +{ > > + =A0char *str =3D get_cell (); > > + =A0xsnprintf (str, CELLSIZE, "%lx", (long) addr); > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= ^^^^ > > > > Note: this will be wrong on Win64... =A0BTW, Ulrich > > was removing several of these functions from GDB > > in the removing-current_gdbarch series. =A0Will this one > > stay? =A0Might be worth it to use the one that is going > > to stay in GDB. >=20 > I think a higher order bit is that gdb and gdbserver cannot share > code. Bringing over all the smarts to handle all the different > portability issues is painful/depressing.=20=20 I don't think a Win64 port of gdbserver will take too long to appear, so I'm sure this will an issue (albeit small, as this is debug output). Note that casting a pointer to long as never been garanteed to be portable. With the other point, all I meant was to look here: http://sourceware.org/ml/gdb-patches/2009-06/msg00223.html and notice that paddr is gone, in favor of paddress. So, it seemed to be that if copying an interface from GDB, might as well copy the one that is going to stay. > I went with something=20 > simple that works for now. > IWBN if this kind of thing were in, say, libiberty and tools could just u= se it. Fine, but this is not really an argument. You'd already brought a bit of code over from GDB, it's just a matter of bringing in more bits. OTOH, for gdbserver, it wouldn't probably be a problem to just use %p instead. But I'm fine with going with cast to long for now. It was a "Note:" afterall. Someone else will have to worry about it. >=20 > >> Index: linux-low.h > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v > >> retrieving revision 1.30 > >> diff -u -p -r1.30 linux-low.h > >> --- linux-low.h=A012 May 2009 22:25:00 -0000=A0=A0=A0=A0=A0=A01.30 > >> +++ linux-low.h=A01 Jun 2009 22:02:43 -0000 > >> @@ -56,8 +56,13 @@ struct process_info_private > >> > >> =A0 =A0/* Connection to the libthread_db library. =A0*/ > >> =A0 =A0td_thragent_t *thread_agent; > >> + > >> + =A0/* Target-specific additions. =A0*/ > > > > Warning: "Target" overload. =A0We need to get into the habit > > of not doing this --- it makes refering to these things quite > > ambiguous. =A0Call it "arch" or something else. =A0There are other > > similar cases. >=20 > I dunno. there's "the_low_target" in linux-low.h > Perhaps we can migrate away but I don't see the above "infraction" as > being critical. Please! Could spare us these extra iterations and go with "Low-target specific additions" then, or some other 4 or 5 letters adjustment, right? ;-) I wasn't asking for a rewrite. > >> --- linux-x86-low.c=A0=A0=A0=A0=A013 May 2009 19:11:04 -0000=A0=A0=A0= =A0=A0=A01.2 > >> +++ linux-x86-low.c=A0=A0=A0=A0=A01 Jun 2009 22:02:43 -0000 > > > >> +static unsigned long > >> +x86_linux_dr_get (ptid_t ptid, int regnum) > >> +{ > >> + =A0int tid; > >> + =A0unsigned long value; > >> + > >> + =A0tid =3D TIDGET (ptid); > >> + =A0if (tid =3D=3D 0) > >> + =A0 =A0tid =3D PIDGET (ptid); > > > > The tid =3D=3D 0 case is dead code coming from GDB, isn't it? > > Likewise in other places. >=20 > Perhaps. There's similar code in linux-low.c:same_lwp. > =3D=3D 0 code deleted. Yes, but same_lwp really handles cases where ptid_get_lwp =3D=3D 0, due to find_lwp_pid. IIUC, these functions are always called with a full thread ptid. > > > >> +/* Update the inferior's debug register REGNUM from STATE. =A0*/ > >> + > >> +void > >> +i386_dr_low_set_addr (const struct i386_debug_reg_state *state, int r= egnum) > >> +{ > >> + =A0struct inferior_list_entry *lp; > >> + =A0CORE_ADDR addr; > >> + > >> + =A0if (! (regnum >=3D 0 && regnum <=3D DR_LASTADDR - DR_FIRSTADDR)) > >> + =A0 =A0fatal ("Invalid debug register %d", regnum); > >> + > >> + =A0addr =3D state->dr_mirror[regnum]; > >> + > >> + =A0/* ??? Will need tweaking for multi-process. =A0*/ > > > > Indeed. =A0Why not just set the debug_registers_changed in lwps > > of the current process? >=20 > Are there any existing examples of this? > I would have done that had process_info contained the list of its > threads (it would have been trivially straightforward). Just match the pid of the current process with the pid of each thread? linux-low.c does that in several places. > + return 0; /* ??? fatal? */ This just means not-stopped-by-watchpoint? Certainly not fatal. > Setting aside breakpoints+watchpoints -> "points", > http://sourceware.org/ml/gdb-patches/2009-06/msg00594.html Right. An extra point: On Tuesday 23 June 2009 08:37:14, Doug Evans wrote: > + default: > + error ("Z_packet_to_hw_type: bad watchpoint type %c", type); This should not call error, but return unsupported. > how about this? It looks goodish, but I'd really like to see the points I raise be addressed, instead of just ignored. It just makes us waste the (narrow already) review bandwidth... --=20 Pedro Alves