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

[Antoine Tremblay <antoine.tremblay@ericsson.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.