Re: [PATCH v3 08/10] Support software single step on ARM in GDBServer.

Pedro Alves
2015-11-26T10:49:22Z

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)

>
> 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?

I wonder whether this ends up being a regression, or whether it only trigger
on new features, like tracepoints? 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?

> + /* 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.

> +
> + 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.

> +
> + 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;
> }

> /* 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. */

> +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.

I think this shows that it'd be good to run the series against the
testsuite on native ARM.

You could avoid these wrong casts by writting "&next_pcs_ctx.base" instead.

> + 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.

> +
> + VEC_free (CORE_ADDR, next_pcs);

Use a cleanup instead.

> +
> + 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.

> + 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.

Thanks,
Pedro Alves