This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] [ARM] Fixup PC in software single step
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Thu, 11 Feb 2016 11:11:29 -0500
- Subject: Re: [PATCH 1/2] [ARM] Fixup PC in software single step
- Authentication-results: sourceware.org; auth=none
- References: <1455203928-7237-1-git-send-email-yao dot qi at linaro dot org> <1455203928-7237-2-git-send-email-yao dot qi at linaro dot org>
Yao Qi writes:
> When I exercise GDBserver software single step, I see the following
> error, which has been already handled by GDB properly.
>
> In GDBserver log, we can see, GDBserver tries to single step instruction
> on 0xb6e0a6e4, and destination address is 0xffff0fe0,
>
> stop pc is 0xb6e0a6e4
> Writing f001f0e7 to 0xffff0fe0 in process 7132
> Failed to insert breakpoint at 0xffff0fe0 (Input/output error).
> Failed to insert breakpoint at 0xffff0fe0 (-1).
>
> (gdb) disassemble __aeabi_read_tp,+8
> Dump of assembler code from 0xb6e0a6e0 to 0xb6e0a6e8:
> 0xb6e0a6e0 <__aeabi_read_tp+0>: mvn r0, #61440 ; 0xf000
> 0xb6e0a6e4 <__aeabi_read_tp+4>: sub pc, r0, #31
>
> however, it fails inserting breakpoint there. This problem has already
> fixed by GDB, see comments in arm-linux-tdep.c:arm_linux_software_single_step
>
> /* The Linux kernel offers some user-mode helpers in a high page. We can
> not read this page (as of 2.6.23), and even if we could then we
> couldn't set breakpoints in it, and even if we could then the atomic
> operations would fail when interrupted. They are all called as
> functions and return to the address in LR, so step to there
> instead. */
>
> so we need to do the same thing in GDB side as well. This patch adds
> a new field fixup in arm_get_next_pcs_ops, so that we can fix up PC
> for arm-linux target. In this way, both GDB and GDBserver can single
> step instructions going to kernel helpers.
>
> gdb:
>
> 2016-02-11 Yao Qi <yao.qi@linaro.org>
>
> * arch/arm-get-next-pcs.c (arm_get_next_pcs): Call
> self->ops->fixup if it isn't NULL.
> * arch/arm-get-next-pcs.h: Include gdb_vecs.h.
> (struct arm_get_next_pcs_ops) <fixup>: New field.
> * arch/arm-linux.c: Include common-regcache.h and
> arch/arm-get-next-pcs.h.
> (arm_linux_get_next_pcs_fixup): New function.
> * arch/arm-linux.h (arm_linux_get_next_pcs_fixup): Declare.
> * arm-linux-tdep.c (arm_linux_get_next_pcs_ops): Initialize
> it with arm_linux_get_next_pcs_fixup.
> (arm_linux_software_single_step): Move code to
> arm_linux_get_next_pcs_fixup.
> * arm-tdep.c (arm_get_next_pcs_ops): Initialize it.
>
> gdb/gdbserver:
>
> 2016-02-11 Yao Qi <yao.qi@linaro.org>
>
> * linux-arm-low.c (get_next_pcs_ops): Initialize it with
> arm_linux_get_next_pcs_fixup.
> ---
> gdb/arch/arm-get-next-pcs.c | 11 +++++++++++
> gdb/arch/arm-get-next-pcs.h | 4 ++++
> gdb/arch/arm-linux.c | 20 ++++++++++++++++++++
> gdb/arch/arm-linux.h | 4 ++++
> gdb/arm-linux-tdep.c | 16 +++-------------
> gdb/arm-tdep.c | 3 ++-
> gdb/gdbserver/linux-arm-low.c | 3 ++-
> 7 files changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
> index e840147..8404869 100644
> --- a/gdb/arch/arm-get-next-pcs.c
> +++ b/gdb/arch/arm-get-next-pcs.c
> @@ -923,5 +923,16 @@ arm_get_next_pcs (struct arm_get_next_pcs *self)
> next_pcs = arm_get_next_pcs_raw (self);
> }
>
> + if (self->ops->fixup != NULL)
> + {
> + CORE_ADDR nextpc;
> + int i;
> +
> + for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, nextpc); i++)
> + {
> + nextpc = self->ops->fixup (self, nextpc);
> + VEC_replace (CORE_ADDR, next_pcs, i, nextpc);
> + }
> + }
> return next_pcs;
> }
> diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
> index 7cb0858..e038982 100644
> --- a/gdb/arch/arm-get-next-pcs.h
> +++ b/gdb/arch/arm-get-next-pcs.h
> @@ -19,6 +19,7 @@
>
> #ifndef ARM_GET_NEXT_PCS_H
> #define ARM_GET_NEXT_PCS_H 1
> +#include "gdb_vecs.h"
>
> /* Forward declaration. */
> struct arm_get_next_pcs;
> @@ -30,6 +31,9 @@ struct arm_get_next_pcs_ops
> CORE_ADDR (*syscall_next_pc) (struct arm_get_next_pcs *self, CORE_ADDR pc);
> CORE_ADDR (*addr_bits_remove) (struct arm_get_next_pcs *self, CORE_ADDR val);
> int (*is_thumb) (struct arm_get_next_pcs *self);
> +
> + /* Fix up PC if needed. */
> + CORE_ADDR (*fixup) (struct arm_get_next_pcs *self, CORE_ADDR pc);
> };
>
> /* Context for a get_next_pcs call on ARM. */
> diff --git a/gdb/arch/arm-linux.c b/gdb/arch/arm-linux.c
> index b43e484..457080c 100644
> --- a/gdb/arch/arm-linux.c
> +++ b/gdb/arch/arm-linux.c
> @@ -18,8 +18,10 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> #include "common-defs.h"
> +#include "common-regcache.h"
> #include "arch/arm.h"
> #include "arm-linux.h"
> +#include "arch/arm-get-next-pcs.h"
>
> /* Calculate the offset from stack pointer of the pc register on the stack
> in the case of a sigreturn or sigreturn_rt syscall. */
> @@ -55,3 +57,21 @@ arm_linux_sigreturn_next_pc_offset (unsigned long sp,
>
> return pc_offset;
> }
> +
> +/* Implementation of "fixup" method of struct arm_get_next_pcs_ops
> + for arm-linux. */
> +
> +CORE_ADDR
> +arm_linux_get_next_pcs_fixup (struct arm_get_next_pcs *self,
> + CORE_ADDR nextpc)
> +{
> + /* The Linux kernel offers some user-mode helpers in a high page. We can
> + not read this page (as of 2.6.23), and even if we could then we
> + couldn't set breakpoints in it, and even if we could then the atomic
> + operations would fail when interrupted. They are all called as
> + functions and return to the address in LR, so step to there
> + instead. */
> + if (nextpc > 0xffff0000)
> + nextpc = regcache_raw_get_unsigned (self->regcache, ARM_LR_REGNUM);
> + return nextpc;
> +}
> diff --git a/gdb/arch/arm-linux.h b/gdb/arch/arm-linux.h
> index 31cb351..dfd634c 100644
> --- a/gdb/arch/arm-linux.h
> +++ b/gdb/arch/arm-linux.h
> @@ -71,4 +71,8 @@ arm_linux_sigreturn_next_pc_offset (unsigned long sp,
> unsigned long svc_number,
> int is_sigreturn);
>
> +struct arm_get_next_pcs;
> +
> +CORE_ADDR arm_linux_get_next_pcs_fixup (struct arm_get_next_pcs *self,
> + CORE_ADDR pc);
> #endif /* ARM_LINUX_H */
> diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> index 3421f3b..3dbf649 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -274,7 +274,8 @@ static struct arm_get_next_pcs_ops arm_linux_get_next_pcs_ops = {
> arm_get_next_pcs_read_memory_unsigned_integer,
> arm_linux_get_next_pcs_syscall_next_pc,
> arm_get_next_pcs_addr_bits_remove,
> - arm_get_next_pcs_is_thumb
> + arm_get_next_pcs_is_thumb,
> + arm_linux_get_next_pcs_fixup,
> };
>
> static void
> @@ -950,18 +951,7 @@ arm_linux_software_single_step (struct frame_info *frame)
> next_pcs = arm_get_next_pcs (&next_pcs_ctx);
>
> for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
> - {
> - /* The Linux kernel offers some user-mode helpers in a high page. We can
> - not read this page (as of 2.6.23), and even if we could then we
> - couldn't set breakpoints in it, and even if we could then the atomic
> - operations would fail when interrupted. They are all called as
> - functions and return to the address in LR, so step to there
> - instead. */
> - if (pc > 0xffff0000)
> - pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM);
> -
> - arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
> - }
> + arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
>
> do_cleanups (old_chain);
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 6ac05f0..bf59ea7 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -247,7 +247,8 @@ static struct arm_get_next_pcs_ops arm_get_next_pcs_ops = {
> arm_get_next_pcs_read_memory_unsigned_integer,
> arm_get_next_pcs_syscall_next_pc,
> arm_get_next_pcs_addr_bits_remove,
> - arm_get_next_pcs_is_thumb
> + arm_get_next_pcs_is_thumb,
> + NULL,
> };
>
> struct arm_prologue_cache
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 0f62706..365f1c9 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -157,7 +157,8 @@ static struct arm_get_next_pcs_ops get_next_pcs_ops = {
> get_next_pcs_read_memory_unsigned_integer,
> get_next_pcs_syscall_next_pc,
> get_next_pcs_addr_bits_remove,
> - get_next_pcs_is_thumb
> + get_next_pcs_is_thumb,
> + arm_linux_get_next_pcs_fixup,
> };
>
> static int
Thanks for this, LGTM.