From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29969 invoked by alias); 24 Jan 2006 11:00:32 -0000 Received: (qmail 29957 invoked by uid 22791); 24 Jan 2006 11:00:29 -0000 X-Spam-Check-By: sourceware.org Received: from ausmtp03.au.ibm.com (HELO ausmtp03.au.ibm.com) (202.81.18.151) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 24 Jan 2006 11:00:26 +0000 Received: from sd0208e0.au.ibm.com (d23rh904.au.ibm.com [202.81.18.202]) by ausmtp03.au.ibm.com (8.12.10/8.12.10) with ESMTP id k0OB3AU0065450 for ; Tue, 24 Jan 2006 22:03:10 +1100 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.250.242]) by sd0208e0.au.ibm.com (8.12.10/NCO/VERS6.8) with ESMTP id k0OB32xx207890 for ; Tue, 24 Jan 2006 22:03:06 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.12.11/8.13.3) with ESMTP id k0OAxqjM006431 for ; Tue, 24 Jan 2006 21:59:52 +1100 Received: from wks190294wss.cn.ibm.com (wks190294wss.cn.ibm.com [9.181.133.242]) by d23av01.au.ibm.com (8.12.11/8.12.11) with ESMTP id k0OAxlFT006368; Tue, 24 Jan 2006 21:59:48 +1100 Date: Tue, 24 Jan 2006 11:00:00 -0000 From: Wu Zhou To: Daniel Jacobowitz cc: Eli Zaretskii , gdb-patches@sources.redhat.com, bje@au1.ibm.com, anton@au1.ibm.com, pgilliam@us.ibm.com Subject: Re: [RFC] GDB patches for hw watchpoints - revised In-Reply-To: Message-ID: References: <20060122205641.GF27224@nevyn.them.org> <20060124034304.GA4719@nevyn.them.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-01/txt/msg00353.txt.bz2 Hi Daniel, Because there are quite a few places in different arch/target of GDB which use region_size_ok_for_hw_watchpoint, so I am prefering to get this done in two steps: first add to_region_ok_for_hw_watchpoint into struct target_ops, then replace these to_region_size_ok_for_hw_watchpoint reference with to_region_ok_for_hw_watchpoint ones. IMHO, it is easier to not confuse the original intention of this patch with this replacement. What is your thought on this? Here is the modified patch following this point. Please review. P.S: I tested it on ppc64 with 2.6.15-git8 kernel (without any patch this time, Anton had check the needed code into the official kernel source tree). It works ok. I can see six more PASS on gdb.base/recurse.exp; the other seven failure are around the second watchpoint, which is S/W one. No regression is found on other testcases. I also tested this on old PPC kernel which don't support DABR, don't find any regression with the introduction of this patch. 2006-01-22 Ben Elliston Wu Zhou * ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG, PTRACE_GETSIGINFO): Define. (last_stopped_data_address): New. (ppc_linux_check_watch_resources): New function to check whether the target has available hardware watchpoint resource. (ppc_linux_region_ok_for_hw_watchpoint): New function to check if the region is ok for hardware watchpoint. (ppc_linux_insert_watchpoint): New function to insert a hardware watchpoint. (ppc_linux_remove_watchpoint): New function to remove a hardware watchpoint. (ppc_linux_stopped_data_address): New function to get the stopped data address of the watchpoint. (ppc_linux_stopped_by_watchpoint): New function to see if the inferior is stopped by watchpoint. (_initialize_ppc_linux_nat): Set the above hardware watchpoint related target vectors. * rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures to have nonsteppable watchpoint. * target.c (default_region_ok_for_hw_watchpoint, debug_to_region_ok_for_hw_watchpoint): New prototypes. (update_current_target): Inherit to_region_ok_for_hw_watchpoint and set default to_region_ok_for_hw_watchpoint. (default_region_ok_for_hw_watchpoint): New function. (debug_to_region_ok_for_hw_watchpoint): New function. (setup_target_debug): Set to_region_ok_for_hw_watchpoint of debug_target. target.h (struct target_ops): Add a new target vector to_region_ok_for_hw_watchpoint. (TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not defined anyplace else. Index: ppc-linux-nat.c =================================================================== RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v retrieving revision 1.55 diff -u -p -r1.55 ppc-linux-nat.c --- ppc-linux-nat.c 10 Sep 2005 18:11:04 -0000 1.55 +++ ppc-linux-nat.c 24 Jan 2006 09:19:06 -0000 @@ -81,6 +81,16 @@ #define PTRACE_SETEVRREGS 21 #endif +/* Similarly for the hardware watchpoint support. */ +#ifndef PTRACE_GET_DEBUGREG +#define PTRACE_GET_DEBUGREG 25 +#endif +#ifndef PTRACE_SET_DEBUGREG +#define PTRACE_SET_DEBUGREG 26 +#endif +#ifndef PTRACE_GETSIGINFO +#define PTRACE_GETSIGINFO 0x4202 +#endif /* This oddity is because the Linux kernel defines elf_vrregset_t as an array of 33 16 bytes long elements. I.e. it leaves out vrsave. @@ -146,6 +156,7 @@ struct gdb_evrregset_t error. */ int have_ptrace_getvrregs = 1; +static CORE_ADDR last_stopped_data_address = 0; /* Non-zero if our kernel may support the PTRACE_GETEVRREGS and PTRACE_SETEVRREGS requests, for reading and writing the SPE @@ -153,7 +164,6 @@ int have_ptrace_getvrregs = 1; error. */ int have_ptrace_getsetevrregs = 1; - int kernel_u_size (void) { @@ -777,6 +787,124 @@ store_ppc_registers (int tid) store_spe_register (tid, -1); } +static int +ppc_linux_check_watch_resources (int type, int cnt, int ot) +{ + int tid; + ptid_t ptid = inferior_ptid; + + /* DABR (data address breakpoint register) is optional for PPC variants. + Some variants have one DABR, others have none. So CNT can't be larger + than 1. */ + if (cnt > 1) + return 0; + + /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether + the target has DABR. If either answer is no, the ptrace call will + return -1. Fail in that case. */ + tid = TIDGET (ptid); + if (tid == 0) + tid = PIDGET (ptid); + + if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1) + return 0; + return 1; +} + +static int +ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) +{ + /* Handle sub-8-byte quantities. */ + if (len <= 0) + return 0; + + /* addr+len must fall in the 8 byte watchable region. */ + if ((addr + len) > (addr & ~7) + 8) + return 0; + + return 1; +} + +/* Set a watchpoint of type TYPE at address ADDR. */ +static long +ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw) +{ + int tid; + long dabr_value; + ptid_t ptid = inferior_ptid; + + dabr_value = addr & ~7; + switch (rw) + { + case hw_read: + /* Set read and translate bits. */ + dabr_value |= 5; + break; + case hw_write: + /* Set write and translate bits. */ + dabr_value |= 6; + break; + case hw_access: + /* Set read, write and translate bits. */ + dabr_value |= 7; + break; + } + + tid = TIDGET (ptid); + if (tid == 0) + tid = PIDGET (ptid); + + return ptrace (PTRACE_SET_DEBUGREG, tid, 0, dabr_value); +} + +static long +ppc_linux_remove_watchpoint (CORE_ADDR addr, int len) +{ + int tid; + ptid_t ptid = inferior_ptid; + + tid = TIDGET (ptid); + if (tid == 0) + tid = PIDGET (ptid); + + return ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0); +} + +static int +ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p) +{ + if (last_stopped_data_address) + { + *addr_p = last_stopped_data_address; + last_stopped_data_address = 0; + return 1; + } + return 0; +} + +static int +ppc_linux_stopped_by_watchpoint (void) +{ + int tid; + struct siginfo siginfo; + ptid_t ptid = inferior_ptid; + CORE_ADDR *addr_p; + + tid = TIDGET(ptid); + if (tid == 0) + tid = PIDGET (ptid); + + errno = 0; + ptrace (PTRACE_GETSIGINFO, tid, (PTRACE_TYPE_ARG3) 0, &siginfo); + + if (errno != 0 || siginfo.si_signo != SIGTRAP || + (siginfo.si_code & 0xffff) != 0x0004) + return 0; + + last_stopped_data_address = (CORE_ADDR) siginfo.si_addr; + return 1; +} + static void ppc_linux_store_inferior_registers (int regno) { @@ -900,6 +1028,14 @@ _initialize_ppc_linux_nat (void) t->to_fetch_registers = ppc_linux_fetch_inferior_registers; t->to_store_registers = ppc_linux_store_inferior_registers; + /* Add our watchpoint methods. */ + t->to_can_use_hw_breakpoint = ppc_linux_check_watch_resources; + t->to_region_ok_for_hw_watchpoint = ppc_linux_region_ok_for_hw_watchpoint; + t->to_insert_watchpoint = ppc_linux_insert_watchpoint; + t->to_remove_watchpoint = ppc_linux_remove_watchpoint; + t->to_stopped_by_watchpoint = ppc_linux_stopped_by_watchpoint; + t->to_stopped_data_address = ppc_linux_stopped_data_address; + /* Register the target. */ add_target (t); } Index: rs6000-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v retrieving revision 1.248 diff -u -p -r1.248 rs6000-tdep.c --- rs6000-tdep.c 1 Nov 2005 19:32:36 -0000 1.248 +++ rs6000-tdep.c 24 Jan 2006 09:19:08 -0000 @@ -3278,6 +3278,10 @@ rs6000_gdbarch_init (struct gdbarch_info _("rs6000_gdbarch_init: " "received unexpected BFD 'arch' value")); + /* P630 has nonsteppable watchpoint. So we are assuming that all PowerPC + targets have nonsteppable watchpoint. */ + set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1); + /* Sanity check on registers. */ gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0); Index: target.c =================================================================== RCS file: /cvs/src/src/gdb/target.c,v retrieving revision 1.111 diff -u -p -r1.111 target.c --- target.c 4 Sep 2005 16:18:20 -0000 1.111 +++ target.c 24 Jan 2006 09:19:09 -0000 @@ -47,6 +47,8 @@ static void kill_or_be_killed (int); static void default_terminal_info (char *, int); +static int default_region_ok_for_hw_watchpoint (CORE_ADDR, int); + static int default_region_size_ok_for_hw_watchpoint (int); static int nosymbol (char *, CORE_ADDR *); @@ -128,6 +130,8 @@ static int debug_to_stopped_by_watchpoin static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *); +static int debug_to_region_ok_for_hw_watchpoint (CORE_ADDR, int); + static int debug_to_region_size_ok_for_hw_watchpoint (int); static void debug_to_terminal_init (void); @@ -405,6 +409,7 @@ update_current_target (void) INHERIT (to_stopped_data_address, t); INHERIT (to_stopped_by_watchpoint, t); INHERIT (to_have_continuable_watchpoint, t); + INHERIT (to_region_ok_for_hw_watchpoint, t); INHERIT (to_region_size_ok_for_hw_watchpoint, t); INHERIT (to_terminal_init, t); INHERIT (to_terminal_inferior, t); @@ -531,6 +536,8 @@ update_current_target (void) de_fault (to_stopped_data_address, (int (*) (struct target_ops *, CORE_ADDR *)) return_zero); + de_fault (to_region_ok_for_hw_watchpoint, + default_region_ok_for_hw_watchpoint); de_fault (to_region_size_ok_for_hw_watchpoint, default_region_size_ok_for_hw_watchpoint); de_fault (to_terminal_init, @@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec } static int +default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) +{ + return (len <= TYPE_LENGTH (builtin_type_void_data_ptr)); +} + +static int default_region_size_ok_for_hw_watchpoint (int byte_count) { return (byte_count <= TYPE_LENGTH (builtin_type_void_data_ptr)); @@ -2115,6 +2128,21 @@ debug_to_can_use_hw_breakpoint (int type } static int +debug_to_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) +{ + CORE_ADDR retval; + + retval = debug_target.to_region_ok_for_hw_watchpoint (addr, len); + + fprintf_unfiltered (gdb_stdlog, + "TARGET_REGION_OK_FOR_HW_WATCHPOINT (%ld, %ld) = 0x%lx\n", + (unsigned long) addr, + (unsigned long) len, + (unsigned long) retval); + return retval; +} + +static int debug_to_region_size_ok_for_hw_watchpoint (int byte_count) { CORE_ADDR retval; @@ -2533,6 +2561,7 @@ setup_target_debug (void) current_target.to_remove_watchpoint = debug_to_remove_watchpoint; current_target.to_stopped_by_watchpoint = debug_to_stopped_by_watchpoint; current_target.to_stopped_data_address = debug_to_stopped_data_address; + current_target.to_region_ok_for_hw_watchpoint = debug_to_region_ok_for_hw_watchpoint; current_target.to_region_size_ok_for_hw_watchpoint = debug_to_region_size_ok_for_hw_watchpoint; current_target.to_terminal_init = debug_to_terminal_init; current_target.to_terminal_inferior = debug_to_terminal_inferior; Index: target.h =================================================================== RCS file: /cvs/src/src/gdb/target.h,v retrieving revision 1.76 diff -u -p -r1.76 target.h --- target.h 4 Sep 2005 16:18:20 -0000 1.76 +++ target.h 24 Jan 2006 09:19:09 -0000 @@ -345,6 +345,7 @@ struct target_ops int (*to_stopped_by_watchpoint) (void); int to_have_continuable_watchpoint; int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *); + int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int); int (*to_region_size_ok_for_hw_watchpoint) (int); void (*to_terminal_init) (void); void (*to_terminal_inferior) (void); @@ -1030,6 +1031,11 @@ extern void (*deprecated_target_new_objf (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE); #endif +#ifndef TARGET_REGION_OK_FOR_HW_WATCHPOINT +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len) \ + (*current_target.to_region_ok_for_hw_watchpoint) (addr, len) +#endif + #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT) #define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \ (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count) Regards - Wu Zhou On Tue, 24 Jan 2006, Wu Zhou wrote: > > On Mon, 23 Jan 2006, Daniel Jacobowitz wrote: > > > On Tue, Jan 24, 2006 at 11:40:16AM +0800, Wu Zhou wrote: > > > p630 is one kind of POWER4 based pSeriese server. It is currently the only > > > available ppc machine I can get. :-) > > > > > > In fact, I am not sure before if the ppc arch has nonsteppable watchpoints > > > or not. But testing on my p630 box, it did had nonsteppable ones. Now > > > that an architecture either have or doesn't have nonsteppable watchpoints, > > > can we get from this test a result that ppc architecture has nonsteppable > > > watchpoints? > > > > > > If so, maybe I can just remove the stupid conditional statement below. > > > (my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-) > > > > Well, it'd be nice to have some architectural reference for this. But > > it's probably a safe bet to assume that this is generally true for all > > PowerPC targets, so let's just assume it. > > OK. I will assume this then. > > > > Function to_region_ok_for_hw_watchpoint is not in the current target > > > vector (I means struct target_ops). Maybe we can add it into > > > target_ops? There are a few other archs also use this. But they had to > > > include it in nm-xxx-yyy.h. If not, the only method I can think of is > > > also include its definition in nm-ppc64-linux.h. So what about the > > > following patch section? > > > > > > int (*to_region_size_ok_for_hw_watchpoint) (int); > > > + int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int); > > > void (*to_terminal_init) (void); > > > > I would recommend replacing to_region_size_ok_for_hw_watchpoint > > with to_region_ok_for_hw_watchpoint. You'll have to update the > > callers, including the non-multi-arch ones, to ignore the first > > argument; shouldn't be hard? > > Do you means to make the following change in gdb/target.h? > > --- gdb/target.h 4 Sep 2005 16:18:20 -0000 1.76 > +++ gdb/target.h 24 Jan 2006 04:17:13 -0000 > @@ -345,7 +345,7 @@ > int (*to_stopped_by_watchpoint) (void); > int to_have_continuable_watchpoint; > int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *); > - int (*to_region_size_ok_for_hw_watchpoint) (int); > + int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int); > void (*to_terminal_init) (void); > void (*to_terminal_inferior) (void); > void (*to_terminal_ours_for_output) (void); > @@ -1030,9 +1030,9 @@ > (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE); > #endif > > -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT) > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \ > - (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count) > +#if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT) > +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, byte_count) \ > + (*current_target.to_region_ok_for_hw_watchpoint) (addr, byte_count) > #endif > > Then make responsive changes to the code where is referenced? > > such as (in config/i386/nm-i386sol2.h): > > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1 > +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE) 1 > > or (in s390-nat.c): > > static int > -s390_region_size_ok_for_hw_watchpoint (int cnt) > +s390_region_ok_for_hw_watchpoint (CORE_ADDR *, int cnt) > { > return 1; > } > @@ -380,7 +380,7 @@ > > /* Add our watchpoint methods. */ > t->to_can_use_hw_breakpoint = s390_can_use_hw_breakpoint; > - t->to_region_size_ok_for_hw_watchpoint = s390_region_size_ok_for_hw_watchpoint; > + t->to_region_ok_for_hw_watchpoint = s390_region_ok_for_hw_watchpoint; > t->to_have_continuable_watchpoint = 1; > t->to_stopped_by_watchpoint = s390_stopped_by_watchpoint; > > and so on. > > P.S: If I can understand and also I understand correctly, it is not hard. > Sometimes I just need a little more time to understand the precise > meaning of your words. Most of the time I attribute it to my english. > > ;-) > > Regards > - Wu Zhou > >