* [Patch 2/2] MIPS hardware watchpoint support.
@ 2009-04-06 7:27 David Daney
2009-04-06 20:27 ` Eli Zaretskii
2009-04-11 17:16 ` Pedro Alves
0 siblings, 2 replies; 10+ messages in thread
From: David Daney @ 2009-04-06 7:27 UTC (permalink / raw)
To: gdb-patches; +Cc: David Daney
[-- Attachment #1: Type: text/plain, Size: 10109 bytes --]
Support for hardware watchpoints for mips-linux was merged into the
Linux kernel for version 2.6.28, with some bug fixes in 2.6.29. This
patch adds the corresponding gdb support. It has been tested on both
mipsel-linux (32-bit kernel) and mips64-linux (64-bit kernel).
For this submission I tested on a mipsel-linux system with these results
(with comments):
--- ./gdb/testsuite/gdb.sum 2009-04-05 20:53:20.000000000 -0700
+++ gdb.sum 2009-04-05 17:53:05.000000000 -0700
@@ -1,4 +1,4 @@
-Test Run By root on Sun Apr 5 18:08:05 2009
+Test Run By root on Sun Apr 5 14:45:02 2009
Native configuration is mipsel-unknown-linux-gnu
=== gdb tests ===
@@ -5717,15 +5717,15 @@
PASS: gdb.base/recurse.exp: continue to recurse (a = 5)
PASS: gdb.base/recurse.exp: next over b = 0 in second instance
PASS: gdb.base/recurse.exp: set second instance watchpoint
-PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
first time
-PASS: gdb.base/recurse.exp: continue to recurse (a = 4)
-PASS: gdb.base/recurse.exp: continue to recurse (a = 3)
-PASS: gdb.base/recurse.exp: continue to recurse (a = 2)
-PASS: gdb.base/recurse.exp: continue to recurse (a = 1)
-PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
second time
-PASS: gdb.base/recurse.exp: second instance watchpoint deleted when
leaving scope
-PASS: gdb.base/recurse.exp: continue to first instance watchpoint,
second time
-PASS: gdb.base/recurse.exp: first instance watchpoint deleted when
leaving scope
+FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
first time
+FAIL: gdb.base/recurse.exp: continue to recurse (a = 4)
+FAIL: gdb.base/recurse.exp: continue to recurse (a = 3)
+FAIL: gdb.base/recurse.exp: continue to recurse (a = 2)
+FAIL: gdb.base/recurse.exp: continue to recurse (a = 1)
+FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
second time
+FAIL: gdb.base/recurse.exp: second instance watchpoint deleted when
leaving scope
+FAIL: gdb.base/recurse.exp: continue to first instance watchpoint,
second time
+FAIL: gdb.base/recurse.exp: first instance watchpoint deleted when
leaving scope
These failures are caused by the fact that the test assumes more than a
the single watch register supported by the test system. Although this
patch supports up to eight registers, the test platform only has one.
We return true from mips_linux_can_use_hw_breakpoint for multiple
requests as it is not possible to know how many watchpoints are
enabled. Later gdb tries to insert more than one watchpoint and all but
the first fail.
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.base/regs.exp ...
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.base/relational.exp ...
PASS: gdb.base/relational.exp: set variable x=14
@@ -8312,26 +8312,26 @@
PASS: gdb.base/watch_thread_num.exp: Disable breakpoint 2
PASS: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
PASS: gdb.base/watch_thread_num.exp: info breakpoint 3
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 1 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 2 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 3 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 4 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 5 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 6 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 7 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 8 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 9 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 10 (timeout)
+PASS: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 1
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 3
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 4
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 6
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 8
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 10
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.base/watchpoint-hw.exp ...
Running
/home/daney/gdbcvs/src/gdb/testsuite/gdb.base/watchpoint-solib.exp ...
PASS: gdb.base/watchpoint-solib.exp: set pending breakpoint
@@ -8516,7 +8516,7 @@
PASS: gdb.cp/annota2.exp: breakpoint at main
PASS: gdb.cp/annota2.exp: run until main breakpoint
PASS: gdb.cp/annota2.exp: set watch on a.x
-PASS: gdb.cp/annota2.exp: watch triggered on a.x
+FAIL: gdb.cp/annota2.exp: watch triggered on a.x
PASS: gdb.cp/annota2.exp: annotate-quit
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.cp/annota3.exp ...
PASS: gdb.cp/annota3.exp: breakpoint main
@@ -8528,7 +8528,7 @@
PASS: gdb.cp/annota3.exp: break at main
PASS: gdb.cp/annota3.exp: second run until main breakpoint
PASS: gdb.cp/annota3.exp: set watch on a.x
-PASS: gdb.cp/annota3.exp: watch triggered on a.x
+FAIL: gdb.cp/annota3.exp: watch triggered on a.x
These two regressions are due to an extra:
frame-address
0x00400860
frame-address-end
Sequence emitted by gdb with 'set annotate 2'. The watchpoints are
triggering but the output is slightly different than the test expects.
I chalk it up to a defect in the test.
PASS: gdb.cp/annota3.exp: annotate-quit
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.cp/anon-union.exp ...
PASS: gdb.cp/anon-union.exp: next 1
@@ -12005,7 +12005,7 @@
PASS: gdb.mi/mi-watch.exp: hw: mi runto callee4
PASS: gdb.mi/mi-watch.exp: hw: break-watch operation
PASS: gdb.mi/mi-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
running)
Reported instruction location of the watchpoint trigger is one
instruction later and one line later.
FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
running)
Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.mi/mi2-basics.exp ...
PASS: gdb.mi/mi2-basics.exp: acceptance of MI operations
@@ -12638,7 +12638,7 @@
PASS: gdb.mi/mi2-watch.exp: hw: mi runto callee4
PASS: gdb.mi/mi2-watch.exp: hw: break-watch operation
PASS: gdb.mi/mi2-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (unknown output
after running)
Reported instruction location of the watchpoint trigger is one
instruction later and one line later.
FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (unknown output
after running)
Running
/home/daney/gdbcvs/src/gdb/testsuite/gdb.modula2/unbounded-array.exp ...
PASS: gdb.modula2/unbounded-array.exp: switch to modula-2
@@ -13297,10 +13297,10 @@
UNSUPPORTED: gdb.threads/tls.exp: Couldn't compile
/home/daney/gdbcvs/src/gdb/testsuite/gdb.threads/tls.c
/home/daney/gdbcvs/src/
gdb/testsuite/gdb.threads/tls2.c: unrecognized error
Running
/home/daney/gdbcvs/src/gdb/testsuite/gdb.threads/watchthreads.exp ...
PASS: gdb.threads/watchthreads.exp: successfully compiled posix threads
test case
-FAIL: gdb.threads/watchthreads.exp: watch args[0]
-FAIL: gdb.threads/watchthreads.exp: watch args[1]
+PASS: gdb.threads/watchthreads.exp: watch args[0]
+PASS: gdb.threads/watchthreads.exp: watch args[1]
FAIL: gdb.threads/watchthreads.exp: threaded watch loop
-FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
+PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread
FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread
@@ -13455,8 +13455,8 @@
=== gdb Summary ===
-# of expected passes 12556
-# of unexpected failures 151
+# of expected passes 12550
+# of unexpected failures 157
# of expected failures 40
# of known failures 52
# of unresolved testcases 18
OK to commit?
[-- Attachment #2: mips-linux-nat.patch --]
[-- Type: text/plain, Size: 20933 bytes --]
2009-04-05 David Daney <ddaney@caviumnetworks.com>
* mips-linux-nat.c (command.h, gdbcmd.h, gdb_assert.h): New #includes.
(maint_show_dr, super_close): New variables.
(super_fetch_registers, super_store_registers): Make static.
(PTRACE_GET_WATCH_REGS, PTRACE_SET_WATCH_REGS, W_BIT, R_BIT, I_BIT,
W_MASK, R_MASK, I_MASK, IRW_MASK, MAX_DEBUG_REGISTER): Define.
(pt_watch_style): Define new enum.
(mips32_watch_regs, mips64_watch_regs, pt_watch_regs,
mips_watchpoint): Define new structs.
(watch_readback_valid, watch_readback, current_watches,
watch_mirror): New variables.
(get_irw_mask, get_reg_mask, get_num_valid, get_watchlo,
set_watchlo, get_watchhi, set_watchhi, mips_show_dr,
mips_linux_read_watch_registers, mips_linux_can_use_hw_breakpoint,
mips_linux_stopped_by_watchpoint, mips_linux_stopped_data_address,
type_to_irw, fill_mask, try_one_watch,
mips_linux_region_ok_for_hw_watchpoint, write_watchpoint_regs,
mips_linux_new_thread, populate_regs_from_watches,
mips_linux_insert_watchpoint, mips_linux_remove_watchpoint,
mips_linux_close): New functions.
(_initialize_mips_linux_nat): Register watchpoint functions with
the target_ops. Add show-debug-regs maintenance command.
Index: mips-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-nat.c,v
retrieving revision 1.32
diff -u -p -r1.32 mips-linux-nat.c
--- mips-linux-nat.c 26 Feb 2009 19:44:39 -0000 1.32
+++ mips-linux-nat.c 6 Apr 2009 05:48:08 -0000
@@ -19,6 +19,9 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "command.h"
+#include "gdbcmd.h"
+#include "gdb_assert.h"
#include "inferior.h"
#include "mips-tdep.h"
#include "target.h"
@@ -44,11 +47,18 @@
we'll clear this and use PTRACE_PEEKUSER instead. */
static int have_ptrace_regsets = 1;
+/* Whether or not to print the mirrored debug registers. */
+static int maint_show_dr;
+
/* Saved function pointers to fetch and store a single register using
PTRACE_PEEKUSER and PTRACE_POKEUSER. */
-void (*super_fetch_registers) (struct target_ops *, struct regcache *, int);
-void (*super_store_registers) (struct target_ops *, struct regcache *, int);
+static void (*super_fetch_registers) (struct target_ops *,
+ struct regcache *, int);
+static void (*super_store_registers) (struct target_ops *,
+ struct regcache *, int);
+
+static void (*super_close) (int);
/* Map gdb internal register number to ptrace ``address''.
These ``addresses'' are normally defined in <asm/ptrace.h>.
@@ -357,12 +367,678 @@ mips_linux_read_description (struct targ
return tdesc_mips64_linux;
}
+#ifndef PTRACE_GET_WATCH_REGS
+# define PTRACE_GET_WATCH_REGS 0xd0
+#endif
+
+#ifndef PTRACE_SET_WATCH_REGS
+# define PTRACE_SET_WATCH_REGS 0xd1
+#endif
+
+#define W_BIT 0
+#define R_BIT 1
+#define I_BIT 2
+
+#define W_MASK (1 << W_BIT)
+#define R_MASK (1 << R_BIT)
+#define I_MASK (1 << I_BIT)
+
+#define IRW_MASK (I_MASK | R_MASK | W_MASK)
+
+enum pt_watch_style {
+ pt_watch_style_mips32,
+ pt_watch_style_mips64
+};
+
+#define MAX_DEBUG_REGISTER 8
+/* A value of zero in a watchlo indicates that it is available. */
+struct mips32_watch_regs
+{
+ uint32_t watchlo[MAX_DEBUG_REGISTER];
+ /* Lower 16 bits of watchhi. */
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ /* Valid mask and I R W bits.
+ * bit 0 -- 1 if W bit is usable.
+ * bit 1 -- 1 if R bit is usable.
+ * bit 2 -- 1 if I bit is usable.
+ * bits 3 - 11 -- Valid watchhi mask bits.
+ */
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ /* The number of valid watch register pairs. */
+ uint32_t num_valid;
+ /* There is confusion across gcc versions about structure alignment,
+ so we force 8 byte alignment for these structures so they match
+ the kernel even if it was build with a different gcc version. */
+} __attribute__ ((aligned (8)));
+
+struct mips64_watch_regs
+{
+ uint64_t watchlo[MAX_DEBUG_REGISTER];
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ uint32_t num_valid;
+} __attribute__ ((aligned (8)));
+
+struct pt_watch_regs
+{
+ enum pt_watch_style style;
+ union
+ {
+ struct mips32_watch_regs mips32;
+ struct mips64_watch_regs mips64;
+ };
+};
+
+/* -1 if the kernel and/or CPU do not support watch registers.
+ 1 if watch_readback is valid and we can read style, num_valid
+ and the masks.
+ 0 if we need to read the watch_readback. */
+static int watch_readback_valid;
+
+/* Cached watch register read values. */
+static struct pt_watch_regs watch_readback;
+
+/* We keep list of all watchpoints we should install and calculate the
+ watch register values each time the list changes. This allows for
+ easy sharing of watch registers for more than one watchpoint. */
+struct mips_watchpoint
+{
+ CORE_ADDR addr;
+ int len;
+ int type;
+ struct mips_watchpoint *next;
+};
+
+static struct mips_watchpoint *current_watches;
+
+/* The current set of watch register values for writing the
+ registers. */
+static struct pt_watch_regs watch_mirror;
+
+/* Assuming usable watch registers, return the irw_mask. */
+static uint32_t
+get_irw_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & IRW_MASK;
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, return the reg_mask. */
+static uint32_t
+get_reg_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & ~IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & ~IRW_MASK;
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, return the num_valid. */
+static uint32_t
+get_num_valid (struct pt_watch_regs *regs)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.num_valid;
+ case pt_watch_style_mips64:
+ return regs->mips64.num_valid;
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, return the watchlo. */
+static CORE_ADDR
+get_watchlo (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchlo[set];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchlo[set];
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, set a watchlo value. */
+static void
+set_watchlo (struct pt_watch_regs *regs, int set, CORE_ADDR value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ /* The cast will never throw away bits as 64 bit addresses can
+ never be used on a 32 bit kernel. */
+ regs->mips32.watchlo[set] = (uint32_t)value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchlo[set] = value;
+ break;
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, return the watchhi. */
+static uint32_t
+get_watchhi (struct pt_watch_regs *regs, int n)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchhi[n];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchhi[n];
+ default:
+ gdb_assert (0);
+ }
+}
+
+/* Assuming usable watch registers, set a watchhi value. */
+static void
+set_watchhi (struct pt_watch_regs *regs, int n, uint16_t value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ regs->mips32.watchhi[n] = value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchhi[n] = value;
+ break;
+ default:
+ gdb_assert (0);
+ }
+}
+
+static void
+mips_show_dr (const char *func, CORE_ADDR addr,
+ int len, enum target_hw_bp_type type)
+{
+ int i;
+
+ puts_unfiltered (func);
+ if (addr || len)
+ printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
+ /* This code is for mips, so casting CORE_ADDR
+ to unsigned long should be okay. */
+ (unsigned long)addr, len,
+ type == hw_write ? "data-write"
+ : (type == hw_read ? "data-read"
+ : (type == hw_access ? "data-read/write"
+ : (type == hw_execute ? "instruction-execute"
+ : "??unknown??"))));
+ puts_unfiltered (":\n");
+
+ for (i = 0; i < MAX_DEBUG_REGISTER; i++)
+ {
+ printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
+ i, paddr (get_watchlo (&watch_mirror, i)),
+ paddr (get_watchhi (&watch_mirror, i)));
+ }
+}
+
+/* Return 1 if watch registers are usable. Cached information is used
+ unless force is true. */
+static int
+mips_linux_read_watch_registers (int force)
+{
+ int tid;
+
+ if (force || watch_readback_valid == 0)
+ {
+ tid = ptid_get_lwp (inferior_ptid);
+ if (ptrace (PTRACE_GET_WATCH_REGS, tid, &watch_readback) == -1)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ switch (watch_readback.style)
+ {
+ case pt_watch_style_mips32:
+ if (watch_readback.mips32.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ case pt_watch_style_mips64:
+ if (watch_readback.mips64.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ default:
+ watch_readback_valid = -1;
+ return 0;
+ }
+ /* Watch registers appear to be usable. */
+ watch_readback_valid = 1;
+ }
+ return (watch_readback_valid == 1) ? 1 : 0;
+}
+
+/* Target to_can_use_hw_breakpoint implementation. Return 1 if we can
+ handle the specified watch type.
+
+ At this point we don't know if we can cover the watch area with the
+ available watch registers. However if it looks plausible, assume
+ we can, and we will fail later if we were overly optimistic. */
+static int
+mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
+{
+ int i;
+ uint32_t irw_mask;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ /* Use the union of all register capabilities. */
+ irw_mask = 0;
+ for (i = 0; i <get_num_valid (&watch_readback); i++)
+ irw_mask |= get_irw_mask (&watch_readback, i);
+
+ switch (type)
+ {
+ case bp_hardware_watchpoint:
+ if ((irw_mask & W_MASK) != W_MASK)
+ return 0;
+ break;
+ case bp_read_watchpoint:
+ if ((irw_mask & R_MASK) != R_MASK)
+ return 0;
+ break;
+ case bp_access_watchpoint:
+ if ((irw_mask & (R_MASK | W_MASK)) != (R_MASK | W_MASK))
+ return 0;
+ break;
+ default:
+ return 0;
+ }
+ return 1;
+}
+
+/* Target to_stopped_by_watchpoint implementation. Return 1 if
+ stopped by watchpoint. The watchhi R and W bits indicate the watch
+ register triggered. */
+static int
+mips_linux_stopped_by_watchpoint (void)
+{
+ int n;
+ int num_valid;
+
+ if (!mips_linux_read_watch_registers (1))
+ {
+ return 0;
+ }
+ num_valid = get_num_valid (&watch_readback);
+ for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
+ {
+ if (get_watchhi (&watch_readback, n) & (R_MASK | W_MASK))
+ return 1;
+ }
+ return 0;
+}
+
+/* Target to_stopped_data_address implementation. Set the address
+ where the watch triggered (if known). Return 1 if the address was
+ known. */
+static int
+mips_linux_stopped_data_address (struct target_ops *t, CORE_ADDR *paddr)
+{
+ /* On mips we don't know the low order 3 bits of the data address,
+ so we must return false. */
+ return 0;
+}
+
+/* Convert GDB's type to an IRW mask. */
+static unsigned
+type_to_irw (int type)
+{
+ switch (type)
+ {
+ case hw_write:
+ return W_MASK;
+ case hw_read:
+ return R_MASK;
+ case hw_access:
+ return (W_MASK | R_MASK);
+ default:
+ return 0;
+ }
+}
+
+/* Set any low order bits in mask that are not set. */
+static CORE_ADDR
+fill_mask (CORE_ADDR mask)
+{
+ CORE_ADDR f = 1;
+ while (f && f < mask)
+ {
+ mask |= f;
+ f <<= 1;
+ }
+ return mask;
+}
+
+/* Try to add a single watch to the specified registers. Return 1 on
+ success, 0 on failure. */
+static int
+try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
+ int len, unsigned irw)
+{
+ CORE_ADDR base_addr, last_byte, break_addr, segment_len;
+ CORE_ADDR mask_bits, t_low, t_low_end;
+ uint16_t t_hi;
+ int i, free_watches;
+ struct pt_watch_regs regs_copy;
+
+ if (len <= 0)
+ return 0;
+
+ last_byte = addr + len - 1;
+ mask_bits = fill_mask (addr ^ last_byte) | IRW_MASK;
+ base_addr = addr & ~mask_bits;
+
+ /* Check to see if it is covered by current registers. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low != 0 && irw == ((unsigned)t_low & irw))
+ {
+ t_hi = get_watchhi (regs, i) | IRW_MASK;
+ t_low &= ~(CORE_ADDR)t_hi;
+ if (addr >= t_low && last_byte <= (t_low + t_hi))
+ return 1;
+ }
+ }
+ /* Try to find an empty register. */
+ free_watches = 0;
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low == 0 && irw == (get_irw_mask (regs, i) & irw))
+ {
+ if (mask_bits <= (get_reg_mask (regs, i) | IRW_MASK))
+ {
+ /* It fits, we'll take it. */
+ set_watchlo (regs, i, base_addr | irw);
+ set_watchhi (regs, i, mask_bits & ~IRW_MASK);
+ return 1;
+ }
+ else
+ {
+ /* It doesn't fit, but has the proper IRW capabilities. */
+ free_watches++;
+ }
+ }
+ }
+ if (free_watches > 1)
+ {
+ /* Try to split it across several registers. */
+ regs_copy = *regs;
+ for (i = 0; i < get_num_valid (®s_copy); i++)
+ {
+ t_low = get_watchlo (®s_copy, i);
+ t_hi = get_reg_mask (®s_copy, i) | IRW_MASK;
+ if (t_low == 0 && irw == (t_hi & irw))
+ {
+ t_low = addr & ~(CORE_ADDR)t_hi;
+ break_addr = t_low + t_hi + 1;
+ if (break_addr >= addr + len)
+ segment_len = len;
+ else
+ segment_len = break_addr - addr;
+ mask_bits = fill_mask (addr ^ (addr + segment_len - 1));
+ set_watchlo (®s_copy, i, (addr & ~mask_bits) | irw);
+ set_watchhi (®s_copy, i, mask_bits & ~IRW_MASK);
+ if (break_addr >= addr + len)
+ {
+ *regs = regs_copy;
+ return 1;
+ }
+ len = addr + len - break_addr;
+ addr = break_addr;
+ }
+ }
+ }
+ /* It didn't fit anywhere, we failed. */
+ return 0;
+}
+
+/* Target to_region_ok_for_hw_watchpoint implementation. Return 1 if
+ the specified region can be covered by the watch registers. */
+static int
+mips_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ struct pt_watch_regs dummy_regs;
+ int i;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ dummy_regs = watch_readback;
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (&dummy_regs); i++)
+ set_watchlo (&dummy_regs, i, 0);
+ return try_one_watch(&dummy_regs, addr, len, 0);
+}
+
+
+/* Write the mirrored watch register values for each thread. */
+static int
+write_watchpoint_regs (void)
+{
+ struct lwp_info *lp;
+ ptid_t ptid;
+ int tid;
+
+ ALL_LWPS (lp, ptid)
+ {
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ {
+ perror_with_name (_("Couldn't write debug register"));
+ return -1;
+ }
+ }
+ return 0;
+}
+
+/* linux_nat new_thread implementation. Write the mirrored watch
+ register values for the new thread. */
+static void
+mips_linux_new_thread (ptid_t ptid)
+{
+ int tid;
+
+ if (!mips_linux_read_watch_registers (0))
+ return;
+
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ {
+ perror_with_name (_("Couldn't write debug register"));
+ }
+}
+
+/* Fill in the watch registers with the currently cached watches. */
+static void
+populate_regs_from_watches (struct pt_watch_regs *regs)
+{
+ struct mips_watchpoint *w;
+ int i;
+
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ set_watchlo (regs, i, 0);
+ set_watchhi (regs, i, 0);
+ }
+
+ w = current_watches;
+ while (w)
+ {
+ i = try_one_watch (regs, w->addr, w->len, type_to_irw (w->type));
+ /* They must all fit, because we previously calculated that they
+ would. */
+ gdb_assert (i);
+ w = w->next;
+ }
+}
+
+/* Target to_insert_watchpoint implementation. Try to insert a new
+ watch. Return zero on success. */
+static int
+mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ struct pt_watch_regs regs;
+ struct mips_watchpoint *new_watch;
+ struct mips_watchpoint **pw;
+
+ int i;
+ int retval;
+
+ if (!mips_linux_read_watch_registers (0))
+ return -1;
+
+ if (len <= 0)
+ return -1;
+
+ regs = watch_readback;
+ /* Add the current watches. */
+ populate_regs_from_watches (®s);
+
+ /* Now try to add the new watch. */
+ if (!try_one_watch (®s, addr, len, type_to_irw (type)))
+ return -1;
+
+ /* It fit. Stick it on the end of the list. */
+ new_watch = (struct mips_watchpoint *)
+ xmalloc (sizeof (struct mips_watchpoint));
+ new_watch->addr = addr;
+ new_watch->len = len;
+ new_watch->type = type;
+ new_watch->next = NULL;
+
+ pw = ¤t_watches;
+ while (*pw != NULL)
+ pw = &(*pw)->next;
+ *pw = new_watch;
+
+ watch_mirror = regs;
+ retval = write_watchpoint_regs();
+
+ if (maint_show_dr)
+ mips_show_dr ("insert_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_remove_watchpoint implementation. Try to remove a watch.
+ Return zero on success. */
+static int
+mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ int retval;
+ int deleted_one;
+
+ struct mips_watchpoint **pw;
+ struct mips_watchpoint *w;
+
+ /* Search for a known watch that matches. Then unlink and free
+ it. */
+ deleted_one = 0;
+ pw = ¤t_watches;
+ while ((w = *pw))
+ {
+ if (w->addr == addr && w->len == len && w->type == type)
+ {
+ *pw = w->next;
+ xfree (w);
+ deleted_one = 1;
+ break;
+ }
+ pw = &(w->next);
+ }
+
+ if (!deleted_one)
+ return -1; /* We don't know about it, fail doing nothing. */
+
+ /* At this point watch_readback is known to be valid because we
+ could not have added the watch without reading it. */
+ gdb_assert (watch_readback_valid == 1);
+
+ watch_mirror = watch_readback;
+ populate_regs_from_watches (&watch_mirror);
+
+ retval = write_watchpoint_regs();
+
+ if (maint_show_dr)
+ mips_show_dr ("remove_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_close implementation. Free any watches and call the
+ super implementation. */
+static void
+mips_linux_close (int quitting)
+{
+ struct mips_watchpoint *w;
+ struct mips_watchpoint *nw;
+
+ /* Clean out the current_watches list. */
+ w = current_watches;
+ while (w)
+ {
+ nw = w->next;
+ xfree (w);
+ w = nw;
+ }
+ current_watches = NULL;
+
+ if (super_close)
+ super_close (quitting);
+}
+
void _initialize_mips_linux_nat (void);
void
_initialize_mips_linux_nat (void)
{
- struct target_ops *t = linux_trad_target (mips_linux_register_u_offset);
+ struct target_ops *t;
+
+ deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
+ var_boolean, (char *) &maint_show_dr, _("\
+Set whether to show variables that mirror the mips debug registers.\n\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the debug registers values are shown when GDB inserts\n\
+or removes a hardware breakpoint or watchpoint, and when the inferior\n\
+triggers a breakpoint or watchpoint."),
+ &maintenancelist);
+
+
+ t = linux_trad_target (mips_linux_register_u_offset);
+
+ super_close = t->to_close;
+ t->to_close = mips_linux_close;
super_fetch_registers = t->to_fetch_registers;
super_store_registers = t->to_store_registers;
@@ -370,9 +1046,17 @@ _initialize_mips_linux_nat (void)
t->to_fetch_registers = mips64_linux_fetch_registers;
t->to_store_registers = mips64_linux_store_registers;
+ t->to_can_use_hw_breakpoint = mips_linux_can_use_hw_breakpoint;
+ t->to_remove_watchpoint = mips_linux_remove_watchpoint;
+ t->to_insert_watchpoint = mips_linux_insert_watchpoint;
+ t->to_stopped_by_watchpoint = mips_linux_stopped_by_watchpoint;
+ t->to_stopped_data_address = mips_linux_stopped_data_address;
+ t->to_region_ok_for_hw_watchpoint = mips_linux_region_ok_for_hw_watchpoint;
+
t->to_read_description = mips_linux_read_description;
linux_nat_add_target (t);
+ linux_nat_set_new_thread (t, mips_linux_new_thread);
/* Initialize the standard target descriptions. */
initialize_tdesc_mips_linux ();
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 2/2] MIPS hardware watchpoint support.
2009-04-06 7:27 [Patch 2/2] MIPS hardware watchpoint support David Daney
@ 2009-04-06 20:27 ` Eli Zaretskii
2009-04-11 17:16 ` Pedro Alves
1 sibling, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2009-04-06 20:27 UTC (permalink / raw)
To: David Daney; +Cc: gdb-patches, ddaney
> Date: Mon, 06 Apr 2009 00:27:07 -0700
> From: David Daney <david.s.daney@gmail.com>
> CC: David Daney <ddaney@caviumnetworks.com>
>
> Support for hardware watchpoints for mips-linux was merged into the
> Linux kernel for version 2.6.28, with some bug fixes in 2.6.29. This
> patch adds the corresponding gdb support. It has been tested on both
> mipsel-linux (32-bit kernel) and mips64-linux (64-bit kernel).
Thanks. If this is approved, please also add a NEWS entry for this.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 2/2] MIPS hardware watchpoint support.
2009-04-06 7:27 [Patch 2/2] MIPS hardware watchpoint support David Daney
2009-04-06 20:27 ` Eli Zaretskii
@ 2009-04-11 17:16 ` Pedro Alves
2009-04-16 17:08 ` David Daney
1 sibling, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2009-04-11 17:16 UTC (permalink / raw)
To: gdb-patches; +Cc: David Daney, David Daney
On Monday 06 April 2009 08:27:07, David Daney wrote:
> Support for hardware watchpoints for mips-linux was merged into the
> Linux kernel for version 2.6.28, with some bug fixes in 2.6.29. This
> patch adds the corresponding gdb support. It has been tested on both
> mipsel-linux (32-bit kernel) and mips64-linux (64-bit kernel).
Thanks! I'll pretend to know a thing about mips from here on, and
hope that it doesn't show too much that I don't.
In general, this looks very good to me, although I didn't
make an effort to understand all the bit juggling going on. Minor
comments to the code below. Then comments to the tests follow that.
> 2009-04-05 David Daney <ddaney@caviumnetworks.com>
> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo,
> set_watchlo, get_watchhi, set_watchhi, mips_show_dr,
Please split these context lines like so instead, as per the
coding conventions:
> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo)
> (set_watchlo, get_watchhi, set_watchhi, mips_show_dr)
> +/* Assuming usable watch registers, return the irw_mask. */
> +static uint32_t
Please add an empty line between comment and function. Here
and elsewhere.
> +get_irw_mask (struct pt_watch_regs *regs, int set)
> +{
> + switch (regs->style)
> + {
> + case pt_watch_style_mips32:
> + return regs->mips32.watch_masks[set] & IRW_MASK;
> + case pt_watch_style_mips64:
> + return regs->mips64.watch_masks[set] & IRW_MASK;
> + default:
> + gdb_assert (0);
Please don't write 'gdb_assert (0)', instead make that a
more bit more friendly `internal_error'. Here and
elsewhere.
> +static void
> +mips_show_dr (const char *func, CORE_ADDR addr,
> + int len, enum target_hw_bp_type type)
> +{
> + int i;
> +
> + puts_unfiltered (func);
> + if (addr || len)
> + printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
> + /* This code is for mips, so casting CORE_ADDR
> + to unsigned long should be okay. */
> + (unsigned long)addr, len,
Why bother to explain that instead of using `paddr'? If there's
a reason, then it would be nice if it was spelled out in the
comment.
> + type == hw_write ? "data-write"
> + : (type == hw_read ? "data-read"
> + : (type == hw_access ? "data-read/write"
> + : (type == hw_execute ? "instruction-execute"
> + : "??unknown??"))));
> + puts_unfiltered (":\n");
> +
> + for (i = 0; i < MAX_DEBUG_REGISTER; i++)
> + {
Unneeded braces. You have a couple more in the patch.
> + printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
> + i, paddr (get_watchlo (&watch_mirror, i)),
> + paddr (get_watchhi (&watch_mirror, i)));
> + }
> +}
> +
> +/* Write the mirrored watch register values for each thread. */
> +static int
> +write_watchpoint_regs (void)
> +{
> + struct lwp_info *lp;
> + ptid_t ptid;
> + int tid;
> +
> + ALL_LWPS (lp, ptid)
> + {
> + tid = ptid_get_lwp (ptid);
> + if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
> + {
> + perror_with_name (_("Couldn't write debug register"));
> + return -1;
perror doesn't return, no need for `return -1'.
> + }
> + }
> + return 0;
> +}
> +
> +
> +/* Target to_insert_watchpoint implementation. Try to insert a new
> + watch. Return zero on success. */
> +static int
> +mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type)
> +{
> +
> + watch_mirror = regs;
> + retval = write_watchpoint_regs();
^ missing space.
> +/* Target to_remove_watchpoint implementation. Try to remove a watch.
> + Return zero on success. */
> +static int
> +mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type)
> +{
> +
> + watch_mirror = watch_readback;
> + populate_regs_from_watches (&watch_mirror);
> +
> + retval = write_watchpoint_regs();
^ missing space.
>
> void
> _initialize_mips_linux_nat (void)
> {
> - struct target_ops *t = linux_trad_target (mips_linux_register_u_offset);
> + struct target_ops *t;
> +
> + deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
> + var_boolean, (char *) &maint_show_dr, _("\
> +Set whether to show variables that mirror the mips debug registers.\n\
> +Use \"on\" to enable, \"off\" to disable.\n\
> +If enabled, the debug registers values are shown when GDB inserts\n\
> +or removes a hardware breakpoint or watchpoint, and when the inferior\n\
> +triggers a breakpoint or watchpoint."),
> + &maintenancelist);
Argh. I've posted a patch that removes deprecated_add_set_cmd, but
never got around to committing it.
http://sourceware.org/ml/gdb-patches/2009-01/msg00119.html
When I get around to finish off that patch, I'll adjust this
file too --- I assume that is OK with you.
However, reusing the x86 command does avoid needing to document
it, but, could you remove the reference to x86 in the current
docs?
@kindex maint show-debug-regs
@cindex x86 hardware debug registers
@item maint show-debug-regs
Control whether to show variables that mirror the x86 hardware debug
registers. Use @code{ON} to enable, @code{OFF} to disable. If
enabled, the debug registers values are shown when @value{GDBN} inserts or
removes a hardware breakpoint or watchpoint, and when the inferior
triggers a hardware-assisted breakpoint or watchpoint.
... since it now applies to mips as well.
> For this submission I tested on a mipsel-linux system with these results
> (with comments):
>
> --- ./gdb/testsuite/gdb.sum 2009-04-05 20:53:20.000000000 -0700
> +++ gdb.sum 2009-04-05 17:53:05.000000000 -0700
> @@ -1,4 +1,4 @@
> -Test Run By root on Sun Apr 5 18:08:05 2009
> +Test Run By root on Sun Apr 5 14:45:02 2009
> Native configuration is mipsel-unknown-linux-gnu
>
> === gdb tests ===
> @@ -5717,15 +5717,15 @@
> PASS: gdb.base/recurse.exp: continue to recurse (a = 5)
> PASS: gdb.base/recurse.exp: next over b = 0 in second instance
> PASS: gdb.base/recurse.exp: set second instance watchpoint
> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
> first time
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 4)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 3)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 2)
> -PASS: gdb.base/recurse.exp: continue to recurse (a = 1)
> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
> second time
> -PASS: gdb.base/recurse.exp: second instance watchpoint deleted when
> leaving scope
> -PASS: gdb.base/recurse.exp: continue to first instance watchpoint,
> second time
> -PASS: gdb.base/recurse.exp: first instance watchpoint deleted when
> leaving scope
> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
> first time
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 4)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 3)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 2)
> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 1)
> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
> second time
> +FAIL: gdb.base/recurse.exp: second instance watchpoint deleted when
> leaving scope
> +FAIL: gdb.base/recurse.exp: continue to first instance watchpoint,
> second time
> +FAIL: gdb.base/recurse.exp: first instance watchpoint deleted when
> leaving scope
>
> These failures are caused by the fact that the test assumes more than a
> the single watch register supported by the test system. Although this
> patch supports up to eight registers, the test platform only has one.
> We return true from mips_linux_can_use_hw_breakpoint for multiple
> requests as it is not possible to know how many watchpoints are
> enabled. Later gdb tries to insert more than one watchpoint and all but
> the first fail.
Could we detect that, and fail the rest of the test gracefully?
What error message do you get?
> /home/daney/gdbcvs/src/gdb/testsuite/gdb.base/watchpoint-solib.exp ...
> PASS: gdb.base/watchpoint-solib.exp: set pending breakpoint
> @@ -8516,7 +8516,7 @@
> PASS: gdb.cp/annota2.exp: breakpoint at main
> PASS: gdb.cp/annota2.exp: run until main breakpoint
> PASS: gdb.cp/annota2.exp: set watch on a.x
> -PASS: gdb.cp/annota2.exp: watch triggered on a.x
> +FAIL: gdb.cp/annota2.exp: watch triggered on a.x
> PASS: gdb.cp/annota2.exp: annotate-quit
> Running /home/daney/gdbcvs/src/gdb/testsuite/gdb.cp/annota3.exp ...
> PASS: gdb.cp/annota3.exp: breakpoint main
> @@ -8528,7 +8528,7 @@
> PASS: gdb.cp/annota3.exp: break at main
> PASS: gdb.cp/annota3.exp: second run until main breakpoint
> PASS: gdb.cp/annota3.exp: set watch on a.x
> -PASS: gdb.cp/annota3.exp: watch triggered on a.x
> +FAIL: gdb.cp/annota3.exp: watch triggered on a.x
>
> These two regressions are due to an extra:
> frame-address
> 0x00400860
> frame-address-end
>
> Sequence emitted by gdb with 'set annotate 2'. The watchpoints are
> triggering but the output is slightly different than the test expects.
> I chalk it up to a defect in the test.
This annotation tests are a nuisance, particularly, since we want to
stop supporting annotations at some point. Ideally, we'd just adjust
the test ...
> -PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
> +FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
> running)
>
> Reported instruction location of the watchpoint trigger is one
> instruction later and one line later.
Why is that?
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 2/2] MIPS hardware watchpoint support.
2009-04-11 17:16 ` Pedro Alves
@ 2009-04-16 17:08 ` David Daney
2009-04-17 13:43 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: David Daney @ 2009-04-16 17:08 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 15190 bytes --]
Pedro Alves wrote:
> On Monday 06 April 2009 08:27:07, David Daney wrote:
>> Support for hardware watchpoints for mips-linux was merged into the
>> Linux kernel for version 2.6.28, with some bug fixes in 2.6.29. This
>> patch adds the corresponding gdb support. It has been tested on both
>> mipsel-linux (32-bit kernel) and mips64-linux (64-bit kernel).
>
> Thanks! I'll pretend to know a thing about mips from here on, and
> hope that it doesn't show too much that I don't.
>
> In general, this looks very good to me, although I didn't
> make an effort to understand all the bit juggling going on. Minor
> comments to the code below. Then comments to the tests follow that.
>
>> 2009-04-05 David Daney <ddaney@caviumnetworks.com>
>
>> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo,
>> set_watchlo, get_watchhi, set_watchhi, mips_show_dr,
>
> Please split these context lines like so instead, as per the
> coding conventions:
>
>> (get_irw_mask, get_reg_mask, get_num_valid, get_watchlo)
>> (set_watchlo, get_watchhi, set_watchhi, mips_show_dr)
>
>
Done. All these years and there is still more to learn about the coding
conventions.
>> +/* Assuming usable watch registers, return the irw_mask. */
>> +static uint32_t
>
> Please add an empty line between comment and function. Here
> and elsewhere.
Done.
>
>> +get_irw_mask (struct pt_watch_regs *regs, int set)
>> +{
>> + switch (regs->style)
>> + {
>> + case pt_watch_style_mips32:
>> + return regs->mips32.watch_masks[set] & IRW_MASK;
>> + case pt_watch_style_mips64:
>> + return regs->mips64.watch_masks[set] & IRW_MASK;
>> + default:
>> + gdb_assert (0);
>
> Please don't write 'gdb_assert (0)', instead make that a
> more bit more friendly `internal_error'. Here and
> elsewhere.
>
Done.
>> + printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
>> + /* This code is for mips, so casting CORE_ADDR
>> + to unsigned long should be okay. */
>> + (unsigned long)addr, len,
>
> Why bother to explain that instead of using `paddr'? If there's
> a reason, then it would be nice if it was spelled out in the
> comment.
Comment deleted, and types casted to (unsigned long long).
>> + for (i = 0; i < MAX_DEBUG_REGISTER; i++)
>> + {
>
> Unneeded braces. You have a couple more in the patch.
Fixed.
>
>> + printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
>> + i, paddr (get_watchlo (&watch_mirror, i)),
>> + paddr (get_watchhi (&watch_mirror, i)));
>> + }
>> +}
>> +
>> + {
>> + perror_with_name (_("Couldn't write debug register"));
>> + return -1;
>
> perror doesn't return, no need for `return -1'.
Fixed.
>> +
>> + watch_mirror = watch_readback;
>> + populate_regs_from_watches (&watch_mirror);
>> +
>> + retval = write_watchpoint_regs();
>
> ^ missing space.
All instances fixed.
>> + deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
>> + var_boolean, (char *) &maint_show_dr, _("\
>> +Set whether to show variables that mirror the mips debug registers.\n\
>> +Use \"on\" to enable, \"off\" to disable.\n\
>> +If enabled, the debug registers values are shown when GDB inserts\n\
>> +or removes a hardware breakpoint or watchpoint, and when the inferior\n\
>> +triggers a breakpoint or watchpoint."),
>> + &maintenancelist);
>
> Argh. I've posted a patch that removes deprecated_add_set_cmd, but
> never got around to committing it.
>
> http://sourceware.org/ml/gdb-patches/2009-01/msg00119.html
>
> When I get around to finish off that patch, I'll adjust this
> file too --- I assume that is OK with you.
>
> However, reusing the x86 command does avoid needing to document
> it, but, could you remove the reference to x86 in the current
> docs?
>
> @kindex maint show-debug-regs
> @cindex x86 hardware debug registers
> @item maint show-debug-regs
> Control whether to show variables that mirror the x86 hardware debug
> registers. Use @code{ON} to enable, @code{OFF} to disable. If
> enabled, the debug registers values are shown when @value{GDBN} inserts or
> removes a hardware breakpoint or watchpoint, and when the inferior
> triggers a hardware-assisted breakpoint or watchpoint.
>
> ... since it now applies to mips as well.
Done.
>> For this submission I tested on a mipsel-linux system with these results
>> (with comments):
>>
>> --- ./gdb/testsuite/gdb.sum 2009-04-05 20:53:20.000000000 -0700
>> +++ gdb.sum 2009-04-05 17:53:05.000000000 -0700
>> @@ -1,4 +1,4 @@
>> -Test Run By root on Sun Apr 5 18:08:05 2009
>> +Test Run By root on Sun Apr 5 14:45:02 2009
>> Native configuration is mipsel-unknown-linux-gnu
>>
>> === gdb tests ===
>> @@ -5717,15 +5717,15 @@
>> PASS: gdb.base/recurse.exp: continue to recurse (a = 5)
>> PASS: gdb.base/recurse.exp: next over b = 0 in second instance
>> PASS: gdb.base/recurse.exp: set second instance watchpoint
>> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
>> first time
>> -PASS: gdb.base/recurse.exp: continue to recurse (a = 4)
>> -PASS: gdb.base/recurse.exp: continue to recurse (a = 3)
>> -PASS: gdb.base/recurse.exp: continue to recurse (a = 2)
>> -PASS: gdb.base/recurse.exp: continue to recurse (a = 1)
>> -PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
>> second time
>> -PASS: gdb.base/recurse.exp: second instance watchpoint deleted when
>> leaving scope
>> -PASS: gdb.base/recurse.exp: continue to first instance watchpoint,
>> second time
>> -PASS: gdb.base/recurse.exp: first instance watchpoint deleted when
>> leaving scope
>> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
>> first time
>> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 4)
>> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 3)
>> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 2)
>> +FAIL: gdb.base/recurse.exp: continue to recurse (a = 1)
>> +FAIL: gdb.base/recurse.exp: continue to second instance watchpoint,
>> second time
>> +FAIL: gdb.base/recurse.exp: second instance watchpoint deleted when
>> leaving scope
>> +FAIL: gdb.base/recurse.exp: continue to first instance watchpoint,
>> second time
>> +FAIL: gdb.base/recurse.exp: first instance watchpoint deleted when
>> leaving scope
>>
>> These failures are caused by the fact that the test assumes more than a
>> the single watch register supported by the test system. Although this
>> patch supports up to eight registers, the test platform only has one.
>> We return true from mips_linux_can_use_hw_breakpoint for multiple
>> requests as it is not possible to know how many watchpoints are
>> enabled. Later gdb tries to insert more than one watchpoint and all but
>> the first fail.
>
> Could we detect that, and fail the rest of the test gracefully?
> What error message do you get?
>
I slightly modified the patch so that it makes more conservative
estimates about how many hardware watches it can handle. This fixes
these failures.
>
>> -PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
>> +FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
>> running)
>>
>> Reported instruction location of the watchpoint trigger is one
>> instruction later and one line later.
>
> Why is that?
I was mistaken, it is earlier, not later. With hardware watch points,
$pc points to the faulting instruction, with software watch points $pc
points to the following instruction. In a couple of tests, this results
in the reported line number being different than the expected value.
Here is the revised patch and its test results:
--- pre/gdb.sum 2009-04-15 12:12:33.000000000 -0700
+++ post/gdb.sum 2009-04-15 10:22:37.000000000 -0700
@@ -1,4 +1,4 @@
-Test Run By root on Wed Apr 15 10:42:35 2009
+Test Run By root on Tue Apr 14 16:28:52 2009
Native configuration is mips64-unknown-linux-gnu
=== gdb tests ===
@@ -5684,7 +5684,7 @@
PASS: gdb.base/recurse.exp: continue to recurse (a = 1)
PASS: gdb.base/recurse.exp: continue to second instance watchpoint,
second time
PASS: gdb.base/recurse.exp: second instance watchpoint deleted when
leaving scope
-PASS: gdb.base/recurse.exp: continue to first instance watchpoint,
second time
+FAIL: gdb.base/recurse.exp: continue to first instance watchpoint,
second time
Different line number as explained above.
PASS: gdb.base/recurse.exp: first instance watchpoint deleted when
leaving scope
Running ../../../src/gdb/testsuite/gdb.base/regs.exp ...
Running ../../../src/gdb/testsuite/gdb.base/relational.exp ...
@@ -8272,26 +8272,26 @@
PASS: gdb.base/watch_thread_num.exp: Disable breakpoint 2
PASS: gdb.base/watch_thread_num.exp: Watchpoint on shared variable
PASS: gdb.base/watch_thread_num.exp: info breakpoint 3
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 1 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 2 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 3 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 4 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 5 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 6 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 7 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 8 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 9 (timeout)
-FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10
(timeout)
-FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 10 (timeout)
+PASS: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 1
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 2
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 3
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 3
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 4
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 4
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 5
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 6
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 6
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 7
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 8
+PASS: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 8
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 9
+FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 10
+FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered
iteration 10
Running ../../../src/gdb/testsuite/gdb.base/watchpoint-hw.exp ...
Running ../../../src/gdb/testsuite/gdb.base/watchpoint-solib.exp ...
PASS: gdb.base/watchpoint-solib.exp: set pending breakpoint
@@ -8476,7 +8476,7 @@
PASS: gdb.cp/annota2.exp: breakpoint at main
PASS: gdb.cp/annota2.exp: run until main breakpoint
PASS: gdb.cp/annota2.exp: set watch on a.x
-PASS: gdb.cp/annota2.exp: watch triggered on a.x
+FAIL: gdb.cp/annota2.exp: watch triggered on a.x
PASS: gdb.cp/annota2.exp: annotate-quit
Running ../../../src/gdb/testsuite/gdb.cp/annota3.exp ...
PASS: gdb.cp/annota3.exp: breakpoint main
@@ -8488,7 +8488,7 @@
PASS: gdb.cp/annota3.exp: break at main
PASS: gdb.cp/annota3.exp: second run until main breakpoint
PASS: gdb.cp/annota3.exp: set watch on a.x
-PASS: gdb.cp/annota3.exp: watch triggered on a.x
+FAIL: gdb.cp/annota3.exp: watch triggered on a.x
The two annotation failures are explained in the
PASS: gdb.cp/annota3.exp: annotate-quit
Running ../../../src/gdb/testsuite/gdb.cp/anon-union.exp ...
PASS: gdb.cp/anon-union.exp: next 1
@@ -11976,7 +11976,7 @@
PASS: gdb.mi/mi-watch.exp: hw: mi runto callee4
PASS: gdb.mi/mi-watch.exp: hw: break-watch operation
PASS: gdb.mi/mi-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
running)
Different line number as explained above.
PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
Running ../../../src/gdb/testsuite/gdb.mi/mi2-basics.exp ...
PASS: gdb.mi/mi2-basics.exp: acceptance of MI operations
@@ -12609,7 +12609,7 @@
PASS: gdb.mi/mi2-watch.exp: hw: mi runto callee4
PASS: gdb.mi/mi2-watch.exp: hw: break-watch operation
PASS: gdb.mi/mi2-watch.exp: hw: list of watchpoints
-PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger
+FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (unknown output
after running)
Different line number as explained above.
PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger
Running ../../../src/gdb/testsuite/gdb.modula2/unbounded-array.exp ...
PASS: gdb.modula2/unbounded-array.exp: switch to modula-2
@@ -13351,10 +13351,10 @@
PASS: gdb.threads/tls.exp: info address a_thread_local
Running ../../../src/gdb/testsuite/gdb.threads/watchthreads.exp ...
PASS: gdb.threads/watchthreads.exp: successfully compiled posix
threads test case
-FAIL: gdb.threads/watchthreads.exp: watch args[0]
+PASS: gdb.threads/watchthreads.exp: watch args[0]
FAIL: gdb.threads/watchthreads.exp: watch args[1]
FAIL: gdb.threads/watchthreads.exp: threaded watch loop
-FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
+PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread
FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread
OK to commit?
[-- Attachment #2: mips-watch.patch --]
[-- Type: text/plain, Size: 22095 bytes --]
gdb/
2009-04-16 David Daney <ddaney@caviumnetworks.com>
* mips-linux-nat.c (command.h, gdbcmd.h, gdb_assert.h): New #includes.
(maint_show_dr, super_close): New variables.
(super_fetch_registers, super_store_registers): Make static.
(PTRACE_GET_WATCH_REGS, PTRACE_SET_WATCH_REGS, W_BIT, R_BIT, I_BIT)
(W_MASK, R_MASK, I_MASK, IRW_MASK, MAX_DEBUG_REGISTER): Define.
(pt_watch_style): Define new enum.
(mips32_watch_regs, mips64_watch_regs, pt_watch_regs, mips_watchpoint):
Define new structs.
(watch_readback_valid, watch_readback, current_watches, watch_mirror):
New variables.
(get_irw_mask, get_reg_mask, get_num_valid, get_watchlo)
(set_watchlo, get_watchhi, set_watchhi, mips_show_dr)
(mips_linux_read_watch_registers, mips_linux_can_use_hw_breakpoint)
(mips_linux_stopped_by_watchpoint, mips_linux_stopped_data_address)
(type_to_irw, fill_mask, try_one_watch)
(mips_linux_region_ok_for_hw_watchpoint, write_watchpoint_regs)
(mips_linux_new_thread, populate_regs_from_watches)
(mips_linux_insert_watchpoint, mips_linux_remove_watchpoint)
(mips_linux_close): New functions.
(_initialize_mips_linux_nat): Register watchpoint functions with
the target_ops. Add show-debug-regs maintenance command.
gdb/doc/
2009-04-16 David Daney <ddaney@caviumnetworks.com>
* gdb.texinfo (maint show-debug-regs): Remove mention of x86.
Index: mips-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-nat.c,v
retrieving revision 1.32
diff -u -p -r1.32 mips-linux-nat.c
--- mips-linux-nat.c 26 Feb 2009 19:44:39 -0000 1.32
+++ mips-linux-nat.c 16 Apr 2009 16:34:56 -0000
@@ -19,6 +19,9 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "command.h"
+#include "gdbcmd.h"
+#include "gdb_assert.h"
#include "inferior.h"
#include "mips-tdep.h"
#include "target.h"
@@ -44,11 +47,19 @@
we'll clear this and use PTRACE_PEEKUSER instead. */
static int have_ptrace_regsets = 1;
+/* Whether or not to print the mirrored debug registers. */
+
+static int maint_show_dr;
+
/* Saved function pointers to fetch and store a single register using
PTRACE_PEEKUSER and PTRACE_POKEUSER. */
-void (*super_fetch_registers) (struct target_ops *, struct regcache *, int);
-void (*super_store_registers) (struct target_ops *, struct regcache *, int);
+static void (*super_fetch_registers) (struct target_ops *,
+ struct regcache *, int);
+static void (*super_store_registers) (struct target_ops *,
+ struct regcache *, int);
+
+static void (*super_close) (int);
/* Map gdb internal register number to ptrace ``address''.
These ``addresses'' are normally defined in <asm/ptrace.h>.
@@ -357,12 +368,697 @@ mips_linux_read_description (struct targ
return tdesc_mips64_linux;
}
+#ifndef PTRACE_GET_WATCH_REGS
+# define PTRACE_GET_WATCH_REGS 0xd0
+#endif
+
+#ifndef PTRACE_SET_WATCH_REGS
+# define PTRACE_SET_WATCH_REGS 0xd1
+#endif
+
+#define W_BIT 0
+#define R_BIT 1
+#define I_BIT 2
+
+#define W_MASK (1 << W_BIT)
+#define R_MASK (1 << R_BIT)
+#define I_MASK (1 << I_BIT)
+
+#define IRW_MASK (I_MASK | R_MASK | W_MASK)
+
+enum pt_watch_style {
+ pt_watch_style_mips32,
+ pt_watch_style_mips64
+};
+
+#define MAX_DEBUG_REGISTER 8
+
+/* A value of zero in a watchlo indicates that it is available. */
+
+struct mips32_watch_regs
+{
+ uint32_t watchlo[MAX_DEBUG_REGISTER];
+ /* Lower 16 bits of watchhi. */
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ /* Valid mask and I R W bits.
+ * bit 0 -- 1 if W bit is usable.
+ * bit 1 -- 1 if R bit is usable.
+ * bit 2 -- 1 if I bit is usable.
+ * bits 3 - 11 -- Valid watchhi mask bits.
+ */
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ /* The number of valid watch register pairs. */
+ uint32_t num_valid;
+ /* There is confusion across gcc versions about structure alignment,
+ so we force 8 byte alignment for these structures so they match
+ the kernel even if it was build with a different gcc version. */
+} __attribute__ ((aligned (8)));
+
+struct mips64_watch_regs
+{
+ uint64_t watchlo[MAX_DEBUG_REGISTER];
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ uint32_t num_valid;
+} __attribute__ ((aligned (8)));
+
+struct pt_watch_regs
+{
+ enum pt_watch_style style;
+ union
+ {
+ struct mips32_watch_regs mips32;
+ struct mips64_watch_regs mips64;
+ };
+};
+
+/* -1 if the kernel and/or CPU do not support watch registers.
+ 1 if watch_readback is valid and we can read style, num_valid
+ and the masks.
+ 0 if we need to read the watch_readback. */
+
+static int watch_readback_valid;
+
+/* Cached watch register read values. */
+
+static struct pt_watch_regs watch_readback;
+
+/* We keep list of all watchpoints we should install and calculate the
+ watch register values each time the list changes. This allows for
+ easy sharing of watch registers for more than one watchpoint. */
+
+struct mips_watchpoint
+{
+ CORE_ADDR addr;
+ int len;
+ int type;
+ struct mips_watchpoint *next;
+};
+
+static struct mips_watchpoint *current_watches;
+
+/* The current set of watch register values for writing the
+ registers. */
+
+static struct pt_watch_regs watch_mirror;
+
+/* Assuming usable watch registers, return the irw_mask. */
+
+static uint32_t
+get_irw_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & IRW_MASK;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the reg_mask. */
+
+static uint32_t
+get_reg_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & ~IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & ~IRW_MASK;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the num_valid. */
+
+static uint32_t
+get_num_valid (struct pt_watch_regs *regs)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.num_valid;
+ case pt_watch_style_mips64:
+ return regs->mips64.num_valid;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the watchlo. */
+
+static CORE_ADDR
+get_watchlo (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchlo[set];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchlo[set];
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, set a watchlo value. */
+
+static void
+set_watchlo (struct pt_watch_regs *regs, int set, CORE_ADDR value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ /* The cast will never throw away bits as 64 bit addresses can
+ never be used on a 32 bit kernel. */
+ regs->mips32.watchlo[set] = (uint32_t)value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchlo[set] = value;
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the watchhi. */
+
+static uint32_t
+get_watchhi (struct pt_watch_regs *regs, int n)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchhi[n];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchhi[n];
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, set a watchhi value. */
+
+static void
+set_watchhi (struct pt_watch_regs *regs, int n, uint16_t value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ regs->mips32.watchhi[n] = value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchhi[n] = value;
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+static void
+mips_show_dr (const char *func, CORE_ADDR addr,
+ int len, enum target_hw_bp_type type)
+{
+ int i;
+
+ puts_unfiltered (func);
+ if (addr || len)
+ printf_unfiltered (" (addr=%llx, len=%d, type=%s)",
+ (unsigned long long)addr, len,
+ type == hw_write ? "data-write"
+ : (type == hw_read ? "data-read"
+ : (type == hw_access ? "data-read/write"
+ : (type == hw_execute ? "instruction-execute"
+ : "??unknown??"))));
+ puts_unfiltered (":\n");
+
+ for (i = 0; i < MAX_DEBUG_REGISTER; i++)
+ printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
+ i, paddr (get_watchlo (&watch_mirror, i)),
+ paddr (get_watchhi (&watch_mirror, i)));
+}
+
+/* Return 1 if watch registers are usable. Cached information is used
+ unless force is true. */
+
+static int
+mips_linux_read_watch_registers (int force)
+{
+ int tid;
+
+ if (force || watch_readback_valid == 0)
+ {
+ tid = ptid_get_lwp (inferior_ptid);
+ if (ptrace (PTRACE_GET_WATCH_REGS, tid, &watch_readback) == -1)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ switch (watch_readback.style)
+ {
+ case pt_watch_style_mips32:
+ if (watch_readback.mips32.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ case pt_watch_style_mips64:
+ if (watch_readback.mips64.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ default:
+ watch_readback_valid = -1;
+ return 0;
+ }
+ /* Watch registers appear to be usable. */
+ watch_readback_valid = 1;
+ }
+ return (watch_readback_valid == 1) ? 1 : 0;
+}
+
+/* Convert GDB's type to an IRW mask. */
+
+static unsigned
+type_to_irw (int type)
+{
+ switch (type)
+ {
+ case hw_write:
+ return W_MASK;
+ case hw_read:
+ return R_MASK;
+ case hw_access:
+ return (W_MASK | R_MASK);
+ default:
+ return 0;
+ }
+}
+
+/* Target to_can_use_hw_breakpoint implementation. Return 1 if we can
+ handle the specified watch type. */
+
+static int
+mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
+{
+ int i;
+ uint32_t wanted_mask, irw_mask;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ switch (type)
+ {
+ case bp_hardware_watchpoint:
+ wanted_mask = W_MASK;
+ break;
+ case bp_read_watchpoint:
+ wanted_mask = R_MASK;
+ break;
+ case bp_access_watchpoint:
+ wanted_mask = R_MASK | W_MASK;
+ break;
+ default:
+ return 0;
+ }
+
+ for (i = 0; i < get_num_valid (&watch_readback) && cnt; i++)
+ {
+ irw_mask = get_irw_mask (&watch_readback, i);
+ if ((irw_mask & wanted_mask) == wanted_mask)
+ cnt--;
+ }
+ return (cnt == 0) ? 1 : 0;
+}
+
+/* Target to_stopped_by_watchpoint implementation. Return 1 if
+ stopped by watchpoint. The watchhi R and W bits indicate the watch
+ register triggered. */
+
+static int
+mips_linux_stopped_by_watchpoint (void)
+{
+ int n;
+ int num_valid;
+
+ if (!mips_linux_read_watch_registers (1))
+ return 0;
+
+ num_valid = get_num_valid (&watch_readback);
+
+ for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
+ if (get_watchhi (&watch_readback, n) & (R_MASK | W_MASK))
+ return 1;
+
+ return 0;
+}
+
+/* Target to_stopped_data_address implementation. Set the address
+ where the watch triggered (if known). Return 1 if the address was
+ known. */
+
+static int
+mips_linux_stopped_data_address (struct target_ops *t, CORE_ADDR *paddr)
+{
+ /* On mips we don't know the low order 3 bits of the data address,
+ so we must return false. */
+ return 0;
+}
+
+/* Set any low order bits in mask that are not set. */
+
+static CORE_ADDR
+fill_mask (CORE_ADDR mask)
+{
+ CORE_ADDR f = 1;
+ while (f && f < mask)
+ {
+ mask |= f;
+ f <<= 1;
+ }
+ return mask;
+}
+
+/* Try to add a single watch to the specified registers. Return 1 on
+ success, 0 on failure. */
+
+static int
+try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
+ int len, unsigned irw)
+{
+ CORE_ADDR base_addr, last_byte, break_addr, segment_len;
+ CORE_ADDR mask_bits, t_low, t_low_end;
+ uint16_t t_hi;
+ int i, free_watches;
+ struct pt_watch_regs regs_copy;
+
+ if (len <= 0)
+ return 0;
+
+ last_byte = addr + len - 1;
+ mask_bits = fill_mask (addr ^ last_byte) | IRW_MASK;
+ base_addr = addr & ~mask_bits;
+
+ /* Check to see if it is covered by current registers. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low != 0 && irw == ((unsigned)t_low & irw))
+ {
+ t_hi = get_watchhi (regs, i) | IRW_MASK;
+ t_low &= ~(CORE_ADDR)t_hi;
+ if (addr >= t_low && last_byte <= (t_low + t_hi))
+ return 1;
+ }
+ }
+ /* Try to find an empty register. */
+ free_watches = 0;
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low == 0 && irw == (get_irw_mask (regs, i) & irw))
+ {
+ if (mask_bits <= (get_reg_mask (regs, i) | IRW_MASK))
+ {
+ /* It fits, we'll take it. */
+ set_watchlo (regs, i, base_addr | irw);
+ set_watchhi (regs, i, mask_bits & ~IRW_MASK);
+ return 1;
+ }
+ else
+ {
+ /* It doesn't fit, but has the proper IRW capabilities. */
+ free_watches++;
+ }
+ }
+ }
+ if (free_watches > 1)
+ {
+ /* Try to split it across several registers. */
+ regs_copy = *regs;
+ for (i = 0; i < get_num_valid (®s_copy); i++)
+ {
+ t_low = get_watchlo (®s_copy, i);
+ t_hi = get_reg_mask (®s_copy, i) | IRW_MASK;
+ if (t_low == 0 && irw == (t_hi & irw))
+ {
+ t_low = addr & ~(CORE_ADDR)t_hi;
+ break_addr = t_low + t_hi + 1;
+ if (break_addr >= addr + len)
+ segment_len = len;
+ else
+ segment_len = break_addr - addr;
+ mask_bits = fill_mask (addr ^ (addr + segment_len - 1));
+ set_watchlo (®s_copy, i, (addr & ~mask_bits) | irw);
+ set_watchhi (®s_copy, i, mask_bits & ~IRW_MASK);
+ if (break_addr >= addr + len)
+ {
+ *regs = regs_copy;
+ return 1;
+ }
+ len = addr + len - break_addr;
+ addr = break_addr;
+ }
+ }
+ }
+ /* It didn't fit anywhere, we failed. */
+ return 0;
+}
+
+/* Target to_region_ok_for_hw_watchpoint implementation. Return 1 if
+ the specified region can be covered by the watch registers. */
+
+static int
+mips_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ struct pt_watch_regs dummy_regs;
+ int i;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ dummy_regs = watch_readback;
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (&dummy_regs); i++)
+ set_watchlo (&dummy_regs, i, 0);
+ return try_one_watch (&dummy_regs, addr, len, 0);
+}
+
+
+/* Write the mirrored watch register values for each thread. */
+
+static int
+write_watchpoint_regs (void)
+{
+ struct lwp_info *lp;
+ ptid_t ptid;
+ int tid;
+
+ ALL_LWPS (lp, ptid)
+ {
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ perror_with_name (_("Couldn't write debug register"));
+ }
+ return 0;
+}
+
+/* linux_nat new_thread implementation. Write the mirrored watch
+ register values for the new thread. */
+
+static void
+mips_linux_new_thread (ptid_t ptid)
+{
+ int tid;
+
+ if (!mips_linux_read_watch_registers (0))
+ return;
+
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ perror_with_name (_("Couldn't write debug register"));
+}
+
+/* Fill in the watch registers with the currently cached watches. */
+
+static void
+populate_regs_from_watches (struct pt_watch_regs *regs)
+{
+ struct mips_watchpoint *w;
+ int i;
+
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ set_watchlo (regs, i, 0);
+ set_watchhi (regs, i, 0);
+ }
+
+ w = current_watches;
+ while (w)
+ {
+ i = try_one_watch (regs, w->addr, w->len, type_to_irw (w->type));
+ /* They must all fit, because we previously calculated that they
+ would. */
+ gdb_assert (i);
+ w = w->next;
+ }
+}
+
+/* Target to_insert_watchpoint implementation. Try to insert a new
+ watch. Return zero on success. */
+
+static int
+mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ struct pt_watch_regs regs;
+ struct mips_watchpoint *new_watch;
+ struct mips_watchpoint **pw;
+
+ int i;
+ int retval;
+
+ if (!mips_linux_read_watch_registers (0))
+ return -1;
+
+ if (len <= 0)
+ return -1;
+
+ regs = watch_readback;
+ /* Add the current watches. */
+ populate_regs_from_watches (®s);
+
+ /* Now try to add the new watch. */
+ if (!try_one_watch (®s, addr, len, type_to_irw (type)))
+ return -1;
+
+ /* It fit. Stick it on the end of the list. */
+ new_watch = (struct mips_watchpoint *)
+ xmalloc (sizeof (struct mips_watchpoint));
+ new_watch->addr = addr;
+ new_watch->len = len;
+ new_watch->type = type;
+ new_watch->next = NULL;
+
+ pw = ¤t_watches;
+ while (*pw != NULL)
+ pw = &(*pw)->next;
+ *pw = new_watch;
+
+ watch_mirror = regs;
+ retval = write_watchpoint_regs ();
+
+ if (maint_show_dr)
+ mips_show_dr ("insert_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_remove_watchpoint implementation. Try to remove a watch.
+ Return zero on success. */
+
+static int
+mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ int retval;
+ int deleted_one;
+
+ struct mips_watchpoint **pw;
+ struct mips_watchpoint *w;
+
+ /* Search for a known watch that matches. Then unlink and free
+ it. */
+ deleted_one = 0;
+ pw = ¤t_watches;
+ while ((w = *pw))
+ {
+ if (w->addr == addr && w->len == len && w->type == type)
+ {
+ *pw = w->next;
+ xfree (w);
+ deleted_one = 1;
+ break;
+ }
+ pw = &(w->next);
+ }
+
+ if (!deleted_one)
+ return -1; /* We don't know about it, fail doing nothing. */
+
+ /* At this point watch_readback is known to be valid because we
+ could not have added the watch without reading it. */
+ gdb_assert (watch_readback_valid == 1);
+
+ watch_mirror = watch_readback;
+ populate_regs_from_watches (&watch_mirror);
+
+ retval = write_watchpoint_regs ();
+
+ if (maint_show_dr)
+ mips_show_dr ("remove_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_close implementation. Free any watches and call the
+ super implementation. */
+
+static void
+mips_linux_close (int quitting)
+{
+ struct mips_watchpoint *w;
+ struct mips_watchpoint *nw;
+
+ /* Clean out the current_watches list. */
+ w = current_watches;
+ while (w)
+ {
+ nw = w->next;
+ xfree (w);
+ w = nw;
+ }
+ current_watches = NULL;
+
+ if (super_close)
+ super_close (quitting);
+}
+
void _initialize_mips_linux_nat (void);
void
_initialize_mips_linux_nat (void)
{
- struct target_ops *t = linux_trad_target (mips_linux_register_u_offset);
+ struct target_ops *t;
+
+ deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
+ var_boolean, (char *) &maint_show_dr, _("\
+Set whether to show variables that mirror the mips debug registers.\n\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the debug registers values are shown when GDB inserts\n\
+or removes a hardware breakpoint or watchpoint, and when the inferior\n\
+triggers a breakpoint or watchpoint."),
+ &maintenancelist);
+
+
+ t = linux_trad_target (mips_linux_register_u_offset);
+
+ super_close = t->to_close;
+ t->to_close = mips_linux_close;
super_fetch_registers = t->to_fetch_registers;
super_store_registers = t->to_store_registers;
@@ -370,9 +1066,17 @@ _initialize_mips_linux_nat (void)
t->to_fetch_registers = mips64_linux_fetch_registers;
t->to_store_registers = mips64_linux_store_registers;
+ t->to_can_use_hw_breakpoint = mips_linux_can_use_hw_breakpoint;
+ t->to_remove_watchpoint = mips_linux_remove_watchpoint;
+ t->to_insert_watchpoint = mips_linux_insert_watchpoint;
+ t->to_stopped_by_watchpoint = mips_linux_stopped_by_watchpoint;
+ t->to_stopped_data_address = mips_linux_stopped_data_address;
+ t->to_region_ok_for_hw_watchpoint = mips_linux_region_ok_for_hw_watchpoint;
+
t->to_read_description = mips_linux_read_description;
linux_nat_add_target (t);
+ linux_nat_set_new_thread (t, mips_linux_new_thread);
/* Initialize the standard target descriptions. */
initialize_tdesc_mips_linux ();
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.579
diff -u -p -r1.579 gdb.texinfo
--- doc/gdb.texinfo 9 Apr 2009 20:11:57 -0000 1.579
+++ doc/gdb.texinfo 16 Apr 2009 16:34:59 -0000
@@ -25715,9 +25715,9 @@ Configuring with @samp{--enable-profilin
compiled with the @samp{-pg} compiler option.
@kindex maint show-debug-regs
-@cindex x86 hardware debug registers
+@cindex hardware debug registers
@item maint show-debug-regs
-Control whether to show variables that mirror the x86 hardware debug
+Control whether to show variables that mirror the hardware debug
registers. Use @code{ON} to enable, @code{OFF} to disable. If
enabled, the debug registers values are shown when @value{GDBN} inserts or
removes a hardware breakpoint or watchpoint, and when the inferior
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 2/2] MIPS hardware watchpoint support.
2009-04-16 17:08 ` David Daney
@ 2009-04-17 13:43 ` Pedro Alves
2009-04-20 18:31 ` [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.' David Daney
2009-04-20 18:40 ` [Patch 2/2] MIPS hardware watchpoint support David Daney
0 siblings, 2 replies; 10+ messages in thread
From: Pedro Alves @ 2009-04-17 13:43 UTC (permalink / raw)
To: David Daney; +Cc: gdb-patches
Thanks for the update.
On Thursday 16 April 2009 18:08:07, David Daney wrote:
> >> + printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
> >> + /* This code is for mips, so casting CORE_ADDR
> >> + to unsigned long should be okay. */
> >> + (unsigned long)addr, len,
> >
> > Why bother to explain that instead of using `paddr'? If there's
> > a reason, then it would be nice if it was spelled out in the
> > comment.
>
> Comment deleted, and types casted to (unsigned long long).
Sorry to insist on a debug printf..., but that doesn't explain anything.
This even looks more misterious. If this is an address, why the cast
instead of paddr? -- you use paddr just a bit below. Is this because addr
isn't really an address?
> >
> >> -PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger
> >> +FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (unknown output after
> >> running)
> >>
> >> Reported instruction location of the watchpoint trigger is one
> >> instruction later and one line later.
> >
> > Why is that?
>
> I was mistaken, it is earlier, not later. With hardware watch points,
> $pc points to the faulting instruction, with software watch points $pc
> points to the following instruction. In a couple of tests, this results
> in the reported line number being different than the expected value.
I'm missing something and am still confused. PC points at the
faulting instruction when the target reports the watchpoint hit to
infrun --- . That step-over-watchpoint dance that patch 1/2 took care of comes
into play. That should move the inferior to the following instruction, evaluate
the watchpoint expression, notice the value changed, and report to the
user. Why does the PC still point at the faulting instruction?
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.'
2009-04-17 13:43 ` Pedro Alves
@ 2009-04-20 18:31 ` David Daney
2009-04-20 19:07 ` Pedro Alves
2009-04-20 18:40 ` [Patch 2/2] MIPS hardware watchpoint support David Daney
1 sibling, 1 reply; 10+ messages in thread
From: David Daney @ 2009-04-20 18:31 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]
Pedro Alves wrote:
> Thanks for the update.
>
> On Thursday 16 April 2009 18:08:07, David Daney wrote:
>>>> Reported instruction location of the watchpoint trigger is one
>>>> instruction later and one line later.
>>> Why is that?
>> I was mistaken, it is earlier, not later. With hardware watch points,
>> $pc points to the faulting instruction, with software watch points $pc
>> points to the following instruction. In a couple of tests, this results
>> in the reported line number being different than the expected value.
>
> I'm missing something and am still confused. PC points at the
> faulting instruction when the target reports the watchpoint hit to
> infrun --- . That step-over-watchpoint dance that patch 1/2 took care of comes
> into play. That should move the inferior to the following instruction, evaluate
> the watchpoint expression, notice the value changed, and report to the
> user. Why does the PC still point at the faulting instruction?
>
This patch had a small problem:
http://sourceware.org/ml/gdb-patches/2009-04/msg00245.html
I was calling registers_changed() to clear the register cache, but then
immediately calling read_pc() which caused it to be reloaded. After the
single step to move past the watchpoint, the old cached register values
were used instead of the current values.
The fix: Move the registers_changed() call after read_pc().
Tested on x86_64-unknown-linux-gnu and mips64-unknown-linux-gnu with no
regressions.
OK to commit?
[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 799 bytes --]
2009-04-20 David Daney <ddaney@caviumnetworks.com>
* infrun.c (handle_inferior_event): Move registers_changed call down.
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.368
diff -u -p -r1.368 infrun.c
--- infrun.c 14 Apr 2009 00:59:47 -0000 1.368
+++ infrun.c 20 Apr 2009 18:14:35 -0000
@@ -2842,9 +2842,9 @@ targets should add new threads to the th
if (!HAVE_STEPPABLE_WATCHPOINT)
remove_breakpoints ();
- registers_changed ();
/* Single step */
hw_step = maybe_software_singlestep (current_gdbarch, read_pc ());
+ registers_changed ();
target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0);
waiton_ptid = ecs->ptid;
if (HAVE_STEPPABLE_WATCHPOINT)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Patch 2/2] MIPS hardware watchpoint support.
2009-04-17 13:43 ` Pedro Alves
2009-04-20 18:31 ` [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.' David Daney
@ 2009-04-20 18:40 ` David Daney
2009-04-20 19:12 ` Pedro Alves
1 sibling, 1 reply; 10+ messages in thread
From: David Daney @ 2009-04-20 18:40 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]
Pedro Alves wrote:
> Thanks for the update.
>
> On Thursday 16 April 2009 18:08:07, David Daney wrote:
>>>> + printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
>>>> + /* This code is for mips, so casting CORE_ADDR
>>>> + to unsigned long should be okay. */
>>>> + (unsigned long)addr, len,
>>> Why bother to explain that instead of using `paddr'? If there's
>>> a reason, then it would be nice if it was spelled out in the
>>> comment.
>> Comment deleted, and types casted to (unsigned long long).
>
> Sorry to insist on a debug printf..., but that doesn't explain anything.
> This even looks more misterious. If this is an address, why the cast
> instead of paddr? -- you use paddr just a bit below. Is this because addr
> isn't really an address?
>
You are right. Using paddr() is the correct thing to do here. Now fixed.
Coupled with the new infrun.c patch:
http://sourceware.org/ml/gdb-patches/2009-04/msg00529.html
I now have no regressions in the gdb testsuite for
mips64-unknown-linux-gnu with this patch.
OK to commit?
[-- Attachment #2: mips-linux-nat.patch --]
[-- Type: text/plain, Size: 21054 bytes --]
2009-04-20 David Daney <ddaney@caviumnetworks.com>
* mips-linux-nat.c (command.h, gdbcmd.h, gdb_assert.h): New #includes.
(maint_show_dr, super_close): New variables.
(super_fetch_registers, super_store_registers): Make static.
(PTRACE_GET_WATCH_REGS, PTRACE_SET_WATCH_REGS, W_BIT, R_BIT, I_BIT)
(W_MASK, R_MASK, I_MASK, IRW_MASK, MAX_DEBUG_REGISTER): Define.
(pt_watch_style): Define new enum.
(mips32_watch_regs, mips64_watch_regs, pt_watch_regs, mips_watchpoint):
Define new structs.
(watch_readback_valid, watch_readback, current_watches, watch_mirror):
New variables.
(get_irw_mask, get_reg_mask, get_num_valid, get_watchlo)
(set_watchlo, get_watchhi, set_watchhi, mips_show_dr)
(mips_linux_read_watch_registers, mips_linux_can_use_hw_breakpoint)
(mips_linux_stopped_by_watchpoint, mips_linux_stopped_data_address)
(type_to_irw, fill_mask, try_one_watch)
(mips_linux_region_ok_for_hw_watchpoint, write_watchpoint_regs)
(mips_linux_new_thread, populate_regs_from_watches)
(mips_linux_insert_watchpoint, mips_linux_remove_watchpoint)
(mips_linux_close): New functions.
(_initialize_mips_linux_nat): Register watchpoint functions with
the target_ops. Add show-debug-regs maintenance command.
Index: mips-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-nat.c,v
retrieving revision 1.32
diff -u -p -r1.32 mips-linux-nat.c
--- mips-linux-nat.c 26 Feb 2009 19:44:39 -0000 1.32
+++ mips-linux-nat.c 20 Apr 2009 18:16:09 -0000
@@ -19,6 +19,9 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "command.h"
+#include "gdbcmd.h"
+#include "gdb_assert.h"
#include "inferior.h"
#include "mips-tdep.h"
#include "target.h"
@@ -44,11 +47,19 @@
we'll clear this and use PTRACE_PEEKUSER instead. */
static int have_ptrace_regsets = 1;
+/* Whether or not to print the mirrored debug registers. */
+
+static int maint_show_dr;
+
/* Saved function pointers to fetch and store a single register using
PTRACE_PEEKUSER and PTRACE_POKEUSER. */
-void (*super_fetch_registers) (struct target_ops *, struct regcache *, int);
-void (*super_store_registers) (struct target_ops *, struct regcache *, int);
+static void (*super_fetch_registers) (struct target_ops *,
+ struct regcache *, int);
+static void (*super_store_registers) (struct target_ops *,
+ struct regcache *, int);
+
+static void (*super_close) (int);
/* Map gdb internal register number to ptrace ``address''.
These ``addresses'' are normally defined in <asm/ptrace.h>.
@@ -357,12 +368,696 @@ mips_linux_read_description (struct targ
return tdesc_mips64_linux;
}
+#ifndef PTRACE_GET_WATCH_REGS
+# define PTRACE_GET_WATCH_REGS 0xd0
+#endif
+
+#ifndef PTRACE_SET_WATCH_REGS
+# define PTRACE_SET_WATCH_REGS 0xd1
+#endif
+
+#define W_BIT 0
+#define R_BIT 1
+#define I_BIT 2
+
+#define W_MASK (1 << W_BIT)
+#define R_MASK (1 << R_BIT)
+#define I_MASK (1 << I_BIT)
+
+#define IRW_MASK (I_MASK | R_MASK | W_MASK)
+
+enum pt_watch_style {
+ pt_watch_style_mips32,
+ pt_watch_style_mips64
+};
+
+#define MAX_DEBUG_REGISTER 8
+
+/* A value of zero in a watchlo indicates that it is available. */
+
+struct mips32_watch_regs
+{
+ uint32_t watchlo[MAX_DEBUG_REGISTER];
+ /* Lower 16 bits of watchhi. */
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ /* Valid mask and I R W bits.
+ * bit 0 -- 1 if W bit is usable.
+ * bit 1 -- 1 if R bit is usable.
+ * bit 2 -- 1 if I bit is usable.
+ * bits 3 - 11 -- Valid watchhi mask bits.
+ */
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ /* The number of valid watch register pairs. */
+ uint32_t num_valid;
+ /* There is confusion across gcc versions about structure alignment,
+ so we force 8 byte alignment for these structures so they match
+ the kernel even if it was build with a different gcc version. */
+} __attribute__ ((aligned (8)));
+
+struct mips64_watch_regs
+{
+ uint64_t watchlo[MAX_DEBUG_REGISTER];
+ uint16_t watchhi[MAX_DEBUG_REGISTER];
+ uint16_t watch_masks[MAX_DEBUG_REGISTER];
+ uint32_t num_valid;
+} __attribute__ ((aligned (8)));
+
+struct pt_watch_regs
+{
+ enum pt_watch_style style;
+ union
+ {
+ struct mips32_watch_regs mips32;
+ struct mips64_watch_regs mips64;
+ };
+};
+
+/* -1 if the kernel and/or CPU do not support watch registers.
+ 1 if watch_readback is valid and we can read style, num_valid
+ and the masks.
+ 0 if we need to read the watch_readback. */
+
+static int watch_readback_valid;
+
+/* Cached watch register read values. */
+
+static struct pt_watch_regs watch_readback;
+
+/* We keep list of all watchpoints we should install and calculate the
+ watch register values each time the list changes. This allows for
+ easy sharing of watch registers for more than one watchpoint. */
+
+struct mips_watchpoint
+{
+ CORE_ADDR addr;
+ int len;
+ int type;
+ struct mips_watchpoint *next;
+};
+
+static struct mips_watchpoint *current_watches;
+
+/* The current set of watch register values for writing the
+ registers. */
+
+static struct pt_watch_regs watch_mirror;
+
+/* Assuming usable watch registers, return the irw_mask. */
+
+static uint32_t
+get_irw_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & IRW_MASK;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the reg_mask. */
+
+static uint32_t
+get_reg_mask (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watch_masks[set] & ~IRW_MASK;
+ case pt_watch_style_mips64:
+ return regs->mips64.watch_masks[set] & ~IRW_MASK;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the num_valid. */
+
+static uint32_t
+get_num_valid (struct pt_watch_regs *regs)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.num_valid;
+ case pt_watch_style_mips64:
+ return regs->mips64.num_valid;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the watchlo. */
+
+static CORE_ADDR
+get_watchlo (struct pt_watch_regs *regs, int set)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchlo[set];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchlo[set];
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, set a watchlo value. */
+
+static void
+set_watchlo (struct pt_watch_regs *regs, int set, CORE_ADDR value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ /* The cast will never throw away bits as 64 bit addresses can
+ never be used on a 32 bit kernel. */
+ regs->mips32.watchlo[set] = (uint32_t)value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchlo[set] = value;
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, return the watchhi. */
+
+static uint32_t
+get_watchhi (struct pt_watch_regs *regs, int n)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ return regs->mips32.watchhi[n];
+ case pt_watch_style_mips64:
+ return regs->mips64.watchhi[n];
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+/* Assuming usable watch registers, set a watchhi value. */
+
+static void
+set_watchhi (struct pt_watch_regs *regs, int n, uint16_t value)
+{
+ switch (regs->style)
+ {
+ case pt_watch_style_mips32:
+ regs->mips32.watchhi[n] = value;
+ break;
+ case pt_watch_style_mips64:
+ regs->mips64.watchhi[n] = value;
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("Unrecognized watch register style"));
+ }
+}
+
+static void
+mips_show_dr (const char *func, CORE_ADDR addr,
+ int len, enum target_hw_bp_type type)
+{
+ int i;
+
+ puts_unfiltered (func);
+ if (addr || len)
+ printf_unfiltered (" (addr=0x%s, len=%d, type=%s)", paddr (addr), len,
+ type == hw_write ? "data-write"
+ : (type == hw_read ? "data-read"
+ : (type == hw_access ? "data-read/write"
+ : (type == hw_execute ? "instruction-execute"
+ : "??unknown??"))));
+ puts_unfiltered (":\n");
+
+ for (i = 0; i < MAX_DEBUG_REGISTER; i++)
+ printf_unfiltered ("\tDR%d: lo=0x%s, hi=0x%s\n",
+ i, paddr (get_watchlo (&watch_mirror, i)),
+ paddr (get_watchhi (&watch_mirror, i)));
+}
+
+/* Return 1 if watch registers are usable. Cached information is used
+ unless force is true. */
+
+static int
+mips_linux_read_watch_registers (int force)
+{
+ int tid;
+
+ if (force || watch_readback_valid == 0)
+ {
+ tid = ptid_get_lwp (inferior_ptid);
+ if (ptrace (PTRACE_GET_WATCH_REGS, tid, &watch_readback) == -1)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ switch (watch_readback.style)
+ {
+ case pt_watch_style_mips32:
+ if (watch_readback.mips32.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ case pt_watch_style_mips64:
+ if (watch_readback.mips64.num_valid == 0)
+ {
+ watch_readback_valid = -1;
+ return 0;
+ }
+ break;
+ default:
+ watch_readback_valid = -1;
+ return 0;
+ }
+ /* Watch registers appear to be usable. */
+ watch_readback_valid = 1;
+ }
+ return (watch_readback_valid == 1) ? 1 : 0;
+}
+
+/* Convert GDB's type to an IRW mask. */
+
+static unsigned
+type_to_irw (int type)
+{
+ switch (type)
+ {
+ case hw_write:
+ return W_MASK;
+ case hw_read:
+ return R_MASK;
+ case hw_access:
+ return (W_MASK | R_MASK);
+ default:
+ return 0;
+ }
+}
+
+/* Target to_can_use_hw_breakpoint implementation. Return 1 if we can
+ handle the specified watch type. */
+
+static int
+mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
+{
+ int i;
+ uint32_t wanted_mask, irw_mask;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ switch (type)
+ {
+ case bp_hardware_watchpoint:
+ wanted_mask = W_MASK;
+ break;
+ case bp_read_watchpoint:
+ wanted_mask = R_MASK;
+ break;
+ case bp_access_watchpoint:
+ wanted_mask = R_MASK | W_MASK;
+ break;
+ default:
+ return 0;
+ }
+
+ for (i = 0; i < get_num_valid (&watch_readback) && cnt; i++)
+ {
+ irw_mask = get_irw_mask (&watch_readback, i);
+ if ((irw_mask & wanted_mask) == wanted_mask)
+ cnt--;
+ }
+ return (cnt == 0) ? 1 : 0;
+}
+
+/* Target to_stopped_by_watchpoint implementation. Return 1 if
+ stopped by watchpoint. The watchhi R and W bits indicate the watch
+ register triggered. */
+
+static int
+mips_linux_stopped_by_watchpoint (void)
+{
+ int n;
+ int num_valid;
+
+ if (!mips_linux_read_watch_registers (1))
+ return 0;
+
+ num_valid = get_num_valid (&watch_readback);
+
+ for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
+ if (get_watchhi (&watch_readback, n) & (R_MASK | W_MASK))
+ return 1;
+
+ return 0;
+}
+
+/* Target to_stopped_data_address implementation. Set the address
+ where the watch triggered (if known). Return 1 if the address was
+ known. */
+
+static int
+mips_linux_stopped_data_address (struct target_ops *t, CORE_ADDR *paddr)
+{
+ /* On mips we don't know the low order 3 bits of the data address,
+ so we must return false. */
+ return 0;
+}
+
+/* Set any low order bits in mask that are not set. */
+
+static CORE_ADDR
+fill_mask (CORE_ADDR mask)
+{
+ CORE_ADDR f = 1;
+ while (f && f < mask)
+ {
+ mask |= f;
+ f <<= 1;
+ }
+ return mask;
+}
+
+/* Try to add a single watch to the specified registers. Return 1 on
+ success, 0 on failure. */
+
+static int
+try_one_watch (struct pt_watch_regs *regs, CORE_ADDR addr,
+ int len, unsigned irw)
+{
+ CORE_ADDR base_addr, last_byte, break_addr, segment_len;
+ CORE_ADDR mask_bits, t_low, t_low_end;
+ uint16_t t_hi;
+ int i, free_watches;
+ struct pt_watch_regs regs_copy;
+
+ if (len <= 0)
+ return 0;
+
+ last_byte = addr + len - 1;
+ mask_bits = fill_mask (addr ^ last_byte) | IRW_MASK;
+ base_addr = addr & ~mask_bits;
+
+ /* Check to see if it is covered by current registers. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low != 0 && irw == ((unsigned)t_low & irw))
+ {
+ t_hi = get_watchhi (regs, i) | IRW_MASK;
+ t_low &= ~(CORE_ADDR)t_hi;
+ if (addr >= t_low && last_byte <= (t_low + t_hi))
+ return 1;
+ }
+ }
+ /* Try to find an empty register. */
+ free_watches = 0;
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ t_low = get_watchlo (regs, i);
+ if (t_low == 0 && irw == (get_irw_mask (regs, i) & irw))
+ {
+ if (mask_bits <= (get_reg_mask (regs, i) | IRW_MASK))
+ {
+ /* It fits, we'll take it. */
+ set_watchlo (regs, i, base_addr | irw);
+ set_watchhi (regs, i, mask_bits & ~IRW_MASK);
+ return 1;
+ }
+ else
+ {
+ /* It doesn't fit, but has the proper IRW capabilities. */
+ free_watches++;
+ }
+ }
+ }
+ if (free_watches > 1)
+ {
+ /* Try to split it across several registers. */
+ regs_copy = *regs;
+ for (i = 0; i < get_num_valid (®s_copy); i++)
+ {
+ t_low = get_watchlo (®s_copy, i);
+ t_hi = get_reg_mask (®s_copy, i) | IRW_MASK;
+ if (t_low == 0 && irw == (t_hi & irw))
+ {
+ t_low = addr & ~(CORE_ADDR)t_hi;
+ break_addr = t_low + t_hi + 1;
+ if (break_addr >= addr + len)
+ segment_len = len;
+ else
+ segment_len = break_addr - addr;
+ mask_bits = fill_mask (addr ^ (addr + segment_len - 1));
+ set_watchlo (®s_copy, i, (addr & ~mask_bits) | irw);
+ set_watchhi (®s_copy, i, mask_bits & ~IRW_MASK);
+ if (break_addr >= addr + len)
+ {
+ *regs = regs_copy;
+ return 1;
+ }
+ len = addr + len - break_addr;
+ addr = break_addr;
+ }
+ }
+ }
+ /* It didn't fit anywhere, we failed. */
+ return 0;
+}
+
+/* Target to_region_ok_for_hw_watchpoint implementation. Return 1 if
+ the specified region can be covered by the watch registers. */
+
+static int
+mips_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+ struct pt_watch_regs dummy_regs;
+ int i;
+
+ if (!mips_linux_read_watch_registers (0))
+ return 0;
+
+ dummy_regs = watch_readback;
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (&dummy_regs); i++)
+ set_watchlo (&dummy_regs, i, 0);
+ return try_one_watch (&dummy_regs, addr, len, 0);
+}
+
+
+/* Write the mirrored watch register values for each thread. */
+
+static int
+write_watchpoint_regs (void)
+{
+ struct lwp_info *lp;
+ ptid_t ptid;
+ int tid;
+
+ ALL_LWPS (lp, ptid)
+ {
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ perror_with_name (_("Couldn't write debug register"));
+ }
+ return 0;
+}
+
+/* linux_nat new_thread implementation. Write the mirrored watch
+ register values for the new thread. */
+
+static void
+mips_linux_new_thread (ptid_t ptid)
+{
+ int tid;
+
+ if (!mips_linux_read_watch_registers (0))
+ return;
+
+ tid = ptid_get_lwp (ptid);
+ if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror) == -1)
+ perror_with_name (_("Couldn't write debug register"));
+}
+
+/* Fill in the watch registers with the currently cached watches. */
+
+static void
+populate_regs_from_watches (struct pt_watch_regs *regs)
+{
+ struct mips_watchpoint *w;
+ int i;
+
+ /* Clear them out. */
+ for (i = 0; i < get_num_valid (regs); i++)
+ {
+ set_watchlo (regs, i, 0);
+ set_watchhi (regs, i, 0);
+ }
+
+ w = current_watches;
+ while (w)
+ {
+ i = try_one_watch (regs, w->addr, w->len, type_to_irw (w->type));
+ /* They must all fit, because we previously calculated that they
+ would. */
+ gdb_assert (i);
+ w = w->next;
+ }
+}
+
+/* Target to_insert_watchpoint implementation. Try to insert a new
+ watch. Return zero on success. */
+
+static int
+mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ struct pt_watch_regs regs;
+ struct mips_watchpoint *new_watch;
+ struct mips_watchpoint **pw;
+
+ int i;
+ int retval;
+
+ if (!mips_linux_read_watch_registers (0))
+ return -1;
+
+ if (len <= 0)
+ return -1;
+
+ regs = watch_readback;
+ /* Add the current watches. */
+ populate_regs_from_watches (®s);
+
+ /* Now try to add the new watch. */
+ if (!try_one_watch (®s, addr, len, type_to_irw (type)))
+ return -1;
+
+ /* It fit. Stick it on the end of the list. */
+ new_watch = (struct mips_watchpoint *)
+ xmalloc (sizeof (struct mips_watchpoint));
+ new_watch->addr = addr;
+ new_watch->len = len;
+ new_watch->type = type;
+ new_watch->next = NULL;
+
+ pw = ¤t_watches;
+ while (*pw != NULL)
+ pw = &(*pw)->next;
+ *pw = new_watch;
+
+ watch_mirror = regs;
+ retval = write_watchpoint_regs ();
+
+ if (maint_show_dr)
+ mips_show_dr ("insert_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_remove_watchpoint implementation. Try to remove a watch.
+ Return zero on success. */
+
+static int
+mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+ int retval;
+ int deleted_one;
+
+ struct mips_watchpoint **pw;
+ struct mips_watchpoint *w;
+
+ /* Search for a known watch that matches. Then unlink and free
+ it. */
+ deleted_one = 0;
+ pw = ¤t_watches;
+ while ((w = *pw))
+ {
+ if (w->addr == addr && w->len == len && w->type == type)
+ {
+ *pw = w->next;
+ xfree (w);
+ deleted_one = 1;
+ break;
+ }
+ pw = &(w->next);
+ }
+
+ if (!deleted_one)
+ return -1; /* We don't know about it, fail doing nothing. */
+
+ /* At this point watch_readback is known to be valid because we
+ could not have added the watch without reading it. */
+ gdb_assert (watch_readback_valid == 1);
+
+ watch_mirror = watch_readback;
+ populate_regs_from_watches (&watch_mirror);
+
+ retval = write_watchpoint_regs ();
+
+ if (maint_show_dr)
+ mips_show_dr ("remove_watchpoint", addr, len, type);
+
+ return retval;
+}
+
+/* Target to_close implementation. Free any watches and call the
+ super implementation. */
+
+static void
+mips_linux_close (int quitting)
+{
+ struct mips_watchpoint *w;
+ struct mips_watchpoint *nw;
+
+ /* Clean out the current_watches list. */
+ w = current_watches;
+ while (w)
+ {
+ nw = w->next;
+ xfree (w);
+ w = nw;
+ }
+ current_watches = NULL;
+
+ if (super_close)
+ super_close (quitting);
+}
+
void _initialize_mips_linux_nat (void);
void
_initialize_mips_linux_nat (void)
{
- struct target_ops *t = linux_trad_target (mips_linux_register_u_offset);
+ struct target_ops *t;
+
+ deprecated_add_set_cmd ("show-debug-regs", class_maintenance,
+ var_boolean, (char *) &maint_show_dr, _("\
+Set whether to show variables that mirror the mips debug registers.\n\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the debug registers values are shown when GDB inserts\n\
+or removes a hardware breakpoint or watchpoint, and when the inferior\n\
+triggers a breakpoint or watchpoint."),
+ &maintenancelist);
+
+
+ t = linux_trad_target (mips_linux_register_u_offset);
+
+ super_close = t->to_close;
+ t->to_close = mips_linux_close;
super_fetch_registers = t->to_fetch_registers;
super_store_registers = t->to_store_registers;
@@ -370,9 +1065,17 @@ _initialize_mips_linux_nat (void)
t->to_fetch_registers = mips64_linux_fetch_registers;
t->to_store_registers = mips64_linux_store_registers;
+ t->to_can_use_hw_breakpoint = mips_linux_can_use_hw_breakpoint;
+ t->to_remove_watchpoint = mips_linux_remove_watchpoint;
+ t->to_insert_watchpoint = mips_linux_insert_watchpoint;
+ t->to_stopped_by_watchpoint = mips_linux_stopped_by_watchpoint;
+ t->to_stopped_data_address = mips_linux_stopped_data_address;
+ t->to_region_ok_for_hw_watchpoint = mips_linux_region_ok_for_hw_watchpoint;
+
t->to_read_description = mips_linux_read_description;
linux_nat_add_target (t);
+ linux_nat_set_new_thread (t, mips_linux_new_thread);
/* Initialize the standard target descriptions. */
initialize_tdesc_mips_linux ();
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.'
2009-04-20 18:31 ` [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.' David Daney
@ 2009-04-20 19:07 ` Pedro Alves
2009-04-20 21:15 ` David Daney
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2009-04-20 19:07 UTC (permalink / raw)
To: gdb-patches; +Cc: David Daney
On Monday 20 April 2009 19:31:16, David Daney wrote:
> This patch had a small problem:
>
> http://sourceware.org/ml/gdb-patches/2009-04/msg00245.html
>
> I was calling registers_changed() to clear the register cache, but then
> immediately calling read_pc() which caused it to be reloaded. After the
> single step to move past the watchpoint, the old cached register values
> were used instead of the current values.
>
> The fix: Move the registers_changed() call after read_pc().
>
> Tested on x86_64-unknown-linux-gnu and mips64-unknown-linux-gnu with no
> regressions.
>
> OK to commit?
I see. There's a call to prepare_to_wait just below, that
usually calls registers_changed, but ... surprise, surprise, ...
static void
prepare_to_wait (struct execution_control_state *ecs)
{
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: prepare_to_wait\n");
if (infwait_state == infwait_normal_state)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
{
overlay_cache_invalid = 1;
/* We have to invalidate the registers BEFORE calling
target_wait because they can be loaded from the target while
in target_wait. This makes remote debugging a bit more
efficient for those targets that provide critical registers
as part of their normal status mechanism. */
registers_changed ();
^^^^^^^^^^^^^^^^^^^^
waiton_ptid = pid_to_ptid (-1);
}
/* This is the old end of the while loop. Let everybody know we
want to wait for the inferior some more and get called again
soon. */
ecs->wait_some_more = 1;
}
... it's skipped this time, because infwait_state != infwait_normal_state.
This just makes no sense, the register cache should *always* be flushed if
we resume the target. Also, wait_for_inferior only calls registers_changed
once, on entry to the loop, instead of calling it always before calling
target_wait, which would be much simpler... I'm fairly sure Ulrich was
cleaning this up ... ah, here:
http://sourceware.org/ml/gdb-patches/2008-12/msg00120.html
Ulrich, maybe we should apply the pieces of that series
we're happy with?
In the mean time, your patch is OK. I'd just move the
registers_changed call to *after* target_resume, because the
target_resume implementation could trigger a register cache
refetch, thus reintroducing the problem (e.g., it doesn't happen
on mips *today*, but see e.g., i386-linux-nat.c:i386_linux_resume). I'd
put it right after the prepare_to_wait call.
> 2009-04-20 David Daney <ddaney@caviumnetworks.com>
>
> * infrun.c (handle_inferior_event): Move registers_changed call down.
>
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.368
> diff -u -p -r1.368 infrun.c
> --- infrun.c 14 Apr 2009 00:59:47 -0000 1.368
> +++ infrun.c 20 Apr 2009 18:14:35 -0000
> @@ -2842,9 +2842,9 @@ targets should add new threads to the th
>
> if (!HAVE_STEPPABLE_WATCHPOINT)
> remove_breakpoints ();
> - registers_changed ();
> /* Single step */
> hw_step = maybe_software_singlestep (current_gdbarch, read_pc ());
> + registers_changed ();
> target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0);
> waiton_ptid = ecs->ptid;
> if (HAVE_STEPPABLE_WATCHPOINT)
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 2/2] MIPS hardware watchpoint support.
2009-04-20 18:40 ` [Patch 2/2] MIPS hardware watchpoint support David Daney
@ 2009-04-20 19:12 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2009-04-20 19:12 UTC (permalink / raw)
To: gdb-patches; +Cc: David Daney
On Monday 20 April 2009 19:40:23, David Daney wrote:
> OK to commit?
Yes. Thank you!
--
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.'
2009-04-20 19:07 ` Pedro Alves
@ 2009-04-20 21:15 ` David Daney
0 siblings, 0 replies; 10+ messages in thread
From: David Daney @ 2009-04-20 21:15 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 754 bytes --]
Pedro Alves wrote:
>
> In the mean time, your patch is OK. I'd just move the
> registers_changed call to *after* target_resume, because the
> target_resume implementation could trigger a register cache
> refetch, thus reintroducing the problem (e.g., it doesn't happen
> on mips *today*, but see e.g., i386-linux-nat.c:i386_linux_resume). I'd
> put it right after the prepare_to_wait call.
>
>> 2009-04-20 David Daney <ddaney@caviumnetworks.com>
>>
>> * infrun.c (handle_inferior_event): Move registers_changed call down.
>>
This is the version I committed.
Thanks for taking the time to review these.
2009-04-20 David Daney <ddaney@caviumnetworks.com>
* infrun.c (handle_inferior_event): Move registers_changed call
down.
[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 719 bytes --]
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.368
diff -u -p -r1.368 infrun.c
--- infrun.c 14 Apr 2009 00:59:47 -0000 1.368
+++ infrun.c 20 Apr 2009 21:06:10 -0000
@@ -2842,10 +2842,10 @@ targets should add new threads to the th
if (!HAVE_STEPPABLE_WATCHPOINT)
remove_breakpoints ();
- registers_changed ();
/* Single step */
hw_step = maybe_software_singlestep (current_gdbarch, read_pc ());
target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0);
+ registers_changed ();
waiton_ptid = ecs->ptid;
if (HAVE_STEPPABLE_WATCHPOINT)
infwait_state = infwait_step_watch_state;
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-04-20 21:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-06 7:27 [Patch 2/2] MIPS hardware watchpoint support David Daney
2009-04-06 20:27 ` Eli Zaretskii
2009-04-11 17:16 ` Pedro Alves
2009-04-16 17:08 ` David Daney
2009-04-17 13:43 ` Pedro Alves
2009-04-20 18:31 ` [Patch 1/2] Fix aftermath of 'infrun.c support for MIPS hardware watchpoints.' David Daney
2009-04-20 19:07 ` Pedro Alves
2009-04-20 21:15 ` David Daney
2009-04-20 18:40 ` [Patch 2/2] MIPS hardware watchpoint support David Daney
2009-04-20 19:12 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox