From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28127 invoked by alias); 10 May 2010 19:27:34 -0000 Received: (qmail 28117 invoked by uid 22791); 10 May 2010 19:27:33 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,TW_OC,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 10 May 2010 19:27:25 +0000 Received: (qmail 9200 invoked from network); 10 May 2010 19:27:23 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 10 May 2010 19:27:23 -0000 From: Pedro Alves To: "Pierre Muller" , Joel Brobecker Subject: Re: [RFC] Add watchpoint hit address function to procfs.c Date: Mon, 10 May 2010 19:27:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <006601cae54b$368452f0$a38cf8d0$@muller@ics-cnrs.unistra.fr> <201005081823.56505.pedro@codesourcery.com> <004e01caf010$ac780450$05680cf0$@muller@ics-cnrs.unistra.fr> In-Reply-To: <004e01caf010$ac780450$05680cf0$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201005102027.10080.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: 2010-05/txt/msg00227.txt.bz2 On Monday 10 May 2010 08:16:08, Pierre Muller wrote: > > I tried to take all your remarks into account and > decreased further the size of the patch. Thanks. Now we can focus on the real interesting part of the patch. :-) > I also checked the testsuite by > running with RUNTESTFLAGS=gdb*/watch*.exp > with both CVS GDB and with patch applied. > (I used my modified gdb.exp to avoid problems > with the 'DOS-like' newlines..) See here for a workaround that requires no testsuite changes: > So there are no regressions. Thanks. > Remaining failures are: > pierre@fpcOpenSolaris:~/gdbcvs/build64/gdb/testsuite$ grep FAIL gdbv4.sum > FAIL: gdb.base/watch-read.exp: read watchpoint triggers when value doesn't change, trapping reads and writes > FAIL: gdb.base/watch-read.exp: only read watchpoint triggers when value doesn't change Odd. I don't suppose a procfs client is supposed to handle overlapping watchpoints instead of having the kernel handle them? The test explicitly sets a write watchpoint watching the same memory as a read watchpoint, and expects reads to still be trapped. Maybe the write request overwrote the read request? Maybe this is a kernel bug? > KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in > multithreaded > app (PRMS: gdb/10116) > FAIL: gdb.threads/watchthreads.exp: threaded watch loop > FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit > FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit > FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread > FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread > FAIL: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30 > > I also checked without adding TIDGET(inferior_ptid), leaving 0 instead, > for both find_procinfo_or_die calls. > This had no effect on the testsuite output (same 8 failures). As I said, if you have this test failing, then it may be that you haven't tested this change at all. What you could do instead, is to try running the test manually, and check whether a watchpoints work in the non-main threads, before and after patch. The theory was that they weren't working correctly before the patch, because target_stopped_by_watchpoint would be returning false for non-main threads. > 2010-05-10 Pierre Muller > > * procfs.c (proc_watchpoint_address): New function. > (procfs_stopped_by_watchpoint): Change second parameter of > find_procinfo_or_die call to TIDGET (INFERIOR_PTID). > Remove useless check after find_procinfo_or_die call. > (procfs_stopped_data_address): New function. > (procfs_use_watchpoints): Register new watchpoint related > function. > > > Index: src/gdb/procfs.c > =================================================================== > RCS file: /cvs/src/src/gdb/procfs.c,v > retrieving revision 1.131 > diff -u -p -r1.131 procfs.c > --- src/gdb/procfs.c 20 Apr 2010 22:36:35 -0000 1.131 > +++ src/gdb/procfs.c 10 May 2010 06:24:55 -0000 > @@ -1313,6 +1313,34 @@ proc_what (procinfo *pi) > #endif > } > > +/* > + * Function: proc_watchpoint_address > + * > + * This function is only called when PI is stopped by a watchpoint. > + * Assuming OS supports it, write to *ADDR the data address which > + * triggered it and return 1. > + * Return 0 if it is not possible to know the address. > + */ > + > +static int > +proc_watchpoint_address (procinfo *pi, CORE_ADDR *addr) > +{ > + if (!pi->status_valid) > + if (!proc_get_status (pi)) > + return 0; > + > +#ifdef NEW_PROC_API > + *addr = (CORE_ADDR) unsigned_pointer_to_address (target_gdbarch, > + builtin_type (target_gdbarch)->builtin_data_ptr, > + (gdb_byte *) &pi->prstatus.pr_lwp.pr_info.si_addr); > +#else > + *addr = (CORE_ADDR) unsigned_pointer_to_address (target_gdbarch, > + builtin_type (target_gdbarch)->builtin_data_ptr, > + (gdb_byte *) &pi->prstatus.pr_info.si_addr); > +#endif > + return 1; > +} target_gdbarch looks to be a bit of a stretch, though close enough probably. The alternative is to just assign the pointer to *addr directly, with an uintptr_t cast. IIRC, last we talked about this, we concluded that any addresses we see here are always below the address range that would need sign extension on signed addresses targets (like mips-irix). Still, in any case, if we're going to use an extraction method, please use gdbarch_pointer_to_address instead of unsigned_pointer_to_address directly, exactly so that archs that have signed pointers use signed_pointer_to_address automatically. Hmm. If there's a procfs target that doesn't report this, but instead leaves the si_addr field as 0, then perhaps we should revert to the hackish interface of claiming no support for the stopped data address if si_addr is 0. Joel, perhaps you could give this patch a quick test on mips-irix or AIX? (before or after it is committed, as you prefer.) Not sure if mips-irix even supports watchpoints, but AIX seems to? Even testing manually, if an OS doesn't report the stopped data address, then this will completely break watchpoints (I don't know if they're supported presently). If it does report it, then this should make read watchpoints actually work. > return 0; > } > +/* Still missing empty line between functions. > + * Function procfs_stopped_data_address > + * > + * Returns 1 if we the OS knows the position of the triggered ("position" is vague, IMO; we say "to that address" not "to that position", just below.) > + * watchpoint. Sets *ADDR to that address. > + * Returns 0 if OS cannot report that address. > + * This function is only called if procfs_stopped_by_watchpoint > + * returned 1, thus no further checks are done. > + * The function also assumes that ADDR is not NULL. > + */ (BTW, I'm thinking of reformating all comments in procfs.c to follow the coding conventions, so we avoid the tentation of proliferating this other format in procfs.c. Joel, you're the most likely to have non-FSF contributed patches to procfs.c. If that change would make it harder for you to merge patches upstream, just say so, and I'll forget doing the reformating.) Otherwise, looks fine. Please commit when you've addressed the comments above, and Joel as commented. -- Pedro Alves