[RFC] GDB patches for hw watchpoints - revised
Wu Zhou
woodzltc@cn.ibm.com
Tue Jan 24 11:00:00 GMT 2006
Hi Daniel,
Because there are quite a few places in different arch/target of GDB which
use region_size_ok_for_hw_watchpoint, so I am prefering to get this done
in two steps: first add to_region_ok_for_hw_watchpoint into struct
target_ops, then replace these to_region_size_ok_for_hw_watchpoint
reference with to_region_ok_for_hw_watchpoint ones. IMHO, it is easier to
not confuse the original intention of this patch with this replacement.
What is your thought on this?
Here is the modified patch following this point. Please review.
P.S: I tested it on ppc64 with 2.6.15-git8 kernel (without any patch this
time, Anton had check the needed code into the official kernel source
tree). It works ok. I can see six more PASS on gdb.base/recurse.exp; the
other seven failure are around the second watchpoint, which is S/W one.
No regression is found on other testcases. I also tested this on old
PPC kernel which don't support DABR, don't find any regression with the
introduction of this patch.
2006-01-22 Ben Elliston <bje@au1.ibm.com>
Wu Zhou <woodzltc@cn.ibm.com>
* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
PTRACE_GETSIGINFO): Define.
(last_stopped_data_address): New.
(ppc_linux_check_watch_resources): New function to check whether
the target has available hardware watchpoint resource.
(ppc_linux_region_ok_for_hw_watchpoint): New function to check if
the region is ok for hardware watchpoint.
(ppc_linux_insert_watchpoint): New function to insert a hardware
watchpoint.
(ppc_linux_remove_watchpoint): New function to remove a hardware
watchpoint.
(ppc_linux_stopped_data_address): New function to get the stopped
data address of the watchpoint.
(ppc_linux_stopped_by_watchpoint): New function to see if the
inferior is stopped by watchpoint.
(_initialize_ppc_linux_nat): Set the above hardware watchpoint
related target vectors.
* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
to have nonsteppable watchpoint.
* target.c (default_region_ok_for_hw_watchpoint,
debug_to_region_ok_for_hw_watchpoint): New prototypes.
(update_current_target): Inherit to_region_ok_for_hw_watchpoint
and set default to_region_ok_for_hw_watchpoint.
(default_region_ok_for_hw_watchpoint): New function.
(debug_to_region_ok_for_hw_watchpoint): New function.
(setup_target_debug): Set to_region_ok_for_hw_watchpoint of
debug_target.
target.h (struct target_ops): Add a new target vector
to_region_ok_for_hw_watchpoint.
(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
defined anyplace else.
Index: ppc-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v
retrieving revision 1.55
diff -u -p -r1.55 ppc-linux-nat.c
--- ppc-linux-nat.c 10 Sep 2005 18:11:04 -0000 1.55
+++ ppc-linux-nat.c 24 Jan 2006 09:19:06 -0000
@@ -81,6 +81,16 @@
#define PTRACE_SETEVRREGS 21
#endif
+/* Similarly for the hardware watchpoint support. */
+#ifndef PTRACE_GET_DEBUGREG
+#define PTRACE_GET_DEBUGREG 25
+#endif
+#ifndef PTRACE_SET_DEBUGREG
+#define PTRACE_SET_DEBUGREG 26
+#endif
+#ifndef PTRACE_GETSIGINFO
+#define PTRACE_GETSIGINFO 0x4202
+#endif
/* This oddity is because the Linux kernel defines elf_vrregset_t as
an array of 33 16 bytes long elements. I.e. it leaves out vrsave.
@@ -146,6 +156,7 @@ struct gdb_evrregset_t
error. */
int have_ptrace_getvrregs = 1;
+static CORE_ADDR last_stopped_data_address = 0;
/* Non-zero if our kernel may support the PTRACE_GETEVRREGS and
PTRACE_SETEVRREGS requests, for reading and writing the SPE
@@ -153,7 +164,6 @@ int have_ptrace_getvrregs = 1;
error. */
int have_ptrace_getsetevrregs = 1;
-
int
kernel_u_size (void)
{
@@ -777,6 +787,124 @@ store_ppc_registers (int tid)
store_spe_register (tid, -1);
}
+static int
+ppc_linux_check_watch_resources (int type, int cnt, int ot)
+{
+ int tid;
+ ptid_t ptid = inferior_ptid;
+
+ /* DABR (data address breakpoint register) is optional for PPC variants.
+ Some variants have one DABR, others have none. So CNT can't be larger
+ than 1. */
+ if (cnt > 1)
+ return 0;
+
+ /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether
+ the target has DABR. If either answer is no, the ptrace call will
+ return -1. Fail in that case. */
+ tid = TIDGET (ptid);
+ if (tid == 0)
+ tid = PIDGET (ptid);
+
+ if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1)
+ return 0;
+ return 1;
+}
+
+static int
+ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ /* Handle sub-8-byte quantities. */
+ if (len <= 0)
+ return 0;
+
+ /* addr+len must fall in the 8 byte watchable region. */
+ if ((addr + len) > (addr & ~7) + 8)
+ return 0;
+
+ return 1;
+}
+
+/* Set a watchpoint of type TYPE at address ADDR. */
+static long
+ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
+{
+ int tid;
+ long dabr_value;
+ ptid_t ptid = inferior_ptid;
+
+ dabr_value = addr & ~7;
+ switch (rw)
+ {
+ case hw_read:
+ /* Set read and translate bits. */
+ dabr_value |= 5;
+ break;
+ case hw_write:
+ /* Set write and translate bits. */
+ dabr_value |= 6;
+ break;
+ case hw_access:
+ /* Set read, write and translate bits. */
+ dabr_value |= 7;
+ break;
+ }
+
+ tid = TIDGET (ptid);
+ if (tid == 0)
+ tid = PIDGET (ptid);
+
+ return ptrace (PTRACE_SET_DEBUGREG, tid, 0, dabr_value);
+}
+
+static long
+ppc_linux_remove_watchpoint (CORE_ADDR addr, int len)
+{
+ int tid;
+ ptid_t ptid = inferior_ptid;
+
+ tid = TIDGET (ptid);
+ if (tid == 0)
+ tid = PIDGET (ptid);
+
+ return ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0);
+}
+
+static int
+ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
+{
+ if (last_stopped_data_address)
+ {
+ *addr_p = last_stopped_data_address;
+ last_stopped_data_address = 0;
+ return 1;
+ }
+ return 0;
+}
+
+static int
+ppc_linux_stopped_by_watchpoint (void)
+{
+ int tid;
+ struct siginfo siginfo;
+ ptid_t ptid = inferior_ptid;
+ CORE_ADDR *addr_p;
+
+ tid = TIDGET(ptid);
+ if (tid == 0)
+ tid = PIDGET (ptid);
+
+ errno = 0;
+ ptrace (PTRACE_GETSIGINFO, tid, (PTRACE_TYPE_ARG3) 0, &siginfo);
+
+ if (errno != 0 || siginfo.si_signo != SIGTRAP ||
+ (siginfo.si_code & 0xffff) != 0x0004)
+ return 0;
+
+ last_stopped_data_address = (CORE_ADDR) siginfo.si_addr;
+ return 1;
+}
+
static void
ppc_linux_store_inferior_registers (int regno)
{
@@ -900,6 +1028,14 @@ _initialize_ppc_linux_nat (void)
t->to_fetch_registers = ppc_linux_fetch_inferior_registers;
t->to_store_registers = ppc_linux_store_inferior_registers;
+ /* Add our watchpoint methods. */
+ t->to_can_use_hw_breakpoint = ppc_linux_check_watch_resources;
+ t->to_region_ok_for_hw_watchpoint = ppc_linux_region_ok_for_hw_watchpoint;
+ t->to_insert_watchpoint = ppc_linux_insert_watchpoint;
+ t->to_remove_watchpoint = ppc_linux_remove_watchpoint;
+ t->to_stopped_by_watchpoint = ppc_linux_stopped_by_watchpoint;
+ t->to_stopped_data_address = ppc_linux_stopped_data_address;
+
/* Register the target. */
add_target (t);
}
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 rs6000-tdep.c
--- rs6000-tdep.c 1 Nov 2005 19:32:36 -0000 1.248
+++ rs6000-tdep.c 24 Jan 2006 09:19:08 -0000
@@ -3278,6 +3278,10 @@ rs6000_gdbarch_init (struct gdbarch_info
_("rs6000_gdbarch_init: "
"received unexpected BFD 'arch' value"));
+ /* P630 has nonsteppable watchpoint. So we are assuming that all PowerPC
+ targets have nonsteppable watchpoint. */
+ set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
+
/* Sanity check on registers. */
gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.111
diff -u -p -r1.111 target.c
--- target.c 4 Sep 2005 16:18:20 -0000 1.111
+++ target.c 24 Jan 2006 09:19:09 -0000
@@ -47,6 +47,8 @@ static void kill_or_be_killed (int);
static void default_terminal_info (char *, int);
+static int default_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
static int default_region_size_ok_for_hw_watchpoint (int);
static int nosymbol (char *, CORE_ADDR *);
@@ -128,6 +130,8 @@ static int debug_to_stopped_by_watchpoin
static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *);
+static int debug_to_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
static int debug_to_region_size_ok_for_hw_watchpoint (int);
static void debug_to_terminal_init (void);
@@ -405,6 +409,7 @@ update_current_target (void)
INHERIT (to_stopped_data_address, t);
INHERIT (to_stopped_by_watchpoint, t);
INHERIT (to_have_continuable_watchpoint, t);
+ INHERIT (to_region_ok_for_hw_watchpoint, t);
INHERIT (to_region_size_ok_for_hw_watchpoint, t);
INHERIT (to_terminal_init, t);
INHERIT (to_terminal_inferior, t);
@@ -531,6 +536,8 @@ update_current_target (void)
de_fault (to_stopped_data_address,
(int (*) (struct target_ops *, CORE_ADDR *))
return_zero);
+ de_fault (to_region_ok_for_hw_watchpoint,
+ default_region_ok_for_hw_watchpoint);
de_fault (to_region_size_ok_for_hw_watchpoint,
default_region_size_ok_for_hw_watchpoint);
de_fault (to_terminal_init,
@@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec
}
static int
+default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ return (len <= TYPE_LENGTH (builtin_type_void_data_ptr));
+}
+
+static int
default_region_size_ok_for_hw_watchpoint (int byte_count)
{
return (byte_count <= TYPE_LENGTH (builtin_type_void_data_ptr));
@@ -2115,6 +2128,21 @@ debug_to_can_use_hw_breakpoint (int type
}
static int
+debug_to_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ CORE_ADDR retval;
+
+ retval = debug_target.to_region_ok_for_hw_watchpoint (addr, len);
+
+ fprintf_unfiltered (gdb_stdlog,
+ "TARGET_REGION_OK_FOR_HW_WATCHPOINT (%ld, %ld) = 0x%lx\n",
+ (unsigned long) addr,
+ (unsigned long) len,
+ (unsigned long) retval);
+ return retval;
+}
+
+static int
debug_to_region_size_ok_for_hw_watchpoint (int byte_count)
{
CORE_ADDR retval;
@@ -2533,6 +2561,7 @@ setup_target_debug (void)
current_target.to_remove_watchpoint = debug_to_remove_watchpoint;
current_target.to_stopped_by_watchpoint = debug_to_stopped_by_watchpoint;
current_target.to_stopped_data_address = debug_to_stopped_data_address;
+ current_target.to_region_ok_for_hw_watchpoint = debug_to_region_ok_for_hw_watchpoint;
current_target.to_region_size_ok_for_hw_watchpoint = debug_to_region_size_ok_for_hw_watchpoint;
current_target.to_terminal_init = debug_to_terminal_init;
current_target.to_terminal_inferior = debug_to_terminal_inferior;
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.76
diff -u -p -r1.76 target.h
--- target.h 4 Sep 2005 16:18:20 -0000 1.76
+++ target.h 24 Jan 2006 09:19:09 -0000
@@ -345,6 +345,7 @@ struct target_ops
int (*to_stopped_by_watchpoint) (void);
int to_have_continuable_watchpoint;
int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
+ int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
int (*to_region_size_ok_for_hw_watchpoint) (int);
void (*to_terminal_init) (void);
void (*to_terminal_inferior) (void);
@@ -1030,6 +1031,11 @@ extern void (*deprecated_target_new_objf
(*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
#endif
+#ifndef TARGET_REGION_OK_FOR_HW_WATCHPOINT
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len) \
+ (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
+#endif
+
#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
(*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)
Regards
- Wu Zhou
On Tue, 24 Jan 2006, Wu Zhou wrote:
>
> On Mon, 23 Jan 2006, Daniel Jacobowitz wrote:
>
> > On Tue, Jan 24, 2006 at 11:40:16AM +0800, Wu Zhou wrote:
> > > p630 is one kind of POWER4 based pSeriese server. It is currently the only
> > > available ppc machine I can get. :-)
> > >
> > > In fact, I am not sure before if the ppc arch has nonsteppable watchpoints
> > > or not. But testing on my p630 box, it did had nonsteppable ones. Now
> > > that an architecture either have or doesn't have nonsteppable watchpoints,
> > > can we get from this test a result that ppc architecture has nonsteppable
> > > watchpoints?
> > >
> > > If so, maybe I can just remove the stupid conditional statement below.
> > > (my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)
> >
> > Well, it'd be nice to have some architectural reference for this. But
> > it's probably a safe bet to assume that this is generally true for all
> > PowerPC targets, so let's just assume it.
>
> OK. I will assume this then.
>
> > > Function to_region_ok_for_hw_watchpoint is not in the current target
> > > vector (I means struct target_ops). Maybe we can add it into
> > > target_ops? There are a few other archs also use this. But they had to
> > > include it in nm-xxx-yyy.h. If not, the only method I can think of is
> > > also include its definition in nm-ppc64-linux.h. So what about the
> > > following patch section?
> > >
> > > int (*to_region_size_ok_for_hw_watchpoint) (int);
> > > + int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
> > > void (*to_terminal_init) (void);
> >
> > I would recommend replacing to_region_size_ok_for_hw_watchpoint
> > with to_region_ok_for_hw_watchpoint. You'll have to update the
> > callers, including the non-multi-arch ones, to ignore the first
> > argument; shouldn't be hard?
>
> Do you means to make the following change in gdb/target.h?
>
> --- gdb/target.h 4 Sep 2005 16:18:20 -0000 1.76
> +++ gdb/target.h 24 Jan 2006 04:17:13 -0000
> @@ -345,7 +345,7 @@
> int (*to_stopped_by_watchpoint) (void);
> int to_have_continuable_watchpoint;
> int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
> - int (*to_region_size_ok_for_hw_watchpoint) (int);
> + int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
> void (*to_terminal_init) (void);
> void (*to_terminal_inferior) (void);
> void (*to_terminal_ours_for_output) (void);
> @@ -1030,9 +1030,9 @@
> (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
> #endif
>
> -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
> - (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)
> +#if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, byte_count) \
> + (*current_target.to_region_ok_for_hw_watchpoint) (addr, byte_count)
> #endif
>
> Then make responsive changes to the code where is referenced?
>
> such as (in config/i386/nm-i386sol2.h):
>
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE) 1
>
> or (in s390-nat.c):
>
> static int
> -s390_region_size_ok_for_hw_watchpoint (int cnt)
> +s390_region_ok_for_hw_watchpoint (CORE_ADDR *, int cnt)
> {
> return 1;
> }
> @@ -380,7 +380,7 @@
>
> /* Add our watchpoint methods. */
> t->to_can_use_hw_breakpoint = s390_can_use_hw_breakpoint;
> - t->to_region_size_ok_for_hw_watchpoint = s390_region_size_ok_for_hw_watchpoint;
> + t->to_region_ok_for_hw_watchpoint = s390_region_ok_for_hw_watchpoint;
> t->to_have_continuable_watchpoint = 1;
> t->to_stopped_by_watchpoint = s390_stopped_by_watchpoint;
>
> and so on.
>
> P.S: If I can understand and also I understand correctly, it is not hard.
> Sometimes I just need a little more time to understand the precise
> meaning of your words. Most of the time I attribute it to my english.
>
> ;-)
>
> Regards
> - Wu Zhou
>
>
More information about the Gdb-patches
mailing list