Message ID | 1449583641-18156-4-git-send-email-antoine.tremblay@ericsson.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 38751 invoked by alias); 8 Dec 2015 14:07:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 38626 invoked by uid 89); 8 Dec 2015 14:07:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: usplmg21.ericsson.net Received: from usplmg21.ericsson.net (HELO usplmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 08 Dec 2015 14:07:34 +0000 Received: from EUSAAHC001.ericsson.se (Unknown_Domain [147.117.188.75]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id D0.48.32102.D14E6665; Tue, 8 Dec 2015 15:07:26 +0100 (CET) Received: from elxa4wqvvz1.dyn.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 8 Dec 2015 09:07:29 -0500 From: Antoine Tremblay <antoine.tremblay@ericsson.com> To: <gdb-patches@sourceware.org> CC: Antoine Tremblay <antoine.tremblay@ericsson.com> Subject: [PATCH v7 3/8] Use xml-syscall to compare syscall numbers in arm_linux_sigreturn_return-addr. Date: Tue, 8 Dec 2015 09:07:16 -0500 Message-ID: <1449583641-18156-4-git-send-email-antoine.tremblay@ericsson.com> In-Reply-To: <1449583641-18156-1-git-send-email-antoine.tremblay@ericsson.com> References: <1449583641-18156-1-git-send-email-antoine.tremblay@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
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
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.
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 */