[PATCH/AARCH64] Fix hardware break points

[PATCH/AARCH64] Fix hardware break points

[Andrew Pinski]

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

I noticed after a fork, gdb would fail with the following message:
Unexpected error setting hardware debug registers

This is because it was trying to setup hardware breakpoint debug
registers but it was passing to ptrace garbage for the registers as
they were not zero'd out.

This fixes the problem by zeroing out the regs variable and now
debugging after a fork works on aarch64.

OK?  Built and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
* aarch64-linux-nat.c (aarch64_linux_set_debug_regs): Zero out regs.

[-- Attachment #2: fixfork.diff.txt --]
[-- Type: text/plain, Size: 614 bytes --]

? .aarch64-linux-nat.c.swp
Index: aarch64-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/aarch64-linux-nat.c,v
retrieving revision 1.4
diff -u -p -r1.4 aarch64-linux-nat.c
--- aarch64-linux-nat.c	14 Feb 2013 13:50:30 -0000	1.4
+++ aarch64-linux-nat.c	27 Jul 2013 20:12:39 -0000
@@ -312,6 +312,7 @@ aarch64_linux_set_debug_regs (const stru
   const CORE_ADDR *addr;
   const unsigned int *ctrl;
 
+  memset (&regs, 0, size(regs));
   iov.iov_base = &regs;
   iov.iov_len = sizeof (regs);
   count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;

Re: [PATCH/AARCH64] Fix hardware break points

[Andreas Schwab]

Andrew Pinski <pinskia@gmail.com> writes:

> OK?  Built and tested on aarch64-linux-gnu with no regressions.

Did you?

> +  memset (&regs, 0, size(regs));

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH/AARCH64] Fix hardware break points

[Andrew Pinski]

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

On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Andrew Pinski <pinskia@gmail.com> writes:
>
>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>
> Did you?
>
>> +  memset (&regs, 0, size(regs));

This is what I get for copying and pasting from one source file to another.

Here is the fixed one which was definitely tested.

Thanks,
Andrew Pinski

>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

[-- Attachment #2: fixfork.diff.txt --]
[-- Type: text/plain, Size: 633 bytes --]

? gdb/.aarch64-linux-nat.c.swp
Index: gdb/aarch64-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/aarch64-linux-nat.c,v
retrieving revision 1.4
diff -u -p -r1.4 aarch64-linux-nat.c
--- gdb/aarch64-linux-nat.c	14 Feb 2013 13:50:30 -0000	1.4
+++ gdb/aarch64-linux-nat.c	27 Jul 2013 22:42:18 -0000
@@ -312,6 +312,7 @@ aarch64_linux_set_debug_regs (const stru
   const CORE_ADDR *addr;
   const unsigned int *ctrl;
 
+  memset (&regs, 0, sizeof (regs));
   iov.iov_base = &regs;
   iov.iov_len = sizeof (regs);
   count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;

Re: [PATCH/AARCH64] Fix hardware break points

[Tom Tromey]

>>>>> "Andrew" == Andrew Pinski <pinskia@gmail.com> writes:

Andrew> Here is the fixed one which was definitely tested.

It seems reasonable to me, but I think that gdbserver needs a parallel
fix.

Just add it to the list of concrete reasons to merge the back ends...

Tom

Re: [PATCH/AARCH64] Fix hardware break points

[Will Newton]

On 27 July 2013 23:42, Andrew Pinski <pinskia@gmail.com> wrote:

Hi Andrew,

> On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Andrew Pinski <pinskia@gmail.com> writes:
>>
>>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>>
>> Did you?
>>
>>> +  memset (&regs, 0, size(regs));
>
> This is what I get for copying and pasting from one source file to another.
>
> Here is the fixed one which was definitely tested.

What's the status of this patch? It seems like it fixes real problems
people are seeing in the field.

Thanks,

-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH/AARCH64] Fix hardware break points

[Andrew Pinski]

On Tue, Sep 10, 2013 at 7:37 AM, Will Newton <will.newton@linaro.org> wrote:
> On 27 July 2013 23:42, Andrew Pinski <pinskia@gmail.com> wrote:
>
> Hi Andrew,
>
>> On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>> Andrew Pinski <pinskia@gmail.com> writes:
>>>
>>>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>>>
>>> Did you?
>>>
>>>> +  memset (&regs, 0, size(regs));
>>
>> This is what I get for copying and pasting from one source file to another.
>>
>> Here is the fixed one which was definitely tested.
>
> What's the status of this patch? It seems like it fixes real problems
> people are seeing in the field.


After not much thought, I decided this was an obvious patch as regs is
used uninitialized otherwise when passed to ptrace.

Committed now.

Thanks,
Andrew Pinski

>
> Thanks,
>
> --
> Will Newton
> Toolchain Working Group, Linaro

Re: [PATCH/AARCH64] Fix hardware break points

[Pedro Alves]

On 09/12/2013 08:15 AM, Andrew Pinski wrote:
> On Tue, Sep 10, 2013 at 7:37 AM, Will Newton <will.newton@linaro.org> wrote:
>> On 27 July 2013 23:42, Andrew Pinski <pinskia@gmail.com> wrote:
>>
>> Hi Andrew,
>>
>>> On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>> Andrew Pinski <pinskia@gmail.com> writes:
>>>>
>>>>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>>>>
>>>> Did you?
>>>>
>>>>> +  memset (&regs, 0, size(regs));
>>>
>>> This is what I get for copying and pasting from one source file to another.
>>>
>>> Here is the fixed one which was definitely tested.
>>
>> What's the status of this patch? It seems like it fixes real problems
>> people are seeing in the field.

> After not much thought, I decided this was an obvious patch as regs is
> used uninitialized otherwise when passed to ptrace.

Leaves me wondering what field of regs other than regs.dbg_regs is
the kernel actually looking at then for NT_ARM_HW_WATCH/NT_ARM_HW_BREAK,
given regs.dbg_regs _is_ initialized:

  for (i = 0; i < count; i++)
    {
      regs.dbg_regs[i].addr = addr[i];
      regs.dbg_regs[i].ctrl = ctrl[i];
    }

  if (ptrace (PTRACE_SETREGSET, tid,
	      watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
	      (void *) &iov))

Makes me wonder whether the issue is that "count" isn't right for
the running kernel.

Was a patch for gdbserver ever posted/committed?  AFAICS,
gdbserver's aarch64_linux_set_debug_regs is an exact copy of gdb's.

-- 
Pedro Alves

Re: [PATCH/AARCH64] Fix hardware break points

[Will Newton]

On 16 September 2013 13:43, Pedro Alves <palves@redhat.com> wrote:
> On 09/12/2013 08:15 AM, Andrew Pinski wrote:
>> On Tue, Sep 10, 2013 at 7:37 AM, Will Newton <will.newton@linaro.org> wrote:
>>> On 27 July 2013 23:42, Andrew Pinski <pinskia@gmail.com> wrote:
>>>
>>> Hi Andrew,
>>>
>>>> On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>> Andrew Pinski <pinskia@gmail.com> writes:
>>>>>
>>>>>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>>>>>
>>>>> Did you?
>>>>>
>>>>>> +  memset (&regs, 0, size(regs));
>>>>
>>>> This is what I get for copying and pasting from one source file to another.
>>>>
>>>> Here is the fixed one which was definitely tested.
>>>
>>> What's the status of this patch? It seems like it fixes real problems
>>> people are seeing in the field.
>
>> After not much thought, I decided this was an obvious patch as regs is
>> used uninitialized otherwise when passed to ptrace.
>
> Leaves me wondering what field of regs other than regs.dbg_regs is
> the kernel actually looking at then for NT_ARM_HW_WATCH/NT_ARM_HW_BREAK,
> given regs.dbg_regs _is_ initialized:

The dbg_info field is probably the important one, so it may be
possible to optimize the clearing slightly but I'm not sure if it's
worth it.

>   for (i = 0; i < count; i++)
>     {
>       regs.dbg_regs[i].addr = addr[i];
>       regs.dbg_regs[i].ctrl = ctrl[i];
>     }
>
>   if (ptrace (PTRACE_SETREGSET, tid,
>               watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
>               (void *) &iov))
>
> Makes me wonder whether the issue is that "count" isn't right for
> the running kernel.
>
> Was a patch for gdbserver ever posted/committed?  AFAICS,
> gdbserver's aarch64_linux_set_debug_regs is an exact copy of gdb's.

I posted a patch for that here:

https://sourceware.org/ml/gdb-patches/2013-09/msg00381.html

-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH/AARCH64] Fix hardware break points

[Pedro Alves]

On 09/16/2013 01:53 PM, Will Newton wrote:
> On 16 September 2013 13:43, Pedro Alves <palves@redhat.com> wrote:
>> On 09/12/2013 08:15 AM, Andrew Pinski wrote:
>>> On Tue, Sep 10, 2013 at 7:37 AM, Will Newton <will.newton@linaro.org> wrote:
>>>> On 27 July 2013 23:42, Andrew Pinski <pinskia@gmail.com> wrote:
>>>>
>>>> Hi Andrew,
>>>>
>>>>> On Sat, Jul 27, 2013 at 2:34 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>> Andrew Pinski <pinskia@gmail.com> writes:
>>>>>>
>>>>>>> OK?  Built and tested on aarch64-linux-gnu with no regressions.
>>>>>>
>>>>>> Did you?
>>>>>>
>>>>>>> +  memset (&regs, 0, size(regs));
>>>>>
>>>>> This is what I get for copying and pasting from one source file to another.
>>>>>
>>>>> Here is the fixed one which was definitely tested.
>>>>
>>>> What's the status of this patch? It seems like it fixes real problems
>>>> people are seeing in the field.
>>
>>> After not much thought, I decided this was an obvious patch as regs is
>>> used uninitialized otherwise when passed to ptrace.
>>
>> Leaves me wondering what field of regs other than regs.dbg_regs is
>> the kernel actually looking at then for NT_ARM_HW_WATCH/NT_ARM_HW_BREAK,
>> given regs.dbg_regs _is_ initialized:
> 
> The dbg_info field is probably the important one, 

Thanks.  That's plausible:

struct user_hwdebug_state {
        __u32           dbg_info;
        __u32           pad;
        struct {
                __u64   addr;
                __u32   ctrl;
                __u32   pad;
        }               dbg_regs[16];
};

Made me wonder whether we shouldn't actually be writing to
dbg_info the number of slots we're actually putting in dbg_regs
ored with AARCH64_DEBUG_ARCH_V8, but it sounds like
the kernel might now be checking for '0' explicitly, which could
be equivalent to assuming AARCH64_DEBUG_ARCH_V8 in the future.
(I encourage you to think about this a little, if you haven't yet.)

> 
>>   for (i = 0; i < count; i++)
>>     {
>>       regs.dbg_regs[i].addr = addr[i];
>>       regs.dbg_regs[i].ctrl = ctrl[i];
>>     }
>>
>>   if (ptrace (PTRACE_SETREGSET, tid,
>>               watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
>>               (void *) &iov))
>>
>> Makes me wonder whether the issue is that "count" isn't right for
>> the running kernel.
>>
>> Was a patch for gdbserver ever posted/committed?  AFAICS,
>> gdbserver's aarch64_linux_set_debug_regs is an exact copy of gdb's.
> 
> I posted a patch for that here:
> 
> https://sourceware.org/ml/gdb-patches/2013-09/msg00381.html

Thanks.  I replied to that thread.

-- 
Pedro Alves