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

Antoine Tremblay
2015-11-26T13:35:52Z

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