Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Yao Qi <qiyaoltc at gmail dot com>]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> gdb/ChangeLog:
> 	* Makefile.in: Add arm-common.o.
> 	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.

"Move to arm-common.c".  Some macros are moved, we should mention the
move here as well.

> 	* common/arm-common.c: New file.
> 	* common/arm-common.h: New file.

arm-common.c is not a good file name to me, how about arm-insn.c?
Please move this file to arch/ directory rather than common/

> 	* configure.tgt: Add arm-common.o.
> 	* gdbserver/Makefile.in: Add arm-common.c/o.

ChangeLog entries for GDBserver should be written separately.

> 	* gdbserver/configure.srv: Add arm-common.o.
> 	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
>         breakpoint types.
> 	(arm_breakpoint_from_pc): New function.

arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is.

> diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c
> new file mode 100644
> index 0000000..861b249
> --- /dev/null
> +++ b/gdb/common/arm-common.c
> @@ -0,0 +1,32 @@
> +/* Common code for generic ARM support.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +

Contents in file are moved from existing file, so we should still keep
the year range of the original file.  It should be

     Copyright (C) 1988-2015 Free Software Foundation, Inc.


> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index c42b4df..e831f59 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
>  	;;
>  arm*-*-linux*)
>  	# Target: ARM based machine running GNU/Linux
> -	gdb_target_obs="arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
> +	gdb_target_obs="arm-common.o arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
>  			solib-svr4.o symfile-mem.o linux-tdep.o linux-record.o"
>  	build_gdbserver=yes

since arm-common.o is moved out of arm-tdep.o, so it should be added to
every target which uses arm-tdep.o, such as aarch64*-*-linux*,
arm*-wince-pe, etc.

> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 367c704..15ecb70 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -30,6 +30,8 @@
>  #include "nat/gdb_ptrace.h"
>  #include <signal.h>
>  
> +#include "common/arm-common.h"
> +
>  /* Defined in auto-generated files.  */
>  void init_registers_arm (void);
>  extern const struct target_desc *tdesc_arm;
> @@ -234,19 +236,28 @@ 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;
> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> +#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

I am confused by your changes here.  Why do you change them?

>  
>  static int
> -arm_breakpoint_at (CORE_ADDR where)
> +arm_is_thumb_mode (void)
>  {
>    struct regcache *regcache = get_thread_regcache (current_thread, 1);
>    unsigned long cpsr;
> @@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
>    collect_register_by_name (regcache, "cpsr", &cpsr);
>  
>    if (cpsr & 0x20)
> +      return 1;
> +  else
> +      return 0;

Indentation looks odd.

> +}
> +
> +/* Returns 1 if there is a software breakpoint at location.  */
> +
> +static int
> +arm_breakpoint_at (CORE_ADDR where)
> +{
> +  if (arm_is_thumb_mode ())
>      {
>        /* Thumb mode.  */
>        unsigned short insn;
> @@ -275,7 +297,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)
> @@ -285,6 +307,53 @@ arm_breakpoint_at (CORE_ADDR where)
>    return 0;
>  }
>  
> +/* Determine the type and size of breakpoint to insert at PCPTR.  Uses
> +   the program counter value to determine whether a 16-bit or 32-bit
> +   breakpoint should be used.  It returns a pointer to a string of
> +   bytes that encode a breakpoint instruction, stores the length of
> +   the string to *lenptr, and adjusts the program counter (if
> +   necessary) to point to the actual memory location where the
> +   breakpoint should be inserted.  */
> +
> +static const unsigned char *
> +arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
> +{
> +  /* Default if no pc is set to arm breakpoint.  */
> +  if (pcptr == NULL)

Such NULL checking is no longer needed, right?

-- 
Yao (éå)