diff mbox

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

Message ID 1449583641-18156-4-git-send-email-antoine.tremblay@ericsson.com
State New
Headers show

Commit Message

Antoine Tremblay Dec. 8, 2015, 2:07 p.m. UTC
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(-)

Comments

Yao Qi Dec. 11, 2015, 11:29 a.m. UTC | #1
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?
Pedro Alves Dec. 11, 2015, 11:59 a.m. UTC | #2
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
Yao Qi Dec. 11, 2015, 12:02 p.m. UTC | #3
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.
Pedro Alves Dec. 11, 2015, 12:05 p.m. UTC | #4
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
Antoine Tremblay Dec. 11, 2015, 12:29 p.m. UTC | #5
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 ?
Antoine Tremblay Dec. 11, 2015, 12:31 p.m. UTC | #6
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.
Pedro Alves Dec. 11, 2015, 12:47 p.m. UTC | #7
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
Antoine Tremblay Dec. 11, 2015, 1:04 p.m. UTC | #8
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.
Pedro Alves Dec. 11, 2015, 1:20 p.m. UTC | #9
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
Antoine Tremblay Dec. 11, 2015, 1:25 p.m. UTC | #10
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
Antoine Tremblay Dec. 15, 2015, 12:46 p.m. UTC | #11
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
Pedro Alves Dec. 16, 2015, 3:59 p.m. UTC | #12
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
Pedro Alves Dec. 16, 2015, 3:59 p.m. UTC | #13
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
Antoine Tremblay Dec. 16, 2015, 4:05 p.m. UTC | #14
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
Yao Qi Dec. 16, 2015, 4:35 p.m. UTC | #15
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 ?
Antoine Tremblay Dec. 16, 2015, 4:37 p.m. UTC | #16
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.
diff mbox

Patch

diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 73e1271..acb5701 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -788,8 +788,11 @@  arm_linux_sigreturn_return_addr (struct frame_info *frame,
 				 unsigned long svc_number,
 				 CORE_ADDR *pc, int *is_thumb)
 {
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+
   /* Is this a sigreturn or rt_sigreturn syscall?  */
-  if (svc_number == 119 || svc_number == 173)
+  if (is_syscall (gdbarch, "sigreturn", svc_number)
+      || is_syscall (gdbarch, "rt_sigreturn", svc_number))
     {
       if (get_frame_type (frame) == SIGTRAMP_FRAME)
 	{
diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
index 31a80a5..e6a6a2a 100644
--- a/gdb/xml-syscall.c
+++ b/gdb/xml-syscall.c
@@ -422,4 +422,17 @@  get_syscall_names (struct gdbarch *gdbarch)
   return xml_list_of_syscalls (gdbarch);
 }
 
+int
+is_syscall (struct gdbarch *gdbarch, const char *syscall_name,
+	    int syscall_number)
+{
+  struct syscall s;
+  get_syscall_by_name (gdbarch, syscall_name, &s);
+
+  if (s.number == syscall_number)
+    return 1;
+  else
+    return 0;
+}
+
 #endif /* ! HAVE_LIBEXPAT */
diff --git a/gdb/xml-syscall.h b/gdb/xml-syscall.h
index 55c9696..e232edc 100644
--- a/gdb/xml-syscall.h
+++ b/gdb/xml-syscall.h
@@ -50,4 +50,10 @@  void get_syscall_by_name (struct gdbarch *gdbarch,
 
 const char **get_syscall_names (struct gdbarch *gdbarch);
 
+/* Compare the syscall number with the syscall name given in argument.  If
+   they match return 1 otherwise return 0.  */
+
+int is_syscall (struct gdbarch *gdbarch, const char *syscall_name,
+		int syscall_number);
+
 #endif /* XML_SYSCALL_H */