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/