[PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[Yufeng Zhang]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Hi,

This patch fixes an AArch64 GDB bug in handling the hardware debug 
registers.  GDB calls ptrace to set hardware debug registers and it 
passes a full-length "struct user_hwdebug_state" variable regardless of 
the number of hardware debug registers available on a target.  When 
there are fewer than 16 (the maximum number) hardware 
breakpoint/watchpoint registers on a target, the kernel will complain 
about the gdb's request to set non-existing hardware debug registers. 
There will be an warning of "Unexpected error setting hardware debug 
registers" when the inferior starts to run.

This patch fixes the issue by setting iov.iov_len with a value 
reflecting the exact size in use.

OK for the mainline?

Thanks.
Yufeng


gdb/

         * aarch64-linux-nat.c (aarch64_linux_set_debug_regs): Set
	iov.iov_len with the real length in use.

gdb/gdbserver/

         * linux-aarch64-low.c (aarch64_linux_set_debug_regs): Set
	iov.iov_len with the real length in use.

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1532 bytes --]

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 256725b..7d76833 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -314,10 +314,13 @@ aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
 
   memset (&regs, 0, sizeof (regs));
   iov.iov_base = &regs;
-  iov.iov_len = sizeof (regs);
   count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;
   addr = watchpoint ? state->dr_addr_wp : state->dr_addr_bp;
   ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
+  if (count == 0)
+    return;
+  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
+		 + sizeof (regs.dbg_regs [count - 1]));
 
   for (i = 0; i < count; i++)
     {
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 93246b3..c2d271a 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -602,10 +602,13 @@ aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
 
   memset (&regs, 0, sizeof (regs));
   iov.iov_base = &regs;
-  iov.iov_len = sizeof (regs);
   count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;
   addr = watchpoint ? state->dr_addr_wp : state->dr_addr_bp;
   ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
+  if (count == 0)
+    return;
+  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
+		 + sizeof (regs.dbg_regs [count - 1]));
 
   for (i = 0; i < count; i++)
     {

Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[Pedro Alves]

On 12/18/2013 04:07 PM, Yufeng Zhang wrote:

> This patch fixes an AArch64 GDB bug in handling the hardware debug 
> registers.  GDB calls ptrace to set hardware debug registers and it 
> passes a full-length "struct user_hwdebug_state" variable regardless of 
> the number of hardware debug registers available on a target.  When 
> there are fewer than 16 (the maximum number) hardware 
> breakpoint/watchpoint registers on a target, the kernel will complain 
> about the gdb's request to set non-existing hardware debug registers. 
> There will be an warning of "Unexpected error setting hardware debug 
> registers" when the inferior starts to run.
> 
> This patch fixes the issue by setting iov.iov_len with a value 
> reflecting the exact size in use.
> 
> OK for the mainline?

OK.

Thanks,
-- 
Pedro Alves

Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[pinskia]

> On Dec 18, 2013, at 8:07 AM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote:
> 
> Hi,
> 
> This patch fixes an AArch64 GDB bug in handling the hardware debug registers.  GDB calls ptrace to set hardware debug registers and it passes a full-length "struct user_hwdebug_state" variable regardless of the number of hardware debug registers available on a target.  When there are fewer than 16 (the maximum number) hardware breakpoint/watchpoint registers on a target, the kernel will complain about the gdb's request to set non-existing hardware debug registers. There will be an warning of "Unexpected error setting hardware debug registers" when the inferior starts to run.
> 
> This patch fixes the issue by setting iov.iov_len with a value reflecting the exact size in use.
> 
> OK for the mainline?

I think this patch is wrong as the size that is passed is always just one element as sizeof (regs.dbg_regs [count - 1]) is the same as sizeof (regs.dbg_regs [0]). This should have been sizeof (regs.dbg_regs [0])*count instead. 

Thanks,
Andrew Pinski

> 
> Thanks.
> Yufeng
> 
> 
> gdb/
> 
>       * aarch64-linux-nat.c (aarch64_linux_set_debug_regs): Set
>   iov.iov_len with the real length in use.
> 
> gdb/gdbserver/
> 
>       * linux-aarch64-low.c (aarch64_linux_set_debug_regs): Set
>   iov.iov_len with the real length in use.
> <patch>

Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[Marcus Shawcroft]

On 19 December 2013 07:00,  <pinskia@gmail.com> wrote:
> I think this patch is wrong as the size that is passed is always just one element as sizeof (regs.dbg_regs [count - 1]) is the same as sizeof (regs.dbg_regs [0]). This should have been sizeof (regs.dbg_regs [0])*count instead.


+  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
+ + sizeof (regs.dbg_regs [count - 1]));

The offsetof() gives the number of bytes from the start of
user_hwdebug_state upto and including all of the reg array bar the
last in use entry.  Adding on the sizeof() therefore gives the number
of bytes from the start of the structure upto and including the last
in use reg entry.

I agree that the sizeof() could be written either with dbg_regs[0] or
dbg_regs[count-1] with no change in behavior.  But I think Yufengs
code is functionally correct.

It would have been simpler to write:
 iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count])

.. but I think that is illegal when count equals the number of
elements defined in the array.

Cheers
/Marcus

Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[pinskia]

> On Dec 19, 2013, at 2:16 AM, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote:
> 
>> On 19 December 2013 07:00,  <pinskia@gmail.com> wrote:
>> I think this patch is wrong as the size that is passed is always just one element as sizeof (regs.dbg_regs [count - 1]) is the same as sizeof (regs.dbg_regs [0]). This should have been sizeof (regs.dbg_regs [0])*count instead.
> 
> 
> +  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
> + + sizeof (regs.dbg_regs [count - 1]));
> 
> The offsetof() gives the number of bytes from the start of
> user_hwdebug_state upto and including all of the reg array bar the
> last in use entry.  Adding on the sizeof() therefore gives the number
> of bytes from the start of the structure upto and including the last
> in use reg entry.
> 
> I agree that the sizeof() could be written either with dbg_regs[0] or
> dbg_regs[count-1] with no change in behavior.  But I think Yufengs
> code is functionally correct.
> 
> It would have been simpler to write:
> iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count])
> 
> .. but I think that is illegal when count equals the number of
> elements defined in the array.


Oh, I see I think this should have been written using offsetof [0] and then count*sizeof to make easier to understand and fits in with most other code which uses a variable size last element. 

Thanks,
Andrew 

> 
> Cheers
> /Marcus

Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling

[Marcus Shawcroft]

On 19 December 2013 10:23,  <pinskia@gmail.com> wrote:

> Oh, I see I think this should have been written using offsetof [0] and then count*sizeof to make easier to understand and fits in with most other code which uses a variable size last element.

Umm, yes, I agree, that would be better.

/Marcus