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

[Yao Qi <qiyaoltc@gmail.com>]

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.

-- 
Yao (齐尧)