Re: [PATCHv3,Hurd] Add hardware watch support

Sergio Durigan Junior
2014-09-10T23:23:06Z

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