[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCHv3,Hurd] Add hardware watch support
From: |
Sergio Durigan Junior |
Subject: |
Re: [PATCHv3,Hurd] Add hardware watch support |
Date: |
Wed, 10 Sep 2014 19:22:56 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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 (®s, thread);
> + regs.dr[DR_CONTROL] = *control;
> + i386_gnu_dr_set (®s, 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 (®s, thread);
> + regs.dr[reg_addr->regnum] = reg_addr->addr;
> + i386_gnu_dr_set (®s, 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/
- [PATCHv3,Hurd] Add hardware watch support, Samuel Thibault, 2014/09/10
- Re: [PATCHv3,Hurd] Add hardware watch support,
Sergio Durigan Junior <=
- Re: [PATCHv3,Hurd] Add hardware watch support, Joel Brobecker, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Thomas Schwinge, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Samuel Thibault, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Joel Brobecker, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Samuel Thibault, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Sergio Durigan Junior, 2014/09/12
- Re: [PATCHv3,Hurd] Add hardware watch support, Joel Brobecker, 2014/09/15