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 07:48 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> > 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.
>
> They are using frame not DWARF to identify the PC, however, it is not
> necessary.  When GDB or GDBserver is doing software single step, it must
> be in the inner-most frame, so we can get the caller PC by examining the
> stack at a certain offset from SP (from regcache), like what are doing in
> arm_linux_sigreturn_init and arm_linux_rt_sigreturn_init, rather than
> replying frame unwinding.  See my comments to arm_get_next_pcs_syscall_next_pc.

Humm and frame can use the dwarf unwinder can it not ? frame is still 
very confusing to me.

But I'm glad it's not the only option, I'll examine the stack and fix it.

> > +
> > +/* Wrapper over syscall_next_pc for use in get_next_pcs.  */
> > +
> > +CORE_ADDR
> > +arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc)
> > +{
> > +  struct arm_gdb_get_next_pcs *next_pcs = (struct arm_gdb_get_next_pcs *) self;
> > +  struct gdbarch_tdep *tdep;
> > +
> > +  tdep = gdbarch_tdep (next_pcs->gdbarch);
> > +  if (tdep->syscall_next_pc != NULL)
> > +    return tdep->syscall_next_pc (next_pcs->frame);
> > +
> > +  return 0;
> > +}
> > +
>
>
> > +/* When PC is at a syscall instruction, return the PC of the next
> > +   instruction to be executed.  */
> > +static CORE_ADDR
> > +get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self, CORE_ADDR pc)
> > +{
> > +  CORE_ADDR return_addr = 0;
> > +  int is_thumb = self->is_thumb;
> > +  ULONGEST svc_number = 0;
> > +
> > +  if (is_thumb)
> > +    {
> > +      svc_number = self->ops->collect_register_unsigned (self, 7);
> > +      return_addr = pc + 2;
> > +    }
> > +  else
> > +    {
> > +      unsigned long this_instr = self->ops->read_memory_unsigned_integer
> > +	((CORE_ADDR) pc, 4, self->byte_order_for_code);
> > +
> > +      unsigned long svc_operand = (0x00ffffff & this_instr);
> > +      if (svc_operand)  /* OABI.  */
> > +	{
> > +	  svc_number = svc_operand - 0x900000;
> > +	}
> > +      else /* EABI.  */
> > +	{
> > +	  svc_number = self->ops->collect_register_unsigned (self, 7);
> > +	}
> > +
> > +      return_addr = pc + 4;
> > +    }
> > +
> > +  /* Is this a sigreturn or rt_sigreturn syscall?
> > +     If so it is currently not handeled.  */
> > +  if (svc_number == 119 || svc_number == 173)
> > +    {
> > +      if (debug_threads)
> > +	debug_printf ("Unhandled sigreturn or rt_sigreturn syscall\n");
>
> We can still compute the caller pc by examining stack.
>
> > +    }
> > +
> > +  /* Addresses for calling Thumb functions have the bit 0 set.  */
> > +  if (is_thumb)
> > +    return_addr = MAKE_THUMB_ADDR (return_addr);
> > +
> > +  return return_addr;
> > +}
>
> This leads me thinking more about current software single step code in
> GDB.  In GDB side, we are using frame to access registers, while in
> GDBserver, we are using regcache.  Why don't we use regcache in both
> sides? so that the "struct arm_get_next_pcs_ops" can be simplified, for
> example, field collect_register_unsigned may be no longer necessary.
>
> In fact, software_single_step used to have regcache argument, added by
> https://sourceware.org/ml/gdb-patches/2007-04/msg00218.html but we
> changed to argument frame
> https://sourceware.org/ml/gdb-patches/2007-04/msg00218.html

This is the same link as the previous one...

 IMO, it is
> better to use regcache than frame.  We have two options,
>
>   #1, switch from frame apis to regcache apis to access registers in arm
>    software single step.  We can get regcache by get_current_regcache ().
>   #2, change argument of gdbarch method software_single_step from frame
>    to regcache, which means all its implementations need update, and
>    switch to regcache apis to access registers.
>
> #2 is the right way to go in long term, and we really need to improve
> software_single_step.  Let me what do you think.

Looking at the impacts of #2, I do not feel comfortable including these 
changes in this patch set. I feel they would require a patch set of 
their own.

However #1 seems like something possible I would start by this option if 
that's fine with you ?

Also, I can still do the refactoring before this patch but it will 
require more work since I'll have to diff the functions moved etc.. do 
you feel it's required to do so or the refactoring could be done after 
this patch ?


Thank you,
Antoine