Re: [RFC] Remove last occurences of target_{insert/remove}_watchpoint

[Pedro Alves <pedro at codesourcery dot com>]

On Thursday 30 April 2009 00:00:26, Pedro Alves wrote:

> 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?


I've just ran this version through sparc solaris 8, and
it survived without regressions (although it looks like solaris test
results got much worse somewhere along these last weeks -- around
1300 fails), and watchpoints still work.  Joel, would you like to
take a look at this and/or ran it on mips-irix?


> 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.
> 

-- 
Pedro Alves

2009-05-04  Pierre Muller  <muller.u-strasbg.fr>
	    Pedro Alves  <pedro@codesourcery.com>

	* 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                  |   58 +++++++++++++++++++++++++++++++++++++++---
 4 files changed, 54 insertions(+), 76 deletions(-)

Index: src/gdb/procfs.c
===================================================================
--- src.orig/gdb/procfs.c	2009-04-30 11:39:18.000000000 +0100
+++ src/gdb/procfs.c	2009-05-04 12:42:31.000000000 +0100
@@ -5321,13 +5321,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;
@@ -5349,6 +5348,53 @@ procfs_stopped_by_watchpoint (ptid_t pti
   return 0;
 }
 
+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)
+{
+  return 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;
+}
+
 /*
  * Memory Mappings Functions:
  */
@@ -5970,6 +6016,10 @@ _initialize_procfs (void)
 
   t = procfs_target ();
 
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+  procfs_use_watchpoints (t);
+#endif
+
   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-04-30 11:39:17.000000000 +0100
+++ src/gdb/config/i386/nm-i386sol2.h	2009-05-04 11:33:44.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-04-30 11:39:18.000000000 +0100
+++ src/gdb/config/mips/nm-irix5.h	2009-05-04 11:33:44.000000000 +0100
@@ -19,26 +19,3 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #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-04-30 11:39:17.000000000 +0100
+++ src/gdb/config/sparc/nm-sol2.h	2009-05-04 11:33:45.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 */