[PATCHv3,Hurd] Add hardware watch support

[PATCHv3,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_one, i386_gnu_dr_set_control,
	i386_gnu_dr_set_addr_one, 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
	(reg_addr): New structure.
	(_initialize_i386gnu_nat) [i386_DEBUG_STATE]: 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,130 @@ 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));
+}
+
+static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg)
+{
+  unsigned long *control = arg;
+  struct i386_debug_state regs;
+  i386_gnu_dr_get (&regs, thread);
+  regs.dr[DR_CONTROL] = *control;
+  i386_gnu_dr_set (&regs, thread);
+}
+
+/* Set DR_CONTROL to CONTROL in all threads.  */
+
+static void
+i386_gnu_dr_set_control (unsigned long control)
+{
+  inf_update_procs (gnu_current_inf);
+  inf_threads (gnu_current_inf, i386_gnu_dr_set_control_one, &control);
+}
+
+struct reg_addr {
+  int regnum;
+  CORE_ADDR addr;
+};
+
+static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg)
+{
+  struct reg_addr *reg_addr = arg;
+  struct i386_debug_state regs;
+  i386_gnu_dr_get (&regs, thread);
+  regs.dr[reg_addr->regnum] = reg_addr->addr;
+  i386_gnu_dr_set (&regs, thread);
+}
+
+/* Set address REGNUM (zero based) to ADDR in all threads.  */
+
+static void
+i386_gnu_dr_set_addr (int regnum, CORE_ADDR addr)
+{
+  struct reg_addr reg_addr;
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
+
+  reg_addr.regnum = regnum;
+  reg_addr.addr = addr;
+
+  inf_update_procs (gnu_current_inf);
+  inf_threads (gnu_current_inf, i386_gnu_dr_set_addr_one, &reg_addr);
+}
+
+/* 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: [PATCHv3,Hurd] Add hardware watch support

[Sergio Durigan Junior]

On Wednesday, September 10 2014, Samuel Thibault wrote:

> 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_one, i386_gnu_dr_set_control,
> 	i386_gnu_dr_set_addr_one, 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
> 	(reg_addr): New structure.
> 	(_initialize_i386gnu_nat) [i386_DEBUG_STATE]: Initialize hardware i386
> 	debugging register hooks.

Thanks for the patch, Samuel.  What do you think of writing a message
explaining a bit more of this feature, for the sake of putting it in the
commit message?  Just thinking aloud here...

I only have comments about style and organization of the code.

> 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
[...]
> +static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg)
> +{
> +  unsigned long *control = arg;
> +  struct i386_debug_state regs;
> +  i386_gnu_dr_get (&regs, thread);
> +  regs.dr[DR_CONTROL] = *control;
> +  i386_gnu_dr_set (&regs, thread);
> +}

This function needs a comment, and the organization is a little messy.
Could you put a newline after the declaration of the variables?

[...]
> +struct reg_addr {
> +  int regnum;
> +  CORE_ADDR addr;
> +};

This structure also needs a comment, and we put a comment on each struct
field as well.

> +static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg)
> +{
> +  struct reg_addr *reg_addr = arg;
> +  struct i386_debug_state regs;
> +  i386_gnu_dr_get (&regs, thread);
> +  regs.dr[reg_addr->regnum] = reg_addr->addr;
> +  i386_gnu_dr_set (&regs, thread);
> +}

Same comment from the function above: missing a comment, and newline
after var declaration.

Is it possible to submit a testcase for this as well?  WDYT?

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCHv3,Hurd] Add hardware watch support

[Joel Brobecker]

> 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_one, i386_gnu_dr_set_control,
> 	i386_gnu_dr_set_addr_one, 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
> 	(reg_addr): New structure.
> 	(_initialize_i386gnu_nat) [i386_DEBUG_STATE]: Initialize hardware i386
> 	debugging register hooks.

In addition to Sergio's comments, I noticed:

You forgot the comment I made about having the function documented
at only one location, and the contents of that documentat. For easy
reference, here are my comments again:

    | 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.

> +static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg)
> +{

For the function implementations, the name of the function should
be at the first column on the next line. This is a GNU Coding Style
(GCS) requirement, and allows easy searching of a function's body by
grepping for "^FUNCTION_NAME". I also think it helps having the name
of the function for each hunk in "diff -p" (not completely sure about
that one).

> +  unsigned long *control = arg;
> +  struct i386_debug_state regs;
> +  i386_gnu_dr_get (&regs, thread);

I just wanted to add that Sergio's request to add an empty line after
the variable declarations is actually a rule in the GDB project.
For the record, our coding standards are documented at:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
It's not complete yet, so do not be too surprise if we come up with
things that you haven't seen there or in the GCS. Do call us on it,
though, and we will update the doc.

> +struct reg_addr {
> +  int regnum;
> +  CORE_ADDR addr;
> +};

While touching this code, we tend to prefer it for struct
definitions if the opening curly brace is at the start of
the next line, please.

        struct reg_addr
        {
          int regnum;
          CORE_ADDR addr;
        };


> +static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg)

Same as above.

> +  struct reg_addr reg_addr;
> +  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);

Missing empty line between var declaration and rest of code.


-- 
Joel

Re: [PATCHv3,Hurd] Add hardware watch support

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

Hi Samuel!

Many thanks for persisting with this patch.  The GDB testsuite shows a
pretty good improvement!  I'll try to assess the remaining issues, but
From a functional point of view that patch is much of an improvement
already.

Unless you have push access to sourceware (do you?), I'll be happy to
push this for you once the pending review comments have been addressed.
I have just to contribute a small patch to add on top of yours (merge it
in) to make it actually build:

diff --git gdb/config/i386/i386gnu.mh gdb/config/i386/i386gnu.mh
index 9d76b59..4cc23e4 100644
--- gdb/config/i386/i386gnu.mh
+++ gdb/config/i386/i386gnu.mh
@@ -1,5 +1,6 @@
 # Host: Intel 386 running the GNU Hurd
-NATDEPFILES= i386gnu-nat.o gnu-nat.o x86-nat.o core-regset.o fork-child.o \
+NATDEPFILES= i386gnu-nat.o gnu-nat.o \
+	     x86-nat.o x86-dregs.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
diff --git gdb/gnu-nat.c gdb/gnu-nat.c
index 2d7c32c..7c6bc42 100644
--- gdb/gnu-nat.c
+++ gdb/gnu-nat.c
@@ -985,12 +985,12 @@ inf_port_to_thread (struct inf *inf, mach_port_t port)
 
 /* Iterate F over threads.  */
 void
-inf_threads (struct inf *inf, inf_threads_ftype *f)
+inf_threads (struct inf *inf, inf_threads_ftype *f, void *arg)
 {
   struct proc *thread;
 
   for (thread = inf->threads; thread; thread = thread->next)
-    f (thread);
+    f (thread, arg);
 }
 
 \f
diff --git gdb/gnu-nat.h gdb/gnu-nat.h
index 011c38c..39a613d 100644
--- gdb/gnu-nat.h
+++ gdb/gnu-nat.h
@@ -29,10 +29,10 @@ 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);
+typedef void (inf_threads_ftype) (struct proc *thread, void *arg);
 
 /* Iterate F over threads.  */
-void inf_threads (struct inf *inf, inf_threads_ftype *f);
+void inf_threads (struct inf *inf, inf_threads_ftype *f, void *arg);
 
 /* Makes sure that INF's thread list is synced with the actual process.  */
 int inf_update_procs (struct inf *inf);


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Thomas Schwinge, le Fri 12 Sep 2014 19:56:13 +0200, a écrit :
> Unless you have push access to sourceware (do you?), I'll be happy to
> push this for you once the pending review comments have been addressed.
> I have just to contribute a small patch to add on top of yours (merge it
> in) to make it actually build:
> -inf_threads (struct inf *inf, inf_threads_ftype *f)
> +inf_threads (struct inf *inf, inf_threads_ftype *f, void *arg)

Ugh?  I guess with having to deal with various trees, some building and
others not, I just manage to mangle that patch.

> Many thanks for persisting with this patch.

I have to say I'm almost about to give up with submitting it.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Sergio Durigan Junior, le Wed 10 Sep 2014 19:22:56 -0400, a écrit :
> Thanks for the patch, Samuel.  What do you think of writing a message
> explaining a bit more of this feature, for the sake of putting it in the
> commit message?  Just thinking aloud here...

Well, hardware watchpoints are fully documented in info.

> Is it possible to submit a testcase for this as well?  WDYT?

AIUI, gdb already has them.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Joel Brobecker, le Fri 12 Sep 2014 09:51:49 -0700, a écrit :
> You forgot the comment I made about having the function documented
> at only one location, and the contents of that documentat.

I didn't forgot, but apparently I completely mangled my patch, sorry
about that.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Thomas Schwinge, le Fri 12 Sep 2014 19:56:13 +0200, a écrit :
> Unless you have push access to sourceware (do you?),

I don't.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Sergio Durigan Junior]

On Friday, September 12 2014, Samuel Thibault wrote:

> Well, hardware watchpoints are fully documented in info.

Yes, but I am not talking about writing documentation for GDB, I am
talking about writing a commit message :-).

>> Is it possible to submit a testcase for this as well?  WDYT?
>
> AIUI, gdb already has them.

You are right, sorry for nonsense request.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCHv3,Hurd] Add hardware watch support

[Joel Brobecker]

> > Many thanks for persisting with this patch.
> 
> I have to say I'm almost about to give up with submitting it.

I would be very interested in hearing your honest feedback on this.
I know it took a long time, and we're not always very responsive,
but we try our best. If we could hear what made you feel this way,
I would like to try to see if there are any ways we can improve
the situation for you.

-- 
Joel

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Joel Brobecker, le Fri 12 Sep 2014 13:01:41 -0700, a écrit :
> > > Many thanks for persisting with this patch.
> > 
> > I have to say I'm almost about to give up with submitting it.
> 
> I would be very interested in hearing your honest feedback on this.
> I know it took a long time, and we're not always very responsive,
> but we try our best. If we could hear what made you feel this way,
> I would like to try to see if there are any ways we can improve
> the situation for you.

I'm sorry I wrote it in such a harsh way while you were all polite in
your requests. Actually it's an unfortunate combination of me also
having a pending patch to the Linux kernel which has been waiting for
several years, with only sporadic reviews, then a request for reshaping,
and ended up with a "well, the way it is done will not fly" without
very much details or discussion, and another pending patch to qemu,
which apparently nobody has the time to review, while they are really
interested in it. Seeing a submission to gdb getting stuck on missing
line breaks got me a bit on my nerves. Fortunately it wasn't only
about that, but also important changes, so I carried on, but having to
resubmit only for missing spaces or new lines would have been really
hard to me, since I don't really plan to submit many patches to gdb,
and I know I'll always make this kind of mistakes since I'm submitting
patches to a lot of various projects with very differing coding styles.

What may help in the process would be to have a script which checks for
style. The Linux kernel's checkpatch.pl is a very good approach, since
one can get to check one's own style quite thoroughly before submitting.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Sergio Durigan Junior]

On Friday, September 12 2014, Samuel Thibault wrote:

> What may help in the process would be to have a script which checks for
> style. The Linux kernel's checkpatch.pl is a very good approach, since
> one can get to check one's own style quite thoroughly before submitting.

This has been discussed a few times at least, but so far we haven't done
much about the subject.  I will try to dedicate some time this weekend
to see if I can come up with an initial attempt to do those checkings
automatically.

I am sorry about your difficulties (and I admit I helped when I dumbly
asked for a testcase), and I am glad that your patch made it.

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCHv3,Hurd] Add hardware watch support

[Joel Brobecker]

> Seeing a submission to gdb getting stuck on missing line breaks got me
> a bit on my nerves. Fortunately it wasn't only about that, but also
> important changes, so I carried on, but having to resubmit only for
> missing spaces or new lines would have been really hard to me, since I
> don't really plan to submit many patches to gdb, and I know I'll
> always make this kind of mistakes since I'm submitting patches to a
> lot of various projects with very differing coding styles.

I understand the fustration, and I certainly would agree if you said
that GDB is very particular with its CS. Aiming for a consistent coding
style is important for long-term maintainability, though, which is why
we almost systematically mention violations we see. If it makes you feel
any better, we make mistakes too, sometimes, and have to re-edit as
a result.  One thing I will say, however, is that I often let go of
minor violations, and only mention them if there are other adjustments
I'd like to request in the same area. And I know others can be the same.
So we try not be be overly picky (you may not believe me on that one ;-)!).

As Sergio mentioned, we do not have a script we can use to check
coding-style violations.  We would very much like to have one,
but no one really sent anything so far. What I can do to help is
being your CS corrector - just send me your patch privately before
submitting, and I will fix all CS violations for you.

Another important part I can see contributing to the fustration
is review delays. I can see how trivial requests can be extra
fustrating if you feel like your patch is being delayed quite
a bit because each review cycle is very long. You can help with
that by pinging us. The accepted practice is to ping after a week
or two, and every week thereafter.

-- 
Joel

Re: [PATCHv3,Hurd] Add hardware watch support

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

Hi Samuel!

On Fri, 12 Sep 2014 19:56:13 +0200, I wrote:
> Many thanks for persisting with this patch.  The GDB testsuite shows a
> pretty good improvement!  I'll try to assess the remaining issues

From a quick scan through the »FAIL:.*watch« matches, the following one
issue should be relevant for a lot of the regressions: given, for
example, the gdb.base/pr11022 test case, on GNU/Linux we see:

    $ gcc [...]/gdb/testsuite/gdb.base/pr11022.c -g -o pr11022
    $ ./gdb -q -ex 'watch x' -ex run pr11022 
    [...]
    Hardware watchpoint 1: x
    
    Old value = 0
    New value = 42
    main () at ../../W._C._Handy/gdb/testsuite/gdb.base/pr11022.c:28
    28          j = i;  /* expect HW watchpoint stop */

... whereas on GNU Hurd we see:

    [...]
    Program received signal SIGTRAP, Trace/breakpoint trap.
    main () at ../../../W._C._Handy/gdb/testsuite/gdb.base/pr11022.c:28
    28          j = i;  /* expect HW watchpoint stop */

You may want to prepend »-ex 'maintenance set show-debug-regs on'« to see
some debugging output (and notice that »watchpoint_hit« is missing on GNU
Hurd).

I tracked this down to gdb/nat/x86-dregs.c:x86_dr_stopped_data_address
and x86_dr_stopped_by_watchpoint not returning the information as
desired, and tracked that in turn down to our kernel interface always
returning zero for DR6, the debug status register.  See the comment in
x86_dr_stopped_data_address for how GDB is using this information.  Do
you agree that thread_get_state(i386_DEBUG_STATE) should be returning the
actual DR6, and where in GNU Mach would we need to copy the DR6 register
into the PCB?


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Thomas Schwinge, le Tue 16 Sep 2014 00:08:01 +0200, a écrit :
> Do you agree that thread_get_state(i386_DEBUG_STATE) should be
> returning the actual DR6,

Indeed.

> and where in GNU Mach would we need to copy the DR6
> register into the PCB?

it would be user_trap(), probably, in the T_DEBUG case.

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

Hi Samuel!

On Tue, 16 Sep 2014 01:09:50 +0200, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Thomas Schwinge, le Tue 16 Sep 2014 00:08:01 +0200, a écrit :
> > Do you agree that thread_get_state(i386_DEBUG_STATE) should be
> > returning the actual DR6,
> 
> Indeed.
> 
> > and where in GNU Mach would we need to copy the DR6
> > register into the PCB?
> 
> it would be user_trap(), probably, in the T_DEBUG case.

Thanks for the pointer.  Something like the following does accomplish its
task w.r.t. GDB, but there are some TODO items.  It might help to compare
what the Linux kernel is doing; »git grep --cached -i dr6 -- arch/x86/«
or similar.

diff -ru gnumach-1.4.orig/i386/i386/trap.c gnumach-1.4/i386/i386/trap.c
--- gnumach-1.4.orig/i386/i386/trap.c	2013-09-27 08:05:57.000000000 +0200
+++ gnumach-1.4/i386/i386/trap.c	2014-09-16 10:45:58.000000000 +0200
@@ -404,6 +404,17 @@
 			return 0;
 		}
 #endif
+
+		/* Make the content of the debug status register (DR6)
+		   available to user space.  */
+		/* TODO: Do we have to sanitize its content?  (Mask out
+		   reserved bits?)  */
+		/* TODO: Where should its content be reset (zeroed)?  From user
+		   space?  */
+		/* TODO: Anything to take care about w.r.t interaction with
+		   KDB?  */
+		thread->pcb->ims.ids.dr[6] = get_dr6();
+
 		exc = EXC_BREAKPOINT;
 		code = EXC_I386_SGL;
 		break;


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Hello,

Thomas Schwinge, le Tue 16 Sep 2014 10:59:47 +0200, a écrit :
> On Tue, 16 Sep 2014 01:09:50 +0200, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Thomas Schwinge, le Tue 16 Sep 2014 00:08:01 +0200, a écrit :
> > > Do you agree that thread_get_state(i386_DEBUG_STATE) should be
> > > returning the actual DR6,
> > 
> > Indeed.
> > 
> > > and where in GNU Mach would we need to copy the DR6
> > > register into the PCB?
> > 
> > it would be user_trap(), probably, in the T_DEBUG case.
> 
> Thanks for the pointer.  Something like the following does accomplish its
> task w.r.t. GDB, but there are some TODO items.  It might help to compare
> what the Linux kernel is doing; »git grep --cached -i dr6 -- arch/x86/«
> or similar.

I believe this will be fine to only expose the known-to-be-safe
information, and clean dr6:

diff --git a/i386/i386/trap.c b/i386/i386/trap.c
index 200cbcc..661bc6a 100644
--- a/i386/i386/trap.c
+++ b/i386/i386/trap.c
@@ -395,6 +395,10 @@ printf("user trap %d error %d sub %08x\n", type, code, subcode);
 			return 0;
 		}
 #endif /* MACH_KDB */
+		/* Make the content of the debug status register (DR6)
+		   available to user space.  */
+		thread->pcb->ims.ids.dr[6] = get_dr6() & 0x600F;
+		set_dr6(0);
 		exc = EXC_BREAKPOINT;
 		code = EXC_I386_SGL;
 		break;

Does it fix GDB too?

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Samuel Thibault]

Samuel Thibault, le Wed 17 Sep 2014 01:17:06 +0200, a écrit :
> I believe this will be fine to only expose the known-to-be-safe
> information, and clean dr6:

(this is mostly what Linux is doing)

Samuel

Re: [PATCHv3,Hurd] Add hardware watch support

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Hi Samuel!

On Wed, 17 Sep 2014 01:17:06 +0200, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Thomas Schwinge, le Tue 16 Sep 2014 10:59:47 +0200, a écrit :
> > On Tue, 16 Sep 2014 01:09:50 +0200, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > > Thomas Schwinge, le Tue 16 Sep 2014 00:08:01 +0200, a écrit :
> > > > Do you agree that thread_get_state(i386_DEBUG_STATE) should be
> > > > returning the actual DR6,

> --- a/i386/i386/trap.c
> +++ b/i386/i386/trap.c
> @@ -395,6 +395,10 @@ printf("user trap %d error %d sub %08x\n", type, code, subcode);
>  			return 0;
>  		}
>  #endif /* MACH_KDB */
> +		/* Make the content of the debug status register (DR6)
> +		   available to user space.  */
> +		thread->pcb->ims.ids.dr[6] = get_dr6() & 0x600F;
> +		set_dr6(0);
>  		exc = EXC_BREAKPOINT;
>  		code = EXC_I386_SGL;
>  		break;
> 
> Does it fix GDB too?

Yes.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]