Re: [PATCH v2 6/7] Refactor the breakpoint definitions in linux-arm-low.c.

[Pedro Alves <palves@redhat.com>]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> Before arm_breakpoint_from_pc would use an #ifdef to return the right
> arm_breakpoint from the abi or eabi breakpoint type.
> 
> arm_breakpoint_at would also check for the arm_breakpoint ||
> arm_eabi_breakpoint.
> 
> Thus the selected arm_breakpoint would be what arm_breakpoint_from_pc returned
> and arm_breakpoint was arm_abi_breakpoint.
> 
> This patch makes it more clear by naming those for what they are : 2 separate
> entities: arm_abi_breakpoint and arm_eabi_breakpoint and set the current used
> one as arm_breakpoint.
> 
> This allows a cleaner arm_breakpoint_from_pc as it just returns arm_breakpoint
> rather than having the #ifdef in that function.
> 
> Any other reference to the arm_breakpoint can now also be clear of #ifdefs...
> 
> No regressions on Ubuntu 14.04 on ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/gdbserver/ChangeLog:
> 	* linux-arm-low.c: Refactor breakpoint definitions.
> 	(arm_breakpoint_at): Adjust for arm_abi_breakpoint.
> 	(arm_breakpoint_from_pc): Adjust for arm_breakpoint.
> ---
>  gdb/gdbserver/linux-arm-low.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 8f420f9..d16ea60 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -236,18 +236,25 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)
>  }
>  
>  /* Correct in either endianness.  */
> -static const unsigned long arm_breakpoint = 0xef9f0001;
> -#define arm_breakpoint_len 4
> -static const unsigned short thumb_breakpoint = 0xde01;
> -#define thumb_breakpoint_len 2
> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> -#define thumb2_breakpoint_len 4
> +#define arm_abi_breakpoint 0xef9f0001UL
>  
>  /* For new EABI binaries.  We recognize it regardless of which ABI
>     is used for gdbserver, so single threaded debugging should work
>     OK, but for multi-threaded debugging we only insert the current
>     ABI's breakpoint instruction.  For now at least.  */
> -static const unsigned long arm_eabi_breakpoint = 0xe7f001f0;
> +#define arm_eabi_breakpoint 0xe7f001f0UL
> +
> +#ifndef __ARM_EABI__
> +static const unsigned long arm_breakpoint = arm_abi_breakpoint;
> +#else
> +static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
> +#endif
> +
> +#define arm_breakpoint_len 4
> +static const unsigned short thumb_breakpoint = 0xde01;
> +#define thumb_breakpoint_len 2
> +static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> +#define thumb2_breakpoint_len 4
>  
>  static int
>  arm_breakpoint_at (CORE_ADDR where)
> @@ -279,7 +286,7 @@ arm_breakpoint_at (CORE_ADDR where)
>        unsigned long insn;
>  
>        (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
> -      if (insn == arm_breakpoint)
> +      if (insn == arm_abi_breakpoint)
>  	return 1;
>  
>        if (insn == arm_eabi_breakpoint)
> @@ -325,11 +332,7 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>    else
>      {
>        *lenptr = arm_breakpoint_len;
> -#ifndef __ARM_EABI__
>        return (const unsigned char *) &arm_breakpoint;
> -#else
> -      return (const unsigned char *) &arm_eabi_breakpoint;
> -#endif
>      }
>  }
>  
> 

LGTM.  (Thought please collect an ack from Yao as well.)

Thanks,
Pedro Alves