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

[Pedro Alves <pedro@codesourcery.com>]

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  <muller.u-strasbg.fr>
> 
> 	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  <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                  |   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 <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-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 */