Hi Samuel,
I initially meant to reply to this to draw your attention to this
patch:
https://sourceware.org/ml/gdb-patches/2014-09/msg00015.html
which, amongst other things, renames a bunch of files and functions
you've referenced to have "x86" prefixes rather than "i386". But
while I was replying I started reviewing your patch. Most of my
corrections are coding standard issues, but don't take that
personally: it happens to us all!
Samuel Thibault wrote:
> 2014-08-31 Samuel Thibault
>
> Add hardware watch support to gnu-i386 platform.
>
> * gdb/gdb/gnu-nat.c (inf_threads): New function.
> * gdb/gdb/gnu-nat.h (inf_threads): New declaration.
> * gdb/gdb/i386gnu-nat.c: Include "i386-nat.h" and "inf-child.h"
> [i386_DEBUG_STATE] (DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL):
> New macros
> [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.
>
> Index: gdb/gdb/gnu-nat.c
> ===
> --- gdb.orig/gdb/gnu-nat.c
> +++ gdb/gdb/gnu-nat.c
> @@ -987,6 +987,16 @@ inf_port_to_thread (struct inf *inf, mac
>return 0;
> }
>
> +/* Interate F over threads. */
"Iterate".
> +void
> +inf_threads (struct inf *inf, void (*f)(struct proc *thread))
Need a space between "(*f)" and "(struct proc *thread)", here and
in gnu-nat.h. Also, because "void (*f) (struct proc *thread)" is
referenced twice it should be made a typedef, inf_threads_ftype or
some such thing.
> +{
> + struct proc *thread;
> +
> + for (thread = inf->threads; thread; thread = thread->next)
> +f(thread);
Should be "f (thread)".
> +}
> +
>
> /* Make INF's list of threads be consistent with reality of TASK. */
> void
> Index: gdb/gdb/gnu-nat.h
> ===
> --- gdb.orig/gdb/gnu-nat.h
> +++ gdb/gdb/gnu-nat.h
> @@ -29,6 +29,9 @@ 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);
>
> +/* Iterate F over threads */
> +void inf_threads (struct inf *inf, void (*f)(struct proc *thread));
As above, space between "(*f)" and "(struct proc *thread)", and the
type of F should be a typedef.
> +#ifndef DR_FIRSTADDR
> +#define DR_FIRSTADDR 0
> +#endif
> +
> +#ifndef DR_LASTADDR
> +#define DR_LASTADDR 3
> +#endif
> +
> +#ifndef DR_STATUS
> +#define DR_STATUS 6
> +#endif
> +
> +#ifndef DR_CONTROL
> +#define DR_CONTROL 7
> +#endif
These four are defined in i386-dregs.h, so this block of code is
unnecessary.
> +/* Get debug registers for thread THREAD. */
> +
> +static void
> +i386_gnu_dr_get(struct i386_debug_state *regs, struct proc *thread)
Need space between function name and opening paren.
Here and numerous other places.
> +{
> + 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 || count != i386_DEBUG_STATE_COUNT)
This should almost certainly be "err != 0".
> +{
> + warning (_("Couldn't fetch debug state from %s"),
> +proc_string (thread));
> + return;
> +}
> +}
The return is unnecessary, and, once removed, the braces should go.
> +/* Set debug registers for thread THREAD. */
> +
> +static void
> +i386_gnu_dr_set(const struct i386_debug_state *regs, struct proc *thread)
Space between function name and opening paren.
> +{
> + error_t err;
> +
> + err = thread_set_state (thread->port, i386_DEBUG_STATE,
> + (thread_state_t) regs, i386_DEBUG_STATE_COUNT);
> + if (err)
> +{
> + warning (_("Couldn't store debug state into %s"),
> +proc_string (thread));
> + return;
> +}
> +}
Again, "err != 0", return unnecessary, braces could go.
> +/* 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(®s, thread);
Space between function name and paren, here and twice below, and also
the same three places in i386_gnu_dr_set_addr below.
> +regs.dr[DR_CONTROL] = control;
> +i386_gnu_dr_set(®s, 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(®s, thread);
> +regs.dr[DR_FIRSTADDR + regnum] = addr;
