[v7,3/8] Use xml-syscall to compare syscall numbers in arm_linux_sigreturn_return-addr.

  This patch changes checks for sigreturn and rt_sigreturn syscalls in
arm-linux-tdep.c from magic numbers to numbers computed from
syscall/arm-linux.xml.

It also adds a new function to xml-syscall.h/c to compare syscalls numbers
called is_syscall.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* arm-linux-tdep.c (arm_linux_sigreturn_return_addr): Use is_syscall.
	* xml-syscall.c (is_syscall): New function.
	* xml-syscall.h (is_syscall): New declaration.
---
 gdb/arm-linux-tdep.c |  5 ++++-
 gdb/xml-syscall.c    | 13 +++++++++++++
 gdb/xml-syscall.h    |  6 ++++++
 3 files changed, 23 insertions(+), 1 deletion(-)
  

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> It also adds a new function to xml-syscall.h/c to compare syscalls numbers
> called is_syscall.

Why don't we use existing get_syscall_by_name in
arm_linux_sigreturn_return_addr rather than adding a new function is_syscall?
  

On 12/11/2015 11:29 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> 
>> It also adds a new function to xml-syscall.h/c to compare syscalls numbers
>> called is_syscall.
> 
> Why don't we use existing get_syscall_by_name in
> arm_linux_sigreturn_return_addr rather than adding a new function is_syscall?
> 

I wonder whether going through xml is really best.  E.g., writing a syscall
call by name as a string is more typo prone than a macro constant.  If you
typo the string, you get a runtime error.  If you typo a macro you get a
compile time error.  You could fix that by adding macros like:

 #define ARM_SIGRETURN_NAME "sigreturn"

but then you might as well just do:

 #define ARM_SIGRETURN 119

and avoid the xml look up.  Syscall numbers are ABI, they can't ever change.

Thanks,
Pedro Alves
  

On 11/12/15 11:59, Pedro Alves wrote:
> but then you might as well just do:
>
>   #define ARM_SIGRETURN 119

How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
what patch #6 does.
  

On 12/11/2015 12:02 PM, Yao Qi wrote:
> On 11/12/15 11:59, Pedro Alves wrote:
>> but then you might as well just do:
>>
>>   #define ARM_SIGRETURN 119
> 
> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
> what patch #6 does.

Those are host/native macros.  Only make sense for the running host.
Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.

Thanks,
Pedro Alves
  

On 12/11/2015 06:29 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> It also adds a new function to xml-syscall.h/c to compare syscalls numbers
>> called is_syscall.
>
> Why don't we use existing get_syscall_by_name in
> arm_linux_sigreturn_return_addr rather than adding a new function is_syscall?
>

This is because it complicated the code a lot just to compare a syscall 
I have to do what is in the is_syscall function.

And I have to do that over and over... so might as well be in a function ?
  

On 12/11/2015 07:05 AM, Pedro Alves wrote:
> On 12/11/2015 12:02 PM, Yao Qi wrote:
>> On 11/12/15 11:59, Pedro Alves wrote:
>>> but then you might as well just do:
>>>
>>>    #define ARM_SIGRETURN 119
>>
>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>> what patch #6 does.
>
> Those are host/native macros.  Only make sense for the running host.
> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>

Exactly that's why I use those only in GDBServer.

Personally while I like the #define ARM_SIGRETURN 119 better, I think 
the xml function is fine.
  

On 12/11/2015 12:31 PM, Antoine Tremblay wrote:
> 
> 
> On 12/11/2015 07:05 AM, Pedro Alves wrote:
>> On 12/11/2015 12:02 PM, Yao Qi wrote:
>>> On 11/12/15 11:59, Pedro Alves wrote:
>>>> but then you might as well just do:
>>>>
>>>>    #define ARM_SIGRETURN 119
>>>
>>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>>> what patch #6 does.
>>
>> Those are host/native macros.  Only make sense for the running host.
>> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
>> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>>
> 
> Exactly that's why I use those only in GDBServer.

Hmm, actually, that sounds wrong.

What about Aarch64 gdbserver debugging Aarch32?

I see that the new code in question in patch #6 is in gdbserver/linux-arm-low.c,
which is not built on Aarch64, but, shouldn't that new arm_sigreturn_next_pc
code be used in the biarch scenario as well?

If you just made that 32-bit-specific code compile on Aarch64 it would
be compiling against Aarch64's __NR_sigreturn, which I'd assume is wrong for Aarch32.

> 
> Personally while I like the #define ARM_SIGRETURN 119 better, I think 
> the xml function is fine.
> 

If you added

#define ARM_LINUX_SIGRETURN 119

to arch/arm-linux.h

you could use it in both places.

Thanks,
Pedro Alves
  

On 12/11/2015 07:47 AM, Pedro Alves wrote:
> On 12/11/2015 12:31 PM, Antoine Tremblay wrote:
>>
>>
>> On 12/11/2015 07:05 AM, Pedro Alves wrote:
>>> On 12/11/2015 12:02 PM, Yao Qi wrote:
>>>> On 11/12/15 11:59, Pedro Alves wrote:
>>>>> but then you might as well just do:
>>>>>
>>>>>     #define ARM_SIGRETURN 119
>>>>
>>>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>>>> what patch #6 does.
>>>
>>> Those are host/native macros.  Only make sense for the running host.
>>> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
>>> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>>>
>>
>> Exactly that's why I use those only in GDBServer.
>
> Hmm, actually, that sounds wrong.
>
> What about Aarch64 gdbserver debugging Aarch32?
>
> I see that the new code in question in patch #6 is in gdbserver/linux-arm-low.c,
> which is not built on Aarch64, but, shouldn't that new arm_sigreturn_next_pc
> code be used in the biarch scenario as well?
>

This is only used if we need software single stepping, I think in that 
case hardware single stepping will be used on aarch64 even if stepping 
over an arm32 program, Yao can confirm ?


> If you just made that 32-bit-specific code compile on Aarch64 it would
> be compiling against Aarch64's __NR_sigreturn, which I'd assume is wrong for Aarch32.
>
>>

Looking at the kernel code in linux/arch/arm64 I see :

include/asm/unistd.h:#define __NR_compat_sigreturn		119
include/asm/unistd.h:#define __NR_compat_rt_sigreturn	173

include/asm/unistd32.h:#define __NR_sigreturn 119
include/asm/unistd32.h:__SYSCALL(__NR_sigreturn, 
compat_sys_sigreturn_wrapper)

include/asm/unistd32.h:#define __NR_rt_sigreturn 173
include/asm/unistd32.h:__SYSCALL(__NR_rt_sigreturn, 
compat_sys_rt_sigreturn_wrapper)

So I think it's ok... but I can't compile on aarch64 at the moment.

>> Personally while I like the #define ARM_SIGRETURN 119 better, I think
>> the xml function is fine.
>>
>
> If you added
>
> #define ARM_LINUX_SIGRETURN 119
>
> to arch/arm-linux.h
>
> you could use it in both places.

That was my first thought, I would be OK with that if Yao is too, 
however I think both methods work.
  

On 12/11/2015 01:04 PM, Antoine Tremblay wrote:
> 
> 
> On 12/11/2015 07:47 AM, Pedro Alves wrote:
>> On 12/11/2015 12:31 PM, Antoine Tremblay wrote:
>>>
>>>
>>> On 12/11/2015 07:05 AM, Pedro Alves wrote:
>>>> On 12/11/2015 12:02 PM, Yao Qi wrote:
>>>>> On 11/12/15 11:59, Pedro Alves wrote:
>>>>>> but then you might as well just do:
>>>>>>
>>>>>>     #define ARM_SIGRETURN 119
>>>>>
>>>>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>>>>> what patch #6 does.
>>>>
>>>> Those are host/native macros.  Only make sense for the running host.
>>>> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
>>>> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>>>>
>>>
>>> Exactly that's why I use those only in GDBServer.
>>
>> Hmm, actually, that sounds wrong.
>>
>> What about Aarch64 gdbserver debugging Aarch32?
>>
>> I see that the new code in question in patch #6 is in gdbserver/linux-arm-low.c,
>> which is not built on Aarch64, but, shouldn't that new arm_sigreturn_next_pc
>> code be used in the biarch scenario as well?
>>
> 
> This is only used if we need software single stepping, I think in that 
> case hardware single stepping will be used on aarch64 even if stepping 
> over an arm32 program, Yao can confirm ?

Ah, I think you're right.

> 
> 
>> If you just made that 32-bit-specific code compile on Aarch64 it would
>> be compiling against Aarch64's __NR_sigreturn, which I'd assume is wrong for Aarch32.
>>
>>>
> 
> Looking at the kernel code in linux/arch/arm64 I see :
> 
> include/asm/unistd.h:#define __NR_compat_sigreturn		119
> include/asm/unistd.h:#define __NR_compat_rt_sigreturn	173
> 
> include/asm/unistd32.h:#define __NR_sigreturn 119
> include/asm/unistd32.h:__SYSCALL(__NR_sigreturn, 
> compat_sys_sigreturn_wrapper)
> 
> include/asm/unistd32.h:#define __NR_rt_sigreturn 173
> include/asm/unistd32.h:__SYSCALL(__NR_rt_sigreturn, 
> compat_sys_rt_sigreturn_wrapper)
> 
> So I think it's ok... but I can't compile on aarch64 at the moment.

From that, it seems to be that Aarch64 64-bit doesn't even
have __NR_sigreturn.  The one we see above is in unistd32.h,
which should mean it's only included when compiling 32-bit code.
So it seems like if you tried to reference __NR_sigreturn on
Aarch64, you'd get a build failure.

Note there are Aarch64 Ubuntu machines on the gcc compile farm.

But, it's moot if we do hardware stepping.

Thanks,
Pedro Alves
  

On 12/11/2015 08:20 AM, Pedro Alves wrote:
> On 12/11/2015 01:04 PM, Antoine Tremblay wrote:

>  From that, it seems to be that Aarch64 64-bit doesn't even
> have __NR_sigreturn.  The one we see above is in unistd32.h,
> which should mean it's only included when compiling 32-bit code.
> So it seems like if you tried to reference __NR_sigreturn on
> Aarch64, you'd get a build failure.
>
> Note there are Aarch64 Ubuntu machines on the gcc compile farm.
>

Yes, one would need to use the _compat_ version I guess.

I'm still waiting for my request to the compile farm to be approved... :(

I do plan to buy an aarch64 however, not sure which one yet if ever 
anyone has experiences/suggestions on that ?

> But, it's moot if we do hardware stepping.
>

Indeed.

Thanks,
Antoine
  

On 12/11/2015 07:31 AM, Antoine Tremblay wrote:
>
>
> On 12/11/2015 07:05 AM, Pedro Alves wrote:
>> On 12/11/2015 12:02 PM, Yao Qi wrote:
>>> On 11/12/15 11:59, Pedro Alves wrote:
>>>> but then you might as well just do:
>>>>
>>>>    #define ARM_SIGRETURN 119
>>>
>>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>>> what patch #6 does.
>>
>> Those are host/native macros.  Only make sense for the running host.
>> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
>> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>>
>
> Exactly that's why I use those only in GDBServer.
>
> Personally while I like the #define ARM_SIGRETURN 119 better, I think
> the xml function is fine.

Any news about this one ?

Given that I use xml in GDB and __NR_sigreturn macros in GDBServer...

Is the patch OK ?

Thanks,
Antoine
  

On 12/11/2015 12:29 PM, Antoine Tremblay wrote:
> 
> 
> On 12/11/2015 06:29 AM, Yao Qi wrote:
>> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>>
>>> It also adds a new function to xml-syscall.h/c to compare syscalls numbers
>>> called is_syscall.
>>
>> Why don't we use existing get_syscall_by_name in
>> arm_linux_sigreturn_return_addr rather than adding a new function is_syscall?
>>
> 
> This is because it complicated the code a lot just to compare a syscall 
> I have to do what is in the is_syscall function.
> 
> And I have to do that over and over... so might as well be in a function ?
> 
> 

The issue is the having to handle the struct syscall instance, right?
So instead of is_syscall, you could add a function that returns the
syscall number directly, like:

 int
 get_syscall_number (struct gdbarch *gdbarch,
                     const char *syscall_name)
 {
   init_syscalls_info (gdbarch);

   return xml_get_syscall_number (gdbarch, syscall_name);
 }

and then users could use the more natural == comparison:

   /* Is this a sigreturn or rt_sigreturn syscall?  */
-  if (svc_number == 119 || svc_number == 173)
+  if (get_syscall_number (gdbarch, "sigreturn") == svc_number)
+      || get_syscall_number (gdbarch, "rt_sigreturn") == svc_number))
     {

and maybe other places could be simplified to use get_syscall_number
for other purposes than direct comparison.

Thanks,
Pedro Alves
  

On 12/15/2015 12:46 PM, Antoine Tremblay wrote:
> 
> 
> On 12/11/2015 07:31 AM, Antoine Tremblay wrote:
>>
>>
>> On 12/11/2015 07:05 AM, Pedro Alves wrote:
>>> On 12/11/2015 12:02 PM, Yao Qi wrote:
>>>> On 11/12/15 11:59, Pedro Alves wrote:
>>>>> but then you might as well just do:
>>>>>
>>>>>    #define ARM_SIGRETURN 119
>>>>
>>>> How about using __NR_sigreturn and __NR_rt_sigreturn directly? like
>>>> what patch #6 does.
>>>
>>> Those are host/native macros.  Only make sense for the running host.
>>> Can't use that in a tdep file, like arm-linux-tdep.c.  Otherwise, e.g.,
>>> a x86-hosted cross debugger would be using the x86 __NR_sigreturn, etc.
>>>
>>
>> Exactly that's why I use those only in GDBServer.
>>
>> Personally while I like the #define ARM_SIGRETURN 119 better, I think
>> the xml function is fine.
> 
> Any news about this one ?
> 
> Given that I use xml in GDB and __NR_sigreturn macros in GDBServer...
> 
> Is the patch OK ?

I'd rather leave the final call to Yao.

Thanks,
Pedro Alves
  

On 12/16/2015 10:59 AM, Pedro Alves wrote:
> On 12/11/2015 12:29 PM, Antoine Tremblay wrote:
>>
>>
>> On 12/11/2015 06:29 AM, Yao Qi wrote:
>>> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>>>
>>>> It also adds a new function to xml-syscall.h/c to compare syscalls numbers
>>>> called is_syscall.
>>>
>>> Why don't we use existing get_syscall_by_name in
>>> arm_linux_sigreturn_return_addr rather than adding a new function is_syscall?
>>>
>>
>> This is because it complicated the code a lot just to compare a syscall
>> I have to do what is in the is_syscall function.
>>
>> And I have to do that over and over... so might as well be in a function ?
>>
>>
>
> The issue is the having to handle the struct syscall instance, right?

Right.

> So instead of is_syscall, you could add a function that returns the
> syscall number directly, like:
>
>   int
>   get_syscall_number (struct gdbarch *gdbarch,
>                       const char *syscall_name)
>   {
>     init_syscalls_info (gdbarch);
>
>     return xml_get_syscall_number (gdbarch, syscall_name);
>   }
>
> and then users could use the more natural == comparison:
>
>     /* Is this a sigreturn or rt_sigreturn syscall?  */
> -  if (svc_number == 119 || svc_number == 173)
> +  if (get_syscall_number (gdbarch, "sigreturn") == svc_number)
> +      || get_syscall_number (gdbarch, "rt_sigreturn") == svc_number))
>       {
>
> and maybe other places could be simplified to use get_syscall_number
> for other purposes than direct comparison.

I like the idea. I'll redo it like that.

Thanks,
Antoine
  

On 15/12/15 12:46, Antoine Tremblay wrote:
> Any news about this one ?
>
> Given that I use xml in GDB and __NR_sigreturn macros in GDBServer...
>
> Is the patch OK ?

Sorry, which patch do you mean?  Is it this one
https://sourceware.org/ml/gdb-patches/2015-12/msg00303.html ?
  

On 12/16/2015 11:35 AM, Yao Qi wrote:
> On 15/12/15 12:46, Antoine Tremblay wrote:
>> Any news about this one ?
>>
>> Given that I use xml in GDB and __NR_sigreturn macros in GDBServer...
>>
>> Is the patch OK ?
>
> Sorry, which patch do you mean?  Is it this one
> https://sourceware.org/ml/gdb-patches/2015-12/msg00303.html ?
>

Yes that's the newest one indeed.