Re: [PATCH 5/5] MIPS GDBserver watchpoint

[Yao Qi <yao at codesourcery dot com>]

On 07/24/2013 08:34 AM, Maciej W. Rozycki wrote:
>> +static enum target_hw_bp_type
>> >+rsp_bp_type_to_target_hw_bp_type (char type)
>> >+{
>> >+  switch (type)
>> >+    {
>> >+    case '2':
>> >+      return hw_write;
>> >+    case '3':
>> >+      return hw_read;
>> >+    case '4':
>> >+      return hw_access;
>> >+    }
>> >+
>> >+  return -1;
>   This looks unclean to me, you're returning a value that's outside the
> valid range of the enum type used.  By the structure of the callers this
> code is expected to be dead though, so please use gdb_assert_not_reached.

OK, gdb_assert_not_reached is added.

> 
>   Please resend with the adjustment requested, the change looks otherwise
> OK to me.

Here is the updated patch, with adding more words in ChangeLog to
address Pedro's comment.

-- 
Yao (éå)

gdb/gdbserver:

2013-07-25  Jie Zhang  <jie@codesourcery.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>
	    Yao Qi  <yao@codesourcery.com>

	* Makefile.in (SFILES): Add common/mips-linux-watch.c.
	(mips-linux-watch.o): New rule.
	* configure.srv <mips*-*-linux*>: Add mips-linux-watch.o to
	srv_tgtobj.
	* linux-mips-low.c: Include mips-linux-watch.h.
	(struct arch_process_info, struct arch_lwp_info): New.
	(update_watch_registers_callback): New.
	(mips_linux_new_process, mips_linux_new_thread) New.
	(mips_linux_prepare_to_resume, mips_insert_point): New.
	(mips_remove_point, mips_stopped_by_watchpoint): New.
	(rsp_bp_type_to_target_hw_bp_type): New.
	(mips_stopped_data_address): New.
	(the_low_target): Add watchpoint support functions.

gdb:

2013-07-25  Yao Qi  <yao@codesourcery.com>

	* NEWS: Mention that GDBserver now supports hardware
	watchpoints on the MIPS GNU/Linux target.
---
 gdb/NEWS                       |    3 +
 gdb/gdbserver/Makefile.in      |    7 +-
 gdb/gdbserver/configure.srv    |    1 +
 gdb/gdbserver/linux-mips-low.c |  366 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 376 insertions(+), 1 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index a4238d0..31a8bc6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -128,6 +128,9 @@ qXfer:libraries-svr4:read's annex
      'qXfer:traceframe-info:read'.  It has the id of the collected
      trace state variables.
 
+  ** GDBserver now supports hardware watchpoints on the MIPS GNU/Linux
+     target.
+
 *** Changes in GDB 7.6
 
 * Target record has been renamed to record-full.
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 2cbf208..fe16cb4 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -157,7 +157,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/common-utils.c $(srcdir)/common/xml-utils.c \
 	$(srcdir)/common/linux-osdata.c $(srcdir)/common/ptid.c \
 	$(srcdir)/common/buffer.c $(srcdir)/common/linux-btrace.c \
-	$(srcdir)/common/filestuff.c
+	$(srcdir)/common/filestuff.c $(srcdir)/common/mips-linux-watch.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -457,6 +457,8 @@ lynx_low_h = $(srcdir)/lynx-low.h $(srcdir)/server.h
 
 nto_low_h = $(srcdir)/nto-low.h
 
+mips_linux_watch_h = $(srcdir)/../common/mips-linux-watch.h
+
 UST_CFLAGS = $(ustinc) -DCONFIG_UST_GDB_INTEGRATION
 
 # Note, we only build the IPA if -fvisibility=hidden is supported in
@@ -552,6 +554,9 @@ agent.o: ../common/agent.c
 linux-btrace.o: ../common/linux-btrace.c $(linux_btrace_h) $(server_h)
 	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
 
+mips-linux-watch.o: ../common/mips-linux-watch.c $(mips_linux_watch_h) $(server_h)
+	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
+
 # We build vasprintf with -DHAVE_CONFIG_H because we want that unit to
 # include our config.h file.  Otherwise, some system headers do not get
 # included, and the compiler emits a warning about implicitly defined
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index 879d0de..b9dfd6c 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -184,6 +184,7 @@ case "${target}" in
 			srv_regobj="${srv_regobj} mips64-dsp-linux.o"
 			srv_tgtobj="linux-low.o linux-osdata.o linux-mips-low.o linux-procfs.o"
 			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
+			srv_tgtobj="${srv_tgtobj} mips-linux-watch.o"
 			srv_xmlfiles="mips-linux.xml"
 			srv_xmlfiles="${srv_xmlfiles} mips-dsp-linux.xml"
 			srv_xmlfiles="${srv_xmlfiles} mips-cpu.xml"
diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
index 1010528..dd6ebaf 100644
--- a/gdb/gdbserver/linux-mips-low.c
+++ b/gdb/gdbserver/linux-mips-low.c
@@ -22,6 +22,7 @@
 #include <sys/ptrace.h>
 #include <endian.h>
 
+#include "mips-linux-watch.h"
 #include "gdb_proc_service.h"
 
 /* Defined in auto-generated file mips-linux.c.  */
@@ -159,6 +160,39 @@ get_usrregs_info (void)
   return regs_info->usrregs;
 }
 
+/* Per-process arch-specific data we want to keep.  */
+
+struct arch_process_info
+{
+  /* -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.  */
+
+  int watch_readback_valid;
+
+  /* Cached watch register read values.  */
+
+  struct pt_watch_regs watch_readback;
+
+  /* Current watchpoint requests for this process.  */
+
+  struct mips_watchpoint *current_watches;
+
+  /* The current set of watch register values for writing the
+     registers.  */
+
+  struct pt_watch_regs watch_mirror;
+};
+
+/* Per-thread arch-specific data we want to keep.  */
+
+struct arch_lwp_info
+{
+  /* Non-zero if our copy differs from what's recorded in the thread.  */
+  int watch_registers_changed;
+};
+
 /* From mips-linux-nat.c.  */
 
 /* Pseudo registers can not be read.  ptrace does not provide a way to
@@ -257,6 +291,328 @@ mips_breakpoint_at (CORE_ADDR where)
   return 0;
 }
 
+/* Mark the watch registers of lwp, represented by ENTRY, as changed,
+   if the lwp's process id is *PID_P.  */
+
+static int
+update_watch_registers_callback (struct inferior_list_entry *entry,
+				 void *pid_p)
+{
+  struct lwp_info *lwp = (struct lwp_info *) entry;
+  int pid = *(int *) pid_p;
+
+  /* Only update the threads of this process.  */
+  if (pid_of (lwp) == pid)
+    {
+      /* The actual update is done later just before resuming the lwp,
+	 we just mark that the registers need updating.  */
+      lwp->arch_private->watch_registers_changed = 1;
+
+      /* If the lwp isn't stopped, force it to momentarily pause, so
+	 we can update its watch registers.  */
+      if (!lwp->stopped)
+	linux_stop_lwp (lwp);
+    }
+
+  return 0;
+}
+
+/* This is the implementation of linux_target_ops method
+   new_process.  */
+
+static struct arch_process_info *
+mips_linux_new_process (void)
+{
+  struct arch_process_info *info = xcalloc (1, sizeof (*info));
+
+  return info;
+}
+
+/* This is the implementation of linux_target_ops method new_thread.
+   Mark the watch registers as changed, so the threads' copies will
+   be updated.  */
+
+static struct arch_lwp_info *
+mips_linux_new_thread (void)
+{
+  struct arch_lwp_info *info = xcalloc (1, sizeof (*info));
+
+  info->watch_registers_changed = 1;
+
+  return info;
+}
+
+/* This is the implementation of linux_target_ops method
+   prepare_to_resume.  If the watch regs have changed, update the
+   thread's copies.  */
+
+static void
+mips_linux_prepare_to_resume (struct lwp_info *lwp)
+{
+  ptid_t ptid = ptid_of (lwp);
+  struct process_info *proc = find_process_pid (ptid_get_pid (ptid));
+  struct arch_process_info *private = proc->private->arch_private;
+
+  if (lwp->arch_private->watch_registers_changed)
+    {
+      /* Only update the watch registers if we have set or unset a
+	 watchpoint already.  */
+      if (mips_linux_watch_get_num_valid (&private->watch_mirror) > 0)
+	{
+	  /* Write the mirrored watch register values.  */
+	  int tid = ptid_get_lwp (ptid);
+
+	  if (-1 == ptrace (PTRACE_SET_WATCH_REGS, tid,
+			    &private->watch_mirror))
+	    perror_with_name ("Couldn't write watch register");
+	}
+
+      lwp->arch_private->watch_registers_changed = 0;
+    }
+}
+
+/* Translate breakpoint type TYPE in rsp to 'enum target_hw_bp_type'.  */
+
+static enum target_hw_bp_type
+rsp_bp_type_to_target_hw_bp_type (char type)
+{
+  switch (type)
+    {
+    case '2':
+      return hw_write;
+    case '3':
+      return hw_read;
+    case '4':
+      return hw_access;
+    }
+
+  gdb_assert_not_reached ("unhandled RSP breakpoint type");
+  return -1;
+}
+
+/* This is the implementation of linux_target_ops method
+   insert_point.  */
+
+static int
+mips_insert_point (char type, CORE_ADDR addr, int len)
+{
+  struct process_info *proc = current_process ();
+  struct arch_process_info *private = proc->private->arch_private;
+  struct pt_watch_regs regs;
+  struct mips_watchpoint *new_watch;
+  struct mips_watchpoint **pw;
+  int pid;
+  long lwpid;
+  enum target_hw_bp_type watch_type;
+  uint32_t irw;
+
+  /* Breakpoint/watchpoint types:
+       '0' - software-breakpoint (not supported)
+       '1' - hardware-breakpoint (not supported)
+       '2' - write watchpoint (supported)
+       '3' - read watchpoint (supported)
+       '4' - access watchpoint (supported).  */
+
+  if (type < '2' || type > '4')
+    {
+      /* Unsupported.  */
+      return 1;
+    }
+
+  lwpid = lwpid_of (get_thread_lwp (current_inferior));
+  if (!mips_linux_read_watch_registers (lwpid,
+					&private->watch_readback,
+					&private->watch_readback_valid,
+					0))
+    return -1;
+
+  if (len <= 0)
+    return -1;
+
+  regs = private->watch_readback;
+  /* Add the current watches.  */
+  mips_linux_watch_populate_regs (private->current_watches, &regs);
+
+  /* Now try to add the new watch.  */
+  watch_type = rsp_bp_type_to_target_hw_bp_type (type);
+  irw = mips_linux_watch_type_to_irw (watch_type);
+  if (!mips_linux_watch_try_one_watch (&regs, addr, len, irw))
+    return -1;
+
+  /* It fit.  Stick it on the end of the list.  */
+  new_watch = xmalloc (sizeof (struct mips_watchpoint));
+  new_watch->addr = addr;
+  new_watch->len = len;
+  new_watch->type = watch_type;
+  new_watch->next = NULL;
+
+  pw = &private->current_watches;
+  while (*pw != NULL)
+    pw = &(*pw)->next;
+  *pw = new_watch;
+
+  private->watch_mirror = regs;
+
+  /* Only update the threads of this process.  */
+  pid = pid_of (proc);
+  find_inferior (&all_lwps, update_watch_registers_callback, &pid);
+
+  return 0;
+}
+
+/* This is the implementation of linux_target_ops method
+   remove_point.  */
+
+static int
+mips_remove_point (char type, CORE_ADDR addr, int len)
+{
+  struct process_info *proc = current_process ();
+  struct arch_process_info *private = proc->private->arch_private;
+
+  int deleted_one;
+  int pid;
+  enum target_hw_bp_type watch_type;
+
+  struct mips_watchpoint **pw;
+  struct mips_watchpoint *w;
+
+  /* Breakpoint/watchpoint types:
+       '0' - software-breakpoint (not supported)
+       '1' - hardware-breakpoint (not supported)
+       '2' - write watchpoint (supported)
+       '3' - read watchpoint (supported)
+       '4' - access watchpoint (supported).  */
+
+  if (type < '2' || type > '4')
+    {
+      /* Unsupported.  */
+      return 1;
+    }
+
+  /* Search for a known watch that matches.  Then unlink and free it.  */
+  watch_type = rsp_bp_type_to_target_hw_bp_type (type);
+  deleted_one = 0;
+  pw = &private->current_watches;
+  while ((w = *pw))
+    {
+      if (w->addr == addr && w->len == len && w->type == watch_type)
+	{
+	  *pw = w->next;
+	  free (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 (private->watch_readback_valid == 1);
+
+  private->watch_mirror = private->watch_readback;
+  mips_linux_watch_populate_regs (private->current_watches,
+				  &private->watch_mirror);
+
+  /* Only update the threads of this process.  */
+  pid = pid_of (proc);
+  find_inferior (&all_lwps, update_watch_registers_callback, &pid);
+  return 0;
+}
+
+/* This is the implementation of linux_target_ops method
+   stopped_by_watchpoint.  The watchhi R and W bits indicate
+   the watch register triggered. */
+
+static int
+mips_stopped_by_watchpoint (void)
+{
+  struct process_info *proc = current_process ();
+  struct arch_process_info *private = proc->private->arch_private;
+  int n;
+  int num_valid;
+  long lwpid = lwpid_of (get_thread_lwp (current_inferior));
+
+  if (!mips_linux_read_watch_registers (lwpid,
+					&private->watch_readback,
+					&private->watch_readback_valid,
+					1))
+    return 0;
+
+  num_valid = mips_linux_watch_get_num_valid (&private->watch_readback);
+
+  for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
+    if (mips_linux_watch_get_watchhi (&private->watch_readback, n)
+	& (R_MASK | W_MASK))
+      return 1;
+
+  return 0;
+}
+
+/* This is the implementation of linux_target_ops method
+   stopped_data_address.  */
+
+static CORE_ADDR
+mips_stopped_data_address (void)
+{
+  struct process_info *proc = current_process ();
+  struct arch_process_info *private = proc->private->arch_private;
+  int n;
+  int num_valid;
+  long lwpid = lwpid_of (get_thread_lwp (current_inferior));
+
+  /* On MIPS we don't know the low order 3 bits of the data address.
+     GDB does not support remote targets that can't report the
+     watchpoint address.  So, make our best guess; return the starting
+     address of a watchpoint request which overlaps the one that
+     triggered.  */
+
+  if (!mips_linux_read_watch_registers (lwpid,
+					&private->watch_readback,
+					&private->watch_readback_valid,
+					0))
+    return 0;
+
+  num_valid = mips_linux_watch_get_num_valid (&private->watch_readback);
+
+  for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
+    if (mips_linux_watch_get_watchhi (&private->watch_readback, n)
+	& (R_MASK | W_MASK))
+      {
+	CORE_ADDR t_low, t_hi;
+	int t_irw;
+	struct mips_watchpoint *watch;
+
+	t_low = mips_linux_watch_get_watchlo (&private->watch_readback, n);
+	t_irw = t_low & IRW_MASK;
+	t_hi = (mips_linux_watch_get_watchhi (&private->watch_readback, n)
+		| IRW_MASK);
+	t_low &= ~(CORE_ADDR)t_hi;
+
+	for (watch = private->current_watches;
+	     watch != NULL;
+	     watch = watch->next)
+	  {
+	    CORE_ADDR addr = watch->addr;
+	    CORE_ADDR last_byte = addr + watch->len - 1;
+
+	    if ((t_irw & mips_linux_watch_type_to_irw (watch->type)) == 0)
+	      {
+		/* Different type.  */
+		continue;
+	      }
+	    /* Check for overlap of even a single byte.  */
+	    if (last_byte >= t_low && addr <= t_low + t_hi)
+	      return addr;
+	  }
+      }
+
+  /* Shouldn't happen.  */
+  return 0;
+}
+
 /* Fetch the thread-local storage pointer for libthread_db.  */
 
 ps_err_e
@@ -506,6 +862,16 @@ struct linux_target_ops the_low_target = {
   mips_reinsert_addr,
   0,
   mips_breakpoint_at,
+  mips_insert_point,
+  mips_remove_point,
+  mips_stopped_by_watchpoint,
+  mips_stopped_data_address,
+  NULL,
+  NULL,
+  NULL, /* siginfo_fixup */
+  mips_linux_new_process,
+  mips_linux_new_thread,
+  mips_linux_prepare_to_resume
 };
 
 void
-- 
1.7.7.6