[PATCHv2,Hurd] Add hardware watch support

[PATCHv2,Hurd] Add hardware watch support

[Samuel Thibault]

2014-09-06  Samuel Thibault  <samuel.thibault@ens-lyon.org>

	Add hardware watch support to gnu-i386 platform.

	* gdb/gdb/gnu-nat.c (inf_threads): New function.
	* gdb/gdb/gnu-nat.h (inf_threads_ftype): New type.
	(inf_threads): New declaration.
	* gdb/gdb/i386gnu-nat.c: Include "x86-nat.h" and "inf-child.h"
        [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,
	i386_gnu_dr_set_control, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg,
	i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control):
	New functions
	[i386_DEBUG_STATE] (_initialize_i386gnu_nat): Initialize hardware i386
	debugging register hooks.

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index c8164d6..2d7c32c 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -983,6 +983,16 @@ inf_port_to_thread (struct inf *inf, mach_port_t port)
   return 0;
 }
 
+/* Iterate F over threads.  */
+void
+inf_threads (struct inf *inf, inf_threads_ftype *f)
+{
+  struct proc *thread;
+
+  for (thread = inf->threads; thread; thread = thread->next)
+    f (thread);
+}
+
 \f
 /* Make INF's list of threads be consistent with reality of TASK.  */
 void
diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
index 8e949eb..011c38c 100644
--- a/gdb/gnu-nat.h
+++ b/gdb/gnu-nat.h
@@ -29,6 +29,11 @@ extern struct inf *gnu_current_inf;
 /* Converts a GDB pid to a struct proc.  */
 struct proc *inf_tid_to_thread (struct inf *inf, int tid);
 
+typedef void (inf_threads_ftype) (struct proc *thread);
+
+/* Iterate F over threads.  */
+void inf_threads (struct inf *inf, inf_threads_ftype *f);
+
 /* Makes sure that INF's thread list is synced with the actual process.  */
 int inf_update_procs (struct inf *inf);
 
diff --git a/gdb/i386gnu-nat.c b/gdb/i386gnu-nat.c
index 8fad871..5654e9a 100644
--- a/gdb/i386gnu-nat.c
+++ b/gdb/i386gnu-nat.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "x86-nat.h"
 #include "inferior.h"
 #include "floatformat.h"
 #include "regcache.h"
@@ -30,6 +31,7 @@
 #include "i386-tdep.h"
 
 #include "gnu-nat.h"
+#include "inf-child.h"
 #include "i387-tdep.h"
 
 #ifdef HAVE_SYS_PROCFS_H
@@ -304,6 +306,119 @@ gnu_store_registers (struct target_ops *ops,
     }
 }
 
+\f
+/* Support for debug registers.  */
+
+#ifdef i386_DEBUG_STATE
+/* Get debug registers for thread THREAD.  */
+
+static void
+i386_gnu_dr_get (struct i386_debug_state *regs, struct proc *thread)
+{
+  mach_msg_type_number_t count = i386_DEBUG_STATE_COUNT;
+  error_t err;
+
+  err = thread_get_state (thread->port, i386_DEBUG_STATE,
+ 			  (thread_state_t) regs, &count);
+  if (err != 0 || count != i386_DEBUG_STATE_COUNT)
+    warning (_("Couldn't fetch debug state from %s"),
+	     proc_string (thread));
+}
+
+/* Set debug registers for thread THREAD.  */
+
+static void
+i386_gnu_dr_set (const struct i386_debug_state *regs, struct proc *thread)
+{
+  error_t err;
+
+  err = thread_set_state (thread->port, i386_DEBUG_STATE,
+			  (thread_state_t) regs, i386_DEBUG_STATE_COUNT);
+  if (err != 0)
+    warning (_("Couldn't store debug state into %s"),
+	     proc_string (thread));
+}
+
+/* Set DR_CONTROL to ADDR in all threads.  */
+
+static void
+i386_gnu_dr_set_control (unsigned long control)
+{
+  void f (struct proc *thread)
+  {
+    struct i386_debug_state regs;
+    i386_gnu_dr_get (&regs, thread);
+    regs.dr[DR_CONTROL] = control;
+    i386_gnu_dr_set (&regs, thread);
+  }
+
+  inf_update_procs (gnu_current_inf);
+  inf_threads (gnu_current_inf, f);
+}
+
+/* Set address REGNUM (zero based) to ADDR in all threads.  */
+
+static void
+i386_gnu_dr_set_addr (int regnum, CORE_ADDR addr)
+{
+  void f (struct proc *thread)
+  {
+    struct i386_debug_state regs;
+    i386_gnu_dr_get (&regs, thread);
+    regs.dr[regnum] = addr;
+    i386_gnu_dr_set (&regs, thread);
+  }
+
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
+
+  inf_update_procs (gnu_current_inf);
+  inf_threads (gnu_current_inf, f);
+}
+
+/* Get debug register REGNUM value from only the one LWP of PTID.  */
+
+static unsigned long
+i386_gnu_dr_get_reg (ptid_t ptid, int regnum)
+{
+  struct i386_debug_state regs;
+  struct proc *thread;
+
+  /* Make sure we know about new threads.  */
+  inf_update_procs (gnu_current_inf);
+
+  thread = inf_tid_to_thread (gnu_current_inf, ptid_get_lwp (ptid));
+  i386_gnu_dr_get (&regs, thread);
+
+  return regs.dr[regnum];
+}
+
+/* Return the inferior's debug register REGNUM.  */
+
+static CORE_ADDR
+i386_gnu_dr_get_addr (int regnum)
+{
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
+
+  return i386_gnu_dr_get_reg (inferior_ptid, regnum);
+}
+
+/* Get DR_STATUS from only the one thread of INFERIOR_PTID.  */
+
+static unsigned long
+i386_gnu_dr_get_status (void)
+{
+  return i386_gnu_dr_get_reg (inferior_ptid, DR_STATUS);
+}
+
+/* Return the inferior's DR7 debug control register.  */
+
+static unsigned long
+i386_gnu_dr_get_control (void)
+{
+  return i386_gnu_dr_get_reg (inferior_ptid, DR_CONTROL);
+}
+#endif /* i386_DEBUG_STATE */
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_i386gnu_nat;
 
@@ -315,6 +430,18 @@ _initialize_i386gnu_nat (void)
   /* Fill in the generic GNU/Hurd methods.  */
   t = gnu_target ();
 
+#ifdef i386_DEBUG_STATE
+  x86_use_watchpoints (t);
+
+  x86_dr_low.set_control = i386_gnu_dr_set_control;
+  gdb_assert (DR_FIRSTADDR == 0 && DR_LASTADDR < i386_DEBUG_STATE_COUNT);
+  x86_dr_low.set_addr = i386_gnu_dr_set_addr;
+  x86_dr_low.get_addr = i386_gnu_dr_get_addr;
+  x86_dr_low.get_status = i386_gnu_dr_get_status;
+  x86_dr_low.get_control = i386_gnu_dr_get_control;
+  x86_set_debug_register_length (4);
+#endif /* i386_DEBUG_STATE */
+
   t->to_fetch_registers = gnu_fetch_registers;
   t->to_store_registers = gnu_store_registers;
 
diff --git a/gdb/config/i386/i386gnu.mh b/gdb/config/i386/i386gnu.mh
index a3ea122..9d76b59 100644
--- a/gdb/config/i386/i386gnu.mh
+++ b/gdb/config/i386/i386gnu.mh
@@ -1,5 +1,5 @@
 # Host: Intel 386 running the GNU Hurd
-NATDEPFILES= i386gnu-nat.o gnu-nat.o core-regset.o fork-child.o \
+NATDEPFILES= i386gnu-nat.o gnu-nat.o x86-nat.o core-regset.o fork-child.o \
 	     notify_S.o process_reply_S.o msg_reply_S.o \
 	     msg_U.o exc_request_U.o exc_request_S.o
 HAVE_NATIVE_GCORE_HOST = 1

Re: [PATCHv2,Hurd] Add hardware watch support

[Joel Brobecker]

Samuel,

I only have small comments.

First, the ChangeLog entry:

> 2014-09-06  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	Add hardware watch support to gnu-i386 platform.
> 
> 	* gdb/gdb/gnu-nat.c (inf_threads): New function.
> 	* gdb/gdb/gnu-nat.h (inf_threads_ftype): New type.
> 	(inf_threads): New declaration.
> 	* gdb/gdb/i386gnu-nat.c: Include "x86-nat.h" and "inf-child.h"

Missing period at the end of this sentence.

>         [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,

It looks like this is improperly indented (spaces instead of tabs?).

> 	i386_gnu_dr_set_control, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg,
> 	i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control):
> 	New functions
> 	[i386_DEBUG_STATE] (_initialize_i386gnu_nat): Initialize hardware i386

I would swap the i386_DEBUG_STATE and _initialize_i386gnu_nat.
You're inside _initialize_i386gnu_nat, not the other around.

> 	debugging register hooks.

> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index c8164d6..2d7c32c 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -983,6 +983,16 @@ inf_port_to_thread (struct inf *inf, mach_port_t port)
>    return 0;
>  }
>  
> +/* Iterate F over threads.  */

Use...

/* See gnu-nat.h.  */

... instead.  This is a fairly trivial comment in this case, so
not biggie, but the idea is that want to avoid duplicating
documentation in order to avoid having one of them being out
of sync.

And please also make sure to always have an empty line before
function documentation and start of implementation.

>  void
> diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
> index 8e949eb..011c38c 100644
> --- a/gdb/gnu-nat.h
> +++ b/gdb/gnu-nat.h
> @@ -29,6 +29,11 @@ extern struct inf *gnu_current_inf;
>  /* Converts a GDB pid to a struct proc.  */
>  struct proc *inf_tid_to_thread (struct inf *inf, int tid);
>  
> +typedef void (inf_threads_ftype) (struct proc *thread);
> +
> +/* Iterate F over threads.  */

Suggest:

/* Call F for every thread in inferior INF.  */

This addresses two issues: "Iterate F" sounds odd, but perhaps
that's because English is not my native language; but also,
it also documents what INF is.

> +/* Set DR_CONTROL to ADDR in all threads.  */
> +
> +static void
> +i386_gnu_dr_set_control (unsigned long control)

The documentation seems out of sync with the function's prototype.

> +{
> +  void f (struct proc *thread)
> +  {
> +    struct i386_debug_state regs;
> +    i386_gnu_dr_get (&regs, thread);
> +    regs.dr[DR_CONTROL] = control;
> +    i386_gnu_dr_set (&regs, thread);
> +  }

Nested functions are not allowed. We'd like GCC to remain
compilable using non-GCC C90 compilers.

> +/* Set address REGNUM (zero based) to ADDR in all threads.  */
> +
> +static void
> +i386_gnu_dr_set_addr (int regnum, CORE_ADDR addr)
> +{
> +  void f (struct proc *thread)
> +  {
> +    struct i386_debug_state regs;
> +    i386_gnu_dr_get (&regs, thread);
> +    regs.dr[regnum] = addr;
> +    i386_gnu_dr_set (&regs, thread);
> +  }

Same here, no nested function, please.


-- 
Joel