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

Antoine Tremblay
2015-12-07T14:48:58Z

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