Re: [PATCH] aarch64 multi-arch part 6: HW breakpoint on unaligned address

[Andrew Pinski <pinskia at gmail dot com>]

On Tue, Oct 13, 2015 at 8:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> +    {
>>> +      if (len == 3)
>>> +    len = 2;
>>
>> I think this warrants a comment.  E.g., someone reading
>> arm-linux-low.c:arm_linux_hw_point_initialize quite easily grasps
>> what 3 means.
>>
>
> How about the comment like this?
>
>    if (len == 3)
>      {
>         /* LEN is 3 means the breakpoint is set on a 32-bit thumb
>            instruction.   Set it to 2 to correctly encode length bit
>            mask in hardware/watchpoint control register.  */
>         len = 2;
>      }
>
>>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>>> index bca6ec1..d15e518 100644
>>> --- a/gdb/nat/aarch64-linux-hw-point.c
>>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>>> @@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
>>>  static int
>>>  aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
>>>  {
>>> -  unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
>>> -    : AARCH64_HBP_ALIGNMENT;
>>> +  unsigned int alignment = 0;
>>> +
>>> +  if (is_watchpoint)
>>> +    alignment = AARCH64_HWP_ALIGNMENT;
>>> +  else
>>> +    {
>>> +      /* Set alignment to 2 only if the current process is 32-bit,
>>> +     since thumb instruction can be 2-byte aligned.  Otherwise, set
>>> +     alignment to AARCH64_HBP_ALIGNMENT.  */
>>> +      alignment = 2;
>>
>> Is some other code doing what the comment says?  I'm not seeing
>> any obvious 32-bit check.
>
> No, I don't do the 32-bit check here.  Ideally, we should set alignment
> to 2 only when the process is 32-bit, and still use 4 as alignment
> otherwise.  However, I don't find an easy way to do the 32-bit check
> here, because this code is used by both GDB and GDBserver.  We can do
> the 32-bit check in GDB and GDBserver respectively, and pass the result
> to nat/aarch64-linux-hw-point.c, but I don't like putting information down
> multiple levels like this.


Also it is not just about 32bit vs 64bit either.  It is about aarch32
vs aarch64.  I think we should push that information down as far as we
can.

Thanks,
Andrew

>
> --
> Yao (éå)