This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer.
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 26 Nov 2015 08:35:46 -0500
- Subject: Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer.
- Authentication-results: sourceware.org; auth=none
- References: <1448287968-12907-1-git-send-email-antoine dot tremblay at ericsson dot com> <1448287968-12907-9-git-send-email-antoine dot tremblay at ericsson dot com> <5656E3AC dot 1010000 at redhat dot com>
On 11/26/2015 05:49 AM, Pedro Alves wrote:
On 11/23/2015 02:12 PM, Antoine Tremblay wrote:
In this v3:
* The common arm_get_next_pcs call has been refactored the same way like
so : VEC (CORE_ADDR) *arm_get_next_pcs (struct arm_get_next_pcs *ctx,
CORE_ADDR pc);
* Use ctor functions to construct gdb|gdbserver_get_next_pcs context.
* Some style fixes.
---
This patch teaches GDBserver how to software single step on ARM
linux by sharing code with GDB.
The arm_get_next_pcs function in GDB is now shared with GDBServer. So that
GDBServer can use the function to return the possible addresses of the next PC.
A proper shared context was also needed so that we could share the code, this
context is described as this data structure (expressed as a class
hierarchy):
struct arch_get_next_pcs
struct arch_(gdb|gdbserver)_get_next_pcs
Where arch can by replaced by arm for this patch. This structure should be
flexible enough to accomodate any arch that would need a get_next_pcs
context.
(accommodate)
Fixed.
Limitations :
GDBServer can't step over a sigreturn or rt_sigreturn syscall since this would
require the DWARF information to identify the caller PC. This situation
only prints a warning for the moment.
I wonder whether this ends up being a regression? E.g., if you
put a breakpoint with a condition that evals false, on top of such an
instruction?
Yes I think it could, I'll see if I can reproduce that.
I wonder whether this ends up being a regression, or whether it only trigger
on new features, like tracepoints?
That was my initial assessment but I think it could be a regression indeed.
I'm thinking that it may be a
regression for the case of a conditional breakpoint with a conditional
evaluated on the target?
Other than the warning, what happens? Does gdbserver lose control of
the inferior?
It will lose control, Yao has suggested a solution, I will look into it.
+ /* Insert a breakpoint right after the end of the atomic sequence. */
+ breaks[0] = loc;
+
+ /* Check for duplicated breakpoints. Check also for a breakpoint
+ placed (branch instruction's destination) anywhere in sequence. */
+ if (last_breakpoint
+ && (breaks[1] == breaks[0]
+ || (breaks[1] >= pc && breaks[1] < loc)))
+ last_breakpoint = 0;
+
+ /* Adds the breakpoints to the list to be inserted. */
+ for (index = 0; index <= last_breakpoint; index++)
+ {
+ VEC_safe_push (CORE_ADDR, *next_pcs, MAKE_THUMB_ADDR (breaks[index]));
+ }
No braces.
Fixed. and in the other place in the file too.
+
+ return 1;
+}
+
+ else if (itstate & 0x0f)
+ {
+ /* We are in a conditional block. Check the condition. */
+ int cond = itstate >> 4;
+
+ if (! condition_true (cond, status))
+ /* Advance to the next instruction. All the 32-bit
+ instructions share a common prefix. */
+ VEC_safe_push (CORE_ADDR, *next_pcs,
+ MAKE_THUMB_ADDR (pc + thumb_insn_size (inst1)));
Here there should be braces around the comment and the VEC call.
Ok.
+
+ return *next_pcs;
+
+ /* Otherwise, handle the instruction normally. */
+ }
@@ -912,27 +922,44 @@ arm_linux_software_single_step (struct frame_info *frame)
{
struct gdbarch *gdbarch = get_frame_arch (frame);
struct address_space *aspace = get_frame_address_space (frame);
- CORE_ADDR next_pc;
-
- if (arm_deal_with_atomic_sequence (frame))
- return 1;
+ struct arm_gdb_get_next_pcs next_pcs_ctx;
+ CORE_ADDR pc;
+ int i;
+ VEC (CORE_ADDR) *next_pcs = NULL;
/* If the target does have hardware single step, GDB doesn't have
to bother software single step. */
if (target_can_do_single_step () == 1)
return 0;
- next_pc = arm_get_next_pc (frame, get_frame_pc (frame));
+ arm_gdb_get_next_pcs_ctor (&next_pcs_ctx,
+ &arm_linux_get_next_pcs_ops,
+ gdbarch_byte_order (gdbarch),
+ gdbarch_byte_order_for_code (gdbarch),
+ arm_frame_is_thumb (frame),
+ arm_apcs_32,
+ gdbarch_tdep (gdbarch)->thumb2_breakpoint,
+ frame,
+ gdbarch);
- /* 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 (next_pc > 0xffff0000)
- next_pc = get_frame_register_unsigned (frame, ARM_LR_REGNUM);
+ next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs_ctx,
+ get_frame_pc (frame));
+
+ 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, next_pc);
+ VEC_free (CORE_ADDR, next_pcs);
This would be better freed with a cleanup:
VEC (CORE_ADDR) *next_pcs = NULL;
old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), next_pcs);
...
do_cleanups (old_chain);
return 1;
}
Ok fixed other occurrences too.
/* There's a lot of code before this instruction. Start with an
optimistic search; it's easy to recognize halfwords that can
@@ -7291,6 +6106,116 @@ thumb2_copy_block_xfer (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2,
return 0;
}
+/* Initialize arm_gdb_get_next_pcs_stor. */
/* Initialize arm_gdb_get_next_pcs. */
Done.
+void
+arm_gdb_get_next_pcs_ctor (struct arm_gdb_get_next_pcs *self,
+ struct arm_get_next_pcs_ops *ops,
+ int byte_order,
+/* single_step() is called just before we want to resume the inferior,
+ if we want to single-step it but there is no hardware or kernel
+ single-step support. We find the target of the coming instructions
+ and breakpoint them. */
+
+int
+arm_software_single_step (struct frame_info *frame)
+{
+ struct gdbarch *gdbarch = get_frame_arch (frame);
+ struct address_space *aspace = get_frame_address_space (frame);
+ struct arm_gdb_get_next_pcs next_pcs_ctx;
+ CORE_ADDR pc;
+ int i;
+ VEC (CORE_ADDR) *next_pcs = NULL;
+
+ arm_gdb_get_next_pcs_ctor (&next_pcs_ctx,
+ &arm_get_next_pcs_ops,
+ gdbarch_byte_order (gdbarch),
+ gdbarch_byte_order_for_code (gdbarch),
+ arm_frame_is_thumb (frame),
+ arm_apcs_32,
+ gdbarch_tdep (gdbarch)->thumb2_breakpoint,
+ frame,
+ gdbarch);
+
+ next_pcs = arm_get_next_pcs ((struct arm_get_next_pcs *) &next_pcs,
This is wrong. Should be "(struct arm_get_next_pcs *) &next_pcs_ctx" instead.
Wow :( Indeed thx.
I think this shows that it'd be good to run the series against the
testsuite on native ARM.
I do run that all the time but on linux, it turns out I made the
misstake in arm-tdep.c but not in arm-linux-tdep.c so it did not show up
in my tests.
Thanks for seeing it!
You could avoid these wrong casts by writting "&next_pcs_ctx.base" instead.
Indeed fixing like so all occurrences.
+ get_frame_pc (frame));
+
+ for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
+ {
+ arm_insert_single_step_breakpoint (gdbarch, aspace, pc);
+ }
No braces.
Fixed.
+
+ VEC_free (CORE_ADDR, next_pcs);
Use a cleanup instead.
Fixed.
+
+ return 1;
+}
+
/* Cleanup/copy SVC (SWI) instructions. These two functions are overridden
for Linux, where some SVC instructions must be treated specially. */
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 9b8447b..f3a13a4 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -23,6 +23,9 @@
struct gdbarch;
struct regset;
struct address_space;
+struct get_next_pcs;
+struct arm_get_next_pcs;
+struct gdb_get_next_pcs;
#include "arch/arm.h"
@@ -221,6 +224,17 @@ struct displaced_step_closure
struct displaced_step_closure *);
};
+/* Context for a get_next_pcs call on ARM in GDB. */
+struct arm_gdb_get_next_pcs
+{
+ /* Common context for gdb/gdbserver. */
+ struct arm_get_next_pcs base;
+ /* Frame information. */
+ struct frame_info *frame;
+ /* Architecture dependent information. */
+ struct gdbarch *gdbarch;
+};
+
/* Values for the WRITE_PC argument to displaced_write_reg. If the register
write may write to the PC, specifies the way the CPSR T bit, etc. is
modified by the instruction. */
@@ -250,11 +264,37 @@ extern void
ULONGEST val, enum pc_write_style write_pc);
CORE_ADDR arm_skip_stub (struct frame_info *, CORE_ADDR);
-CORE_ADDR arm_get_next_pc (struct frame_info *, CORE_ADDR);
+
+ULONGEST arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
+ int len,
+ int byte_order);
+
+void arm_gdb_get_next_pcs_ctor (struct arm_gdb_get_next_pcs *self,
+ struct arm_get_next_pcs_ops *ops,
+ int byte_order,
+ int byte_order_for_code,
+ int is_thumb,
+ int arm_apcs_32,
+ const gdb_byte *arm_thumb2_breakpoint,
+ struct frame_info *frame,
+ struct gdbarch *gdbarch);
+
+CORE_ADDR
+arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
See extension.h for how to indent these long declarations. The function
name should not be at column 0.
Ok, thanks for the example.
+ CORE_ADDR val);
+
+ULONGEST
+arm_get_next_pcs_collect_register_unsigned (struct arm_get_next_pcs *self,
+ int n);
+
+CORE_ADDR
+arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc);
+
+int arm_software_single_step (struct frame_info *frame);
+
+
/* These are in <asm/elf.h> in current kernels. */
#define HWCAP_VFP 64
#define HWCAP_IWMMXT 512
@@ -146,6 +156,29 @@ static int arm_regmap[] = {
64
};
+/* Forward declarations needed for get_next_pcs ops. */
+static ULONGEST
+get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
+ int len,
+ int byte_order);
+
+static ULONGEST
+get_next_pcs_collect_register_unsigned (struct arm_get_next_pcs *self, int n);
+
+static CORE_ADDR
+get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self, CORE_ADDR val);
+
+static CORE_ADDR
+get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc);
+
Ditto.
Done.
Note I also used a cleanup now in
install_software_single_step_breakpoint and removed the braces in the for.
Thank you,
Antoine