Re: [PATCHv3,Hurd] Add hardware watch support

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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/