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

Antoine Tremblay
2015-12-11T12:41:54Z

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.