[PATCHv3,Hurd] Add hardware watch support
Commit Message
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.
Comments
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 (®s, thread);
> + regs.dr[DR_CONTROL] = *control;
> + i386_gnu_dr_set (®s, 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 (®s, thread);
> + regs.dr[reg_addr->regnum] = reg_addr->addr;
> + i386_gnu_dr_set (®s, 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,
> 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 (®s, 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.
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
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
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.
@@ -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);
+}
+
/* Make INF's list of threads be consistent with reality of TASK. */
void
@@ -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);
@@ -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,
}
}
+
+/* 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 (®s, thread);
+ regs.dr[DR_CONTROL] = *control;
+ i386_gnu_dr_set (®s, 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 (®s, thread);
+ regs.dr[reg_addr->regnum] = reg_addr->addr;
+ i386_gnu_dr_set (®s, 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, ®_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 (®s, 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;
@@ -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