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

[Pedro Alves <palves@redhat.com>]

On 10/13/2015 11:11 AM, Yao Qi wrote:

> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -315,9 +315,12 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
>  	ret = -1;
>      }
>    else
> -    ret =
> -      aarch64_handle_breakpoint (targ_type, addr, len, 1 /* is_insert */,
> -				 state);
> +    {
> +      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.

> +      ret = aarch64_handle_breakpoint (targ_type, addr, len,
> +				       1 /* is_insert */, state);
> +    }
>  
>    if (show_debug_regs)
>      aarch64_show_debug_reg_state (state, "insert_point", addr, len,
> @@ -353,9 +356,12 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
>        aarch64_handle_watchpoint (targ_type, addr, len, 0 /* is_insert */,
>  				 state);
>    else
> -    ret =
> -      aarch64_handle_breakpoint (targ_type, addr, len, 0 /* is_insert */,
> -				 state);
> +    {
> +      if (len == 3)
> +	len = 2;
> +      ret = aarch64_handle_breakpoint (targ_type, addr, len,
> +				       0 /* is_insert */,  state);
> +    }
>  
>    if (show_debug_regs)
>      aarch64_show_debug_reg_state (state, "remove_point", addr, len,
> 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.

> +    }
>  
>    if (addr & (alignment - 1))
>      return 0;
> @@ -445,7 +454,7 @@ aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
>  			   struct aarch64_debug_reg_state *state)
>  {
>    /* The hardware breakpoint on AArch64 should always be 4-byte
> -     aligned.  */
> +     aligned, but on AArch32, it can be 2-byte aligned.  */
>    if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len))
>      return -1;
>  
> -- 1.9.1
> 


Thanks,
Pedro Alves