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