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

Pedro Alves
2009-05-04T19:32:46Z

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