Re: [PATCH v7 4/8] Refactor arm_software_single_step to use regcache.

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 12/11/2015 06:37 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> Hi Antoine,
> Patch is OK to me, two nits below.
>
> > diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> > index acb5701..eb4341c 100644
> > --- a/gdb/arm-linux-tdep.c
> > +++ b/gdb/arm-linux-tdep.c
> > @@ -808,6 +808,68 @@ arm_linux_sigreturn_return_addr (struct frame_info *frame,
> >     return 0;
> >   }
> >
> > +/* Calculate the offset from stack pointer of the pc register on the stack
> > +   in the case of a sigreturn or sigreturn_rt syscall.  */
> > +static int
> > +arm_linux_sigreturn_next_pc_offset (unsigned long sp,
> > +				    unsigned long sp_data,
> > +				    unsigned long svc_number,
> > +				    int is_sigreturn)
> > +{
> > +  /* Offset of R0 register.  */
> > +  int r0_offset = 0;
> > +  /* Offset of PC register.  */
> > +  int pc_offset = 0;
> > +
> > +  if (is_sigreturn)
> > +    {
> > +      if (sp_data == ARM_NEW_SIGFRAME_MAGIC)
> > +	r0_offset = ARM_UCONTEXT_SIGCONTEXT + ARM_SIGCONTEXT_R0;
> > +      else
> > +	r0_offset = ARM_SIGCONTEXT_R0;
> > +    }
> > +  else
> > +    {
> > +      if (sp_data == sp + ARM_OLD_RT_SIGFRAME_SIGINFO)
> > +	r0_offset = ARM_OLD_RT_SIGFRAME_UCONTEXT + ARM_UCONTEXT_SIGCONTEXT
> > +	  + ARM_SIGCONTEXT_R0;
> > +      else
> > +	r0_offset = ARM_NEW_RT_SIGFRAME_UCONTEXT + ARM_UCONTEXT_SIGCONTEXT
> > +	  + ARM_SIGCONTEXT_R0;
>
> Nit: we can write these long lines like this,
>
>        if (sp_data == sp + ARM_OLD_RT_SIGFRAME_SIGINFO)
>          r0_offset = ARM_OLD_RT_SIGFRAME_UCONTEXT;
>        else
>          r0_offset = ARM_NEW_RT_SIGFRAME_UCONTEXT;
>
>        r0_offset += (ARM_UCONTEXT_SIGCONTEXT + ARM_SIGCONTEXT_R0);

OK. I removed the parens however after r0_offset += ..

> > -  arm_linux_sigreturn_return_addr (frame, svc_number, &return_addr, &is_thumb);
> > +  if (is_syscall (gdbarch, "sigreturn", svc_number)
> > +      || is_syscall (gdbarch, "rt_sigreturn", svc_number))
> > +    next_pc = arm_linux_sigreturn_next_pc (regcache, svc_number);
> >
> >     /* Addresses for calling Thumb functions have the bit 0 set.  */
> >     if (is_thumb)
> > -    return_addr |= 1;
> > +    next_pc |= 1;
>
> Nit: use MAKE_THUMB_ADDR.

OK.