* [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
@ 2013-12-18 16:07 Yufeng Zhang
2013-12-18 16:24 ` Pedro Alves
2013-12-19 7:00 ` pinskia
0 siblings, 2 replies; 6+ messages in thread
From: Yufeng Zhang @ 2013-12-18 16:07 UTC (permalink / raw)
To: gdb-patches; +Cc: Marcus Shawcroft
[-- 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 (®s, 0, sizeof (regs));
iov.iov_base = ®s;
- 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 (®s, 0, sizeof (regs));
iov.iov_base = ®s;
- 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++)
{
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
2013-12-18 16:07 [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling Yufeng Zhang
@ 2013-12-18 16:24 ` Pedro Alves
2013-12-19 7:00 ` pinskia
1 sibling, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2013-12-18 16:24 UTC (permalink / raw)
To: Yufeng Zhang; +Cc: gdb-patches, Marcus Shawcroft
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
2013-12-18 16:07 [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling Yufeng Zhang
2013-12-18 16:24 ` Pedro Alves
@ 2013-12-19 7:00 ` pinskia
2013-12-19 10:16 ` Marcus Shawcroft
1 sibling, 1 reply; 6+ messages in thread
From: pinskia @ 2013-12-19 7:00 UTC (permalink / raw)
To: Yufeng Zhang; +Cc: gdb-patches, Marcus Shawcroft
> 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
2013-12-19 7:00 ` pinskia
@ 2013-12-19 10:16 ` Marcus Shawcroft
2013-12-19 10:23 ` pinskia
0 siblings, 1 reply; 6+ messages in thread
From: Marcus Shawcroft @ 2013-12-19 10:16 UTC (permalink / raw)
To: pinskia; +Cc: Yufeng Zhang, gdb-patches, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
2013-12-19 10:16 ` Marcus Shawcroft
@ 2013-12-19 10:23 ` pinskia
2013-12-19 10:26 ` Marcus Shawcroft
0 siblings, 1 reply; 6+ messages in thread
From: pinskia @ 2013-12-19 10:23 UTC (permalink / raw)
To: Marcus Shawcroft; +Cc: Yufeng Zhang, gdb-patches, Marcus Shawcroft
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling
2013-12-19 10:23 ` pinskia
@ 2013-12-19 10:26 ` Marcus Shawcroft
0 siblings, 0 replies; 6+ messages in thread
From: Marcus Shawcroft @ 2013-12-19 10:26 UTC (permalink / raw)
To: pinskia; +Cc: Yufeng Zhang, gdb-patches, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-19 10:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-18 16:07 [PATCH, AArch64] Fix bug in hardware watchpoint/breakpoint handling Yufeng Zhang
2013-12-18 16:24 ` Pedro Alves
2013-12-19 7:00 ` pinskia
2013-12-19 10:16 ` Marcus Shawcroft
2013-12-19 10:23 ` pinskia
2013-12-19 10:26 ` Marcus Shawcroft
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).