From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24540 invoked by alias); 29 Apr 2009 23:00:30 -0000 Received: (qmail 24510 invoked by uid 22791); 29 Apr 2009 23:00:27 -0000 X-SWARE-Spam-Status: No, hits=-0.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS,URIBL_BLACK 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; Wed, 29 Apr 2009 23:00:21 +0000 Received: (qmail 6490 invoked from network); 29 Apr 2009 23:00:18 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Apr 2009 23:00:18 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFC] Remove last occurences of target_{insert/remove}_watchpoint Date: Wed, 29 Apr 2009 23:00:00 -0000 User-Agent: KMail/1.9.10 Cc: "Pierre Muller" References: <001a01c9c39f$e3f217c0$abd64740$@u-strasbg.fr> In-Reply-To: <001a01c9c39f$e3f217c0$abd64740$@u-strasbg.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200904300000.26661.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-04/txt/msg00800.txt.bz2 On Thursday 23 April 2009 00:13:03, Pierre Muller wrote: > The two macros target_insert_watchpoint and target_remove_watchpoint > and only still defined inn three config files. > > These macros are really bad, because that > forbid remote target to work properly on the targets > that define them. Right on. > The patch below tries to get rid of them, > by defining appropriate target functions. > > I needed to insert another new macros that I called > STOP_AFTER_WATCHPOINT because out of three, two of these targets > call procfs_set_watchpoint with after_trap set to one, > and the third (mips/nm-irix5.h) set to zero. i386-solaris and sparc-solaris can select between continuable and non-continuable watchpoints through the WA_TRAPAFTER flag, which is what the after_trap flag controls. In the GDB user's perpective, watchpoints are always reported after the memory access having happened. If behind the scenes, they don't trap after the memory access, then GDB single steps once to finish the memory access. This latter case is the that of the mips architecture, mips-irix included. See: #else /* Irix method for watchpoints */ enum { READ_WATCHFLAG = MA_READ, WRITE_WATCHFLAG = MA_WRITE, EXEC_WATCHFLAG = MA_EXEC, AFTER_WATCHFLAG = 0 /* trapafter not implemented */ }; #endif Unfortunatelly, infrun.c's way of checking for continuable/non-continuable watchpoints is full of historic twists. There's a to_have_continuable_watchpoint target_ops flag, but nothing uses it anymore. Instead, continuable == (!steppable && !non-steppable), but, `steppable' is a target property (to_have_steppable_watchpoint), and non_steppable is a gdbarch property (gdbarch_have_nonsteppable_watchpoint). Knowing that, we can avoid introducing that STOP_AFTER_WATCHPOINT macro. Instead do the same checks infrun.c wants to do. As I was describing this, I went ahead and cleaned up your patch, as below. Completely untested. Care to take a look? And since you got me started, ..., notice that TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT was at some point converted to TARGET_REGION_OK_FOR_HW_WATCHPOINT, but, these procfs uses were missed in the convertion. procfs allows setting watchpoints watching large memory regions, but, it that support has been broken since. While I'm here, I'm converting TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT to the proper target method. TARGET_CAN_USE_HARDWARE_WATCHPOINT is not used anymore, so we can remove those, and same for HAVE_CONTINUABLE_WATCHPOINT. Note that this isn't ideal yet, but, cleaning this up better involves better splitting of procfs.c into further *-nat.c files that inherit and extend the return of procfs_target. It is now getting easier to do it though. > 2009-04-23 Pierre Muller > > Remove target_insert_watchpoint and target_remove_watchpoint. > * procfs.c (procfs_insert_watchpoint, procfs_remove_watchpoint) > (procfs_insert_hw_breakpoint, procfs_remove_hw_breakpoint) > (procfs_use_watchpoints): > New functions. > (procfs_stopped_by_watchpoint): Made static, ptid type > argument removed. > (_initialize_procfs): Register new watchpoint related > target functions. > * config/i386/nm-i386sol2.h (STOP_AFTER_WATCHPOINT): New macro. > (target_insert_watchpoint, target_remove_watchpoint): Delete. > * config/mips/nm-irix5.h: Idem. > * config/sparc/nm-sol2.h: Idem. You were implementing hw breakpoints in this patch. Let's not mix cleaning up with new functionality, please. -- Pedro Alves 2009-04-29 Pierre Muller Pedro Alves * procfs.c (procfs_insert_watchpoint, procfs_remove_watchpoint) (procfs_region_ok_for_hw_watchpoint, procfs_use_watchpoints): New functions. (procfs_stopped_by_watchpoint): Made static, ptid argument removed. (_initialize_procfs): Register new watchpoint related target functions. * config/i386/nm-i386sol2.h (TARGET_CAN_USE_HARDWARE_WATCHPOINT) (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT, STOPPED_BY_WATCHPOINT) (HAVE_CONTINUABLE_WATCHPOINT): Delete. (target_insert_watchpoint, target_remove_watchpoint): Delete. (procfs_stopped_by_watchpoint, procfs_set_watchpoint): Delete declarations. * config/mips/nm-irix5.h (STOPPED_BY_WATCHPOINT) (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete. (target_insert_watchpoint, target_remove_watchpoint): Delete. (procfs_stopped_by_watchpoint, procfs_set_watchpoint): Delete declarations. * config/sparc/nm-sol2.h (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT) (HAVE_CONTINUABLE_WATCHPOINT, STOPPED_BY_WATCHPOINT): Delete. (target_insert_watchpoint, target_remove_watchpoint): Delete. (procfs_stopped_by_watchpoint, procfs_set_watchpoint): Delete declarations. --- gdb/config/i386/nm-i386sol2.h | 24 --------------- gdb/config/mips/nm-irix5.h | 23 --------------- gdb/config/sparc/nm-sol2.h | 25 ---------------- gdb/procfs.c | 64 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 60 insertions(+), 76 deletions(-) Index: src/gdb/procfs.c =================================================================== --- src.orig/gdb/procfs.c 2009-04-16 20:04:52.000000000 +0100 +++ src/gdb/procfs.c 2009-04-29 23:46:43.000000000 +0100 @@ -153,6 +153,8 @@ static char * procfs_make_note_section ( static int procfs_can_use_hw_breakpoint (int, int, int); +static int procfs_stopped_by_watchpoint (void); + #if defined (PR_MODEL_NATIVE) && (PR_MODEL_NATIVE == PR_MODEL_LP64) /* When GDB is built as 64-bit application on Solaris, the auxv data is presented in 64-bit format. We need to provide a custom parser to handle @@ -181,6 +183,57 @@ procfs_auxv_parse (struct target_ops *op } #endif +#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS + +static int +procfs_insert_watchpoint (CORE_ADDR addr, int len, int type) +{ + if (!HAVE_STEPPABLE_WATCHPOINT + && !gdbarch_have_nonsteppable_watchpoint (current_gdbarch)) + { + /* When a hardware watchpoint fires off the PC will be left at + the instruction following the one which caused the + watchpoint. It will *NOT* be necessary for GDB to step over + the watchpoint. */ + return procfs_set_watchpoint (inferior_ptid, addr, len, type, 1); + } + else + { + /* When a hardware watchpoint fires off the PC will be left at + the instruction which caused the watchpoint. It will be + necessary for GDB to step over the watchpoint. */ + return procfs_set_watchpoint (inferior_ptid, addr, len, type, 0); + } +} + +static int +procfs_remove_watchpoint (CORE_ADDR addr, int len, int type) +{ + procfs_set_watchpoint (inferior_ptid, addr, 0, 0, 0); +} + +static int +procfs_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) +{ + /* The man page for proc(4) on Solaris 2.6 and up says that the + system can support "thousands" of hardware watchpoints, but gives + no method for finding out how many; It doesn't say anything about + the allowed size for the watched area either. So we just tell + GDB 'yes'. */ + return 1; +} + +void +procfs_use_watchpoints (struct target_ops *t) +{ + t->to_stopped_by_watchpoint = procfs_stopped_by_watchpoint; + t->to_insert_watchpoint = procfs_insert_watchpoint; + t->to_remove_watchpoint = procfs_remove_watchpoint; + t->to_region_ok_for_hw_watchpoint = procfs_region_ok_for_hw_watchpoint; +} + +#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */ + static struct target_ops * procfs_target (void) { @@ -5321,13 +5374,12 @@ procfs_can_use_hw_breakpoint (int type, * else returns zero. */ -int -procfs_stopped_by_watchpoint (ptid_t ptid) +static int +procfs_stopped_by_watchpoint (void) { procinfo *pi; - pi = find_procinfo_or_die (PIDGET (ptid) == -1 ? - PIDGET (inferior_ptid) : PIDGET (ptid), 0); + pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0); if (!pi) /* If no process, then not stopped by watchpoint! */ return 0; @@ -5970,6 +6022,10 @@ _initialize_procfs (void) t = procfs_target (); +#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS + procfs_use_watchpoints (t); +#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */ + add_target (t); add_info ("proc", info_proc_cmd, _("\ Index: src/gdb/config/i386/nm-i386sol2.h =================================================================== --- src.orig/gdb/config/i386/nm-i386sol2.h 2009-01-03 05:57:54.000000000 +0000 +++ src/gdb/config/i386/nm-i386sol2.h 2009-04-29 23:45:12.000000000 +0100 @@ -20,17 +20,6 @@ #define TARGET_HAS_HARDWARE_WATCHPOINTS -/* The man page for proc4 on solaris 6 and 7 says that the system - can support "thousands" of hardware watchpoints, but gives no - method for finding out how many. So just tell GDB 'yes'. */ -#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE, CNT, OT) 1 -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1 - -/* When a hardware watchpoint fires off the PC will be left at the - instruction following the one which caused the watchpoint. - It will *NOT* be necessary for GDB to step over the watchpoint. */ -#define HAVE_CONTINUABLE_WATCHPOINT 1 - /* Solaris x86 2.6 and 2.7 targets have a kernel bug when stepping over an instruction that causes a page fault without triggering a hardware watchpoint. The kernel properly notices that it shouldn't @@ -41,17 +30,4 @@ step anyway. */ #define CANNOT_STEP_HW_WATCHPOINTS -extern int procfs_stopped_by_watchpoint (ptid_t); -#define STOPPED_BY_WATCHPOINT(W) \ - procfs_stopped_by_watchpoint(inferior_ptid) - -/* Use these macros for watchpoint insertion/deletion. */ -/* type can be 0: write watch, 1: read watch, 2: access watch (read/write) */ - -extern int procfs_set_watchpoint (ptid_t, CORE_ADDR, int, int, int); -#define target_insert_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, LEN, TYPE, 1) -#define target_remove_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, 0, 0, 0) - #endif /* NEW_PROC_API */ Index: src/gdb/config/mips/nm-irix5.h =================================================================== --- src.orig/gdb/config/mips/nm-irix5.h 2009-01-03 05:57:55.000000000 +0000 +++ src/gdb/config/mips/nm-irix5.h 2009-04-29 23:45:27.000000000 +0100 @@ -19,26 +19,3 @@ along with this program. If not, see . */ #define TARGET_HAS_HARDWARE_WATCHPOINTS - -/* TARGET_CAN_USE_HARDWARE_WATCHPOINT is now defined to go through - the target vector. For Irix5, procfs_can_use_hw_watchpoint() - should be invoked. */ - -/* When a hardware watchpoint fires off the PC will be left at the - instruction which caused the watchpoint. It will be necessary for - GDB to step over the watchpoint. */ - -#define STOPPED_BY_WATCHPOINT(W) \ - procfs_stopped_by_watchpoint(inferior_ptid) -extern int procfs_stopped_by_watchpoint (ptid_t); - -/* Use these macros for watchpoint insertion/deletion. */ -/* type can be 0: write watch, 1: read watch, 2: access watch (read/write) */ -#define target_insert_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, LEN, TYPE, 0) -#define target_remove_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, 0, 0, 0) -extern int procfs_set_watchpoint (ptid_t, CORE_ADDR, int, int, int); - -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1 - Index: src/gdb/config/sparc/nm-sol2.h =================================================================== --- src.orig/gdb/config/sparc/nm-sol2.h 2009-01-03 05:57:56.000000000 +0000 +++ src/gdb/config/sparc/nm-sol2.h 2009-04-29 23:43:44.000000000 +0100 @@ -30,31 +30,6 @@ #define TARGET_HAS_HARDWARE_WATCHPOINTS -/* The man page for proc(4) on Solaris 2.6 and up says that the system - can support "thousands" of hardware watchpoints, but gives no - method for finding out how many; It doesn't say anything about the - allowed size for the watched area either. So we just tell GDB - 'yes'. */ -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1 - -/* When a hardware watchpoint fires off the PC will be left at the - instruction following the one which caused the watchpoint. It will - *NOT* be necessary for GDB to step over the watchpoint. */ -#define HAVE_CONTINUABLE_WATCHPOINT 1 - -extern int procfs_stopped_by_watchpoint (ptid_t); -#define STOPPED_BY_WATCHPOINT(W) \ - procfs_stopped_by_watchpoint(inferior_ptid) - -/* Use these macros for watchpoint insertion/deletion. TYPE can be 0 - (write watch), 1 (read watch), 2 (access watch (read/write). */ - -extern int procfs_set_watchpoint (ptid_t, CORE_ADDR, int, int, int); -#define target_insert_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, LEN, TYPE, 1) -#define target_remove_watchpoint(ADDR, LEN, TYPE) \ - procfs_set_watchpoint (inferior_ptid, ADDR, 0, 0, 0) - #endif /* NEW_PROC_API */ #endif /* nm-sol2.h */