From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21290 invoked by alias); 8 May 2010 17:24:37 -0000 Received: (qmail 21280 invoked by uid 22791); 8 May 2010 17:24:36 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,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; Sat, 08 May 2010 17:24:30 +0000 Received: (qmail 9356 invoked from network); 8 May 2010 17:24:22 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 May 2010 17:24:22 -0000 From: Pedro Alves To: "Pierre Muller" Subject: Re: [RFC] Add watchpoint hit address function to procfs.c Date: Sat, 08 May 2010 17:24: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> <201005071625.18583.pedro@codesourcery.com> <000301caeecd$f7be2320$e73a6960$@muller@ics-cnrs.unistra.fr> In-Reply-To: <000301caeecd$f7be2320$e73a6960$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201005081823.56505.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/msg00214.txt.bz2 On Saturday 08 May 2010 17:46:11, Pierre Muller wrote: > > > + if (!proc_get_status (pi)) > > > + return 0; /* FIXME: not a good failure value (but what > > is?) */ > > > > Add a new CORE_ADDR pointer parameter, and use that as > > output. C hange the return type to int to indicate failure/success. > > That is, model the interface of this function from > > to_stopped_data_address's > > interface. > > OK, modified as you suggested. Thanks. > > > } > > > +/* > > > + * Function procfs_stopped_data_address > > > + * Returns non-zero if we can find the position > > > + * of the triggring watchpoint. > > > + */ > > > + Typo, "triggring". > > > +static int > > > +procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR > > *addr) > > > +{ > > > + CORE_ADDR waddr; > > > + procinfo *pi; > > > + int watchpoint_hit = 0; Left over unused variable. > > > + > > > + pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0); > > > + > > > + if (!pi) /* If no process, then not stopped by watchpoint! */ > > > + return 0; > > > > Find_procinfo_or_die throws if the procinfo isn't > > found (it's the die part), so this check is dead code. > Then we should simply call find_procinfo, no? > and also in procfs_stopped_by_watchpoint. Well, we can never reach here without finding a procinfo, unless we have a weird bug somewhere. This function is only ever called if to_stopped_by_watchpoint returned true. If that one returned true, a procinfo must be on the list.. And when to_stopped_by_watchpoint is called, the thread that reported the watchpoint must be on the list, an hence so its procinfo... I'd prefer to simply remove these checks; they really add nothing, and only add to the mess that is procfs.c... IMO. > > I wonder if this procinfo is the correct one. Shouldn't > > this look for the the procinfo for the current thread (the tid > > in inferior_ptid), not the one for the main thread? > I simply adapted the code from procfs_stopped_by_watchpoint > which also uses (PIDGET (inferior_ptid), 0). > > Should those two be converted into > (PIDGET (inferior_ptid), TIDGET (inferior_ptid))? > This is what is done below... I'd think so, but I didn't go check solaris's man pages to check which lwp reports the watchpoint hit. Did you? > New version of the patch below, > I ran it on a OpenSolaris 2.11 (x86_64 processor) > with RUNTESTFLAGS=gdb*/watch*.exp > It gave 152 PASS for 8 FAIL, > while current CVS gives 127 PASS and 33 FAIL. Sounds good, but the numbers alone aren't the full story: are any of those 8 FAILs regressions? > > Using TIDGET (inferior_ptid) or 0 > had no effect on that subset of the testsuite... If the "watchthreads*" are passing cleanly, it sounds good. If they're not passing neither before/after patch, you haven't really tested the change. All other watchpoints tests in the testsuite are single-threaded, IIRC. > PS: I noticed a strange thing for gdb.threads/watchthreads > Each thread is reported twice in `info threads': > (gdb) inf thr > [New Thread 2 (LWP 2)] > [New Thread 3 (LWP 3)] > [New Thread 5 (LWP 5)] > 11 Thread 5 (LWP 5) 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 10 Thread 3 (LWP 3) 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 9 Thread 2 (LWP 2) 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 8 LWP 6 0xfffffd7fff2d4a28 in _thrp_setup () > from /lib/64/libc.so.1 > * 7 Thread 4 (LWP 4) 0x0000000000401146 in thread_function (arg=0x2) > at ../../../src/gdb/testsuite/gdb.threads/watchthreads.c:75 > 6 LWP 5 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 5 LWP 4 0x0000000000401146 in thread_function (arg=0x2) > at ../../../src/gdb/testsuite/gdb.threads/watchthreads.c:75 > 4 LWP 3 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 3 LWP 2 0xfffffd7fff2dcf8a in __nanosleep () > from /lib/64/libc.so.1 > 2 Thread 1 (LWP 1) 0xfffffd7fff2de03a in __lwp_create () > from /lib/64/libc.so.1 > 1 LWP 1 0xfffffd7fff2de03a in __lwp_create () > from /lib/64/libc.so.1 > Is this expected? Yes. > 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 7 May 2010 16:01:59 -0000 > @@ -1312,6 +1312,33 @@ proc_what (procinfo *pi) > return pi->prstatus.pr_what; > #endif > } > +/* > + * Function: proc_watchpoint_address > + * > + * returns the prstatus.pr_lwp.pr_info.__data.__fault.__addr field in ADDR. > + * > + * si_addr is a macro for __data.__fault.__addr. Please remove all mentions of "__data.__fault.__addr". It's an internal detail you're not really supposed to be exposing anywhere. All accesses to siginfo_t fields like those go through macros like si_addr, si_FOO, everywhere, in all OSs. Actually, the whole comment doesn't add much. It is just repeating what the code does, like this: /* Increment i by one. */ i++; It should instead describe what the function does from a high level perspective. E.g., something like: Assuming PI is stopped at a watchpoint, and the OS supports it, write to *ADDR the data address which triggered it and return 1. Returns 0 if the 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; > +} > + > > #ifndef PIOCSSPCACT /* The following is not supported on OSF. */ > /* > @@ -5539,7 +5566,7 @@ procfs_stopped_by_watchpoint (void) > { > procinfo *pi; > > - pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0); > + pi = find_procinfo (PIDGET (inferior_ptid), TIDGET (inferior_ptid)); > > if (!pi) /* If no process, then not stopped by watchpoint! */ > return 0; > @@ -5560,6 +5587,29 @@ procfs_stopped_by_watchpoint (void) > } > return 0; > } > +/* Space between functions. > + * Function procfs_stopped_data_address > + * Returns non-zero if we can find the position > + * of the triggring watchpoint. Typo, "triggring". > + */ > + > +static int > +procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR *addr) > +{ > + CORE_ADDR waddr = 0; > + procinfo *pi; > + int res; > + > + pi = find_procinfo (PIDGET (inferior_ptid), TIDGET (inferior_ptid)); > + > + if (!pi) /* If no process, then not stopped by watchpoint! */ > + return 0; > + > + res = proc_watchpoint_address (pi, &waddr); > + if (addr) > + *addr = waddr; > + return res; > +} > > static int > procfs_insert_watchpoint (CORE_ADDR addr, int len, int type) > @@ -5607,6 +5657,7 @@ procfs_use_watchpoints (struct target_op > t->to_remove_watchpoint = procfs_remove_watchpoint; > t->to_region_ok_for_hw_watchpoint = procfs_region_ok_for_hw_watchpoint; > t->to_can_use_hw_breakpoint = procfs_can_use_hw_breakpoint; > + t->to_stopped_data_address = procfs_stopped_data_address; > } > > /* -- Pedro Alves