Re: [PATCH v6 3/7] Refactor arm_software_single_step to use regcache.

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

On 12/07/2015 09:32 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +/* 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)
>> +{
>> +  /* Offset of R0 register.  */
>> +  int r0_offset = 0;
>> +  /* Offset of PC register.  */
>> +  int pc_offset = 0;
>> +
>> +  gdb_assert (svc_number == 119 || svc_number == 173);
>> +
>> +  /* sigreturn.  */
>> +  if (svc_number == 119)
>
> Can we get rid of these magic numbers?
>

Yes I can find a place to put a define, good idea.

>> +    {
>> +      if (sp_data == ARM_NEW_SIGFRAME_MAGIC)
>> +	r0_offset = ARM_UCONTEXT_SIGCONTEXT + ARM_SIGCONTEXT_R0;
>> +      else
>> +	r0_offset = ARM_SIGCONTEXT_R0;
>> +    }
>> +  /* rt_sigreturn. */
>> +  else if (svc_number == 173)
>> +    {
>> +      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;
>> +    }
>> +
>> +  pc_offset = r0_offset + 4 * 15;
>> +
>> +  return pc_offset;
>> +}
>> +
>
>> +
>>   /* At a ptrace syscall-stop, return the syscall number.  This either
>>      comes from the SWI instruction (OABI) or from r7 (EABI).
>>
>> @@ -862,21 +924,21 @@ arm_linux_get_syscall_number (struct gdbarch *gdbarch,
>>      instruction to be executed.  */
>>
>>   static CORE_ADDR
>> -arm_linux_syscall_next_pc (struct frame_info *frame)
>> +arm_linux_syscall_next_pc (struct regcache *regcache)
>>   {
>> -  CORE_ADDR pc = get_frame_pc (frame);
>> -  CORE_ADDR return_addr = 0;
>> -  int is_thumb = arm_frame_is_thumb (frame);
>> +  CORE_ADDR pc = regcache_read_pc (regcache);
>> +  CORE_ADDR next_pc = 0;
>> +  int is_thumb = arm_is_thumb (regcache);
>
> Nit: looks you rename return_addr to next_pc.  If you don't that, the patch
> can be shorter.  On the other hand, return_addr sounds a good variable
> name to me, which means the address after syscall returns.
>

Yes, I do since the function is called arm_linux_syscall*_next_pc*
and that is called from the get_next_pcs context.  It seems more 
consistent to me to refer to this address as the next program counter in 
that context.

Also I wanted to mark the difference between 
arm_linux_sigreturn_return_addr and arm_linux_sigreturn_next_pc.

Calling return_addr = arm_linux_sigreturn_next_pc seemed weird when 
there was return_addr = arm_linux_sigreturn_return_addr before.

In short I prefer to keep the function name consistent with the return 
value name.

Regards,
Antoine