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

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

[Pierre Muller]

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.

  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.


I tried a similar patch on an i386 open solaris and it seems to
give almost unchanged results for the testsuite.
(Beware that the patch below is not exactly the one I tested, because I
couldn't
retrieve it...)


Who are the maintainers of
 - i386 solaris
 - sparc solaris
and
 - mips irix?

 
Comments most welcome,



Pierre Muller
Pascal language support maintainer for GDB



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.

Index: procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.104
diff -u -p -r1.104 procfs.c
--- procfs.c	16 Apr 2009 18:41:40 -0000	1.104
+++ procfs.c	22 Apr 2009 22:54:05 -0000
@@ -223,6 +223,56 @@ procfs_target (void)
   return t;
 }
 
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+static int procfs_stopped_by_watchpoint ();
+
+static int
+procfs_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  return procfs_set_watchpoint (inferior_ptid, addr, len, type, 
+				STOP_AFTER_WATCHPOINT);
+}
+
+static int
+procfs_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  procfs_set_watchpoint (inferior_ptid, addr, 0, 0, 0);
+}
+
+static int
+procfs_insert_hw_breakpoint (struct bp_target_info *bp_tgt)
+{
+  CORE_ADDR addr = bp_tgt->placed_address;
+  return procfs_set_watchpoint (inferior_ptid, addr, 1, hw_execute, 0);
+}
+
+static int
+procfs_remove_hw_breakpoint (struct bp_target_info *bp_tgt)
+{
+  CORE_ADDR addr = bp_tgt->placed_address;
+  procfs_set_watchpoint (inferior_ptid, addr, 0, 0, 0);
+}
+
+
+void
+procfs_use_watchpoints (struct target_ops *t)
+{
+  // region_ok_for_hw_watchpoint has a default function
+  // t->to_region_ok_for_hw_watchpoint = procfs_region_ok_for_watchpoint;
+  t->to_stopped_by_watchpoint = procfs_stopped_by_watchpoint;
+  /* FIXME: Is it possible in procfs to know the address of the triggered
+     watchpoint?  */
+  /* t->to_stopped_data_address = procfs_stopped_data_address;
+     this function does not exist yet.  */
+  t->to_insert_watchpoint = procfs_insert_watchpoint;
+  t->to_remove_watchpoint = procfs_remove_watchpoint;
+  t->to_insert_hw_breakpoint = procfs_insert_hw_breakpoint;
+  t->to_remove_hw_breakpoint = procfs_remove_hw_breakpoint;
+}
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
+
 /* =================== END, TARGET_OPS "MODULE" =================== */
 
 /*
@@ -5321,13 +5371,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 ()
 {
   procinfo *pi;
 
-  pi = find_procinfo_or_die (PIDGET (ptid) == -1 ?
-			     PIDGET (inferior_ptid) : PIDGET (ptid), 0);
+  pi = find_procinfo_or_die (PIDGET (inferior_ptid));
 
   if (!pi)	/* If no process, then not stopped by watchpoint!  */
     return 0;
@@ -5970,6 +6019,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: config/i386/nm-i386sol2.h
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/nm-i386sol2.h,v
retrieving revision 1.16
diff -u -p -r1.16 nm-i386sol2.h
--- config/i386/nm-i386sol2.h	3 Jan 2009 05:57:54 -0000	1.16
+++ config/i386/nm-i386sol2.h	22 Apr 2009 22:52:44 -0000
@@ -19,6 +19,7 @@
 #ifdef NEW_PROC_API	/* Solaris 6 and above can do HW watchpoints */
 
 #define TARGET_HAS_HARDWARE_WATCHPOINTS
+#define STOP_AFTER_WATCHPOINT 1
 
 /* The man page for proc4 on solaris 6 and 7 says that the system
    can support "thousands" of hardware watchpoints, but gives no
@@ -41,17 +42,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: config/mips/nm-irix5.h
===================================================================
RCS file: /cvs/src/src/gdb/config/mips/nm-irix5.h,v
retrieving revision 1.18
diff -u -p -r1.18 nm-irix5.h
--- config/mips/nm-irix5.h	3 Jan 2009 05:57:55 -0000	1.18
+++ config/mips/nm-irix5.h	22 Apr 2009 22:52:44 -0000
@@ -19,6 +19,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/
 
 #define TARGET_HAS_HARDWARE_WATCHPOINTS
+#define STOP_AFTER_WATCHPOINT 0
 
 /* TARGET_CAN_USE_HARDWARE_WATCHPOINT is now defined to go through
    the target vector.  For Irix5, procfs_can_use_hw_watchpoint()
@@ -28,17 +29,5 @@
    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: config/sparc/nm-sol2.h
===================================================================
RCS file: /cvs/src/src/gdb/config/sparc/nm-sol2.h,v
retrieving revision 1.7
diff -u -p -r1.7 nm-sol2.h
--- config/sparc/nm-sol2.h	3 Jan 2009 05:57:56 -0000	1.7
+++ config/sparc/nm-sol2.h	22 Apr 2009 22:52:45 -0000
@@ -29,6 +29,7 @@
 #ifdef NEW_PROC_API
 
 #define TARGET_HAS_HARDWARE_WATCHPOINTS
+#define STOP_AFTER_WATCHPOINT 1
 
 /* 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
@@ -42,19 +43,6 @@
    *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 */

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

[Joel Brobecker]

Before looking at the patch itself, let me answer the following
question:

> Who are the maintainers of
>  - i386 solaris
>  - sparc solaris
> and
>  - mips irix?

I don't think we have any specific maintainers for these ports.
I will look at the patch, and test them on i386/sparc solaris.
I will also try to give mips-irix a shot at seeing if watchpoints
still work after your patch.


-- 
Joel

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

[Pedro Alves]

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 */

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

[Pedro Alves]

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 */

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

[Joel Brobecker]

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

I won't have much time in the next few weeks, unfortunately :-(.
But I did run it through the testsuite. The results are really abysmal
but no regression.

        # of expected passes            8361
        # of unexpected failures        2114
        # of unexpected successes       4
        # of expected failures          33
        # of known failures             29
        # of unresolved testcases       33
        # of untested testcases         72
        # of unsupported tests          30

-- 
Joel

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

[Pedro Alves]

On Wednesday 06 May 2009 17:54:25, Joel Brobecker wrote:
> > 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?
> 
> I won't have much time in the next few weeks, unfortunately :-(.
> But I did run it through the testsuite. The results are really abysmal
> but no regression.

Okay, I'll check it in then.


>         # of expected passes            8361
>         # of unexpected failures        2114
>         # of unexpected successes       4
>         # of expected failures          33
>         # of known failures             29
>         # of unresolved testcases       33
>         # of untested testcases         72
>         # of unsupported tests          30

It seems most failures on solaris are related to a single problem, e.g., 

p/c fun()
$1 = 49 '<error reading variable>
(gdb) FAIL: gdb.base/call-sc.exp: p/c fun(); call call-sc-tc

>grep "error reading variable" gdb.log  | wc -l
1312

Looks like things are *really* broken.  That's around
as many failures as instances of that error:

# of expected passes            10933
# of unexpected failures        1337
# of expected failures          41
# of known failures             54
# of unresolved testcases       9
# of untested testcases         13
# of unsupported tests          67

Does this ring a bell?  I'll look at this at some point,
but probably only a couple of weeks or more from now.

-- 
Pedro Alves

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

[Joel Brobecker]

> It seems most failures on solaris are related to a single problem, e.g., 
> 
> p/c fun()
> $1 = 49 '<error reading variable>
> (gdb) FAIL: gdb.base/call-sc.exp: p/c fun(); call call-sc-tc
> 
> >grep "error reading variable" gdb.log  | wc -l
> 1312

Yeah, I have roughly the same number of hits for that one.
That leaves another thousand of so errors. As far as I can tell,
they are made of several different symptoms: ~100 internal-errors,
about a 100 are caused by the fact that we failed to run the program
or ran the program to completion, etc etc etc.

I can send you the gdb.log file if you're interested. I suspect not ;-).

-- 
Joel

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

[Pedro Alves]

On Wednesday 06 May 2009 19:35:42, Joel Brobecker wrote:
> Yeah, I have roughly the same number of hits for that one.
> That leaves another thousand of so errors. As far as I can tell,
> they are made of several different symptoms: ~100 internal-errors,
> about a 100 are caused by the fact that we failed to run the program
> or ran the program to completion, etc etc etc.

I think that those other problems may be related to the issue
I mentioned on IRC the other day.  The fact that the procfs.c
code that sets a breakpoint at __dbx_link during the startup phase
is broken.  I'll paste it here for posterity.  :-)

- That breakpoint was set during the startup phase by catching
  syscall-exits during the startup phase, more precisely, during
  the phase that the inferior is ran through the dynamic linker in
  solib_create_inferior_hook -> irix_solib_create_inferior_hook.

  This was done by enabling PR_SYSEXIT syscall tracing in procfs_init_inferior,
  and disabling after fork_inferior returned.  Look for SYS_syssgi in
  procfs.c to see this. 

  This worked because many moons ago, at the time that code was
  written, fork_inferior used to call solib_create_inferior_hook before
  returning.

  http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/fork-child.c.diff?r1=1.28&r2=1.27.6.1&cvsroot=src&f=h

- Problem is, that now, solib_create_inferior is only called *after* all of this, in
  post_create_inferior.  So the wrapping to catch syscall-exits is being done
  at the wrong place.

- You probably need to delay disabling catching syscall-exits/looking
  for __dbx_link into the inferior_created observer, instead of disabling
  it at the end of procfs_create_inferior.

> I can send you the gdb.log file if you're interested. I suspect not ;-).

Not.  :-)

-- 
Pedro Alves