This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCHv3,Hurd] Add hardware watch support

On Wednesday, September 10 2014, Samuel Thibault wrote:

> 2014-09-06  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_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?


GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]