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

[Antoine Tremblay <antoine dot tremblay at ericsson 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