This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Mon, 28 Sep 2015 11:29:42 +0100
- Subject: Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.
- Authentication-results: sourceware.org; auth=none
- References: <1442577749-6650-1-git-send-email-antoine dot tremblay at ericsson dot com> <1442577749-6650-4-git-send-email-antoine dot tremblay at ericsson 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 (éå)