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

[Pedro Alves <palves@redhat.com>]

On 10/13/2015 04:26 PM, Yao Qi 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;
>      }

Sounds fine.

> 
>>> 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.

At least the comment should be updated.  It's quite misleading as is.

Thanks,
Pedro Alves