Re: [PATCH/AARCH64] Fix hardware break points

[Pedro Alves <palves@redhat.com>]

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