| Message ID | 1457090537-23825-1-git-send-email-vgupta@synopsys.com |
|---|---|
| State | Superseded |
| Headers | show |
On Monday 07 March 2016 03:45 PM, Cyril Hrubis wrote: > Hi! >> To me all the song-and-dance in ltp_rt_sigaction() doesn't make sense. > These are required if you call the raw rt_sigaction syscall. Since > before signal handler is called the process context is saved on the > user-space stack and once signal is done it has to be restored. In order > to do that signal trampoline is called which calls sigreturn to restore > the context. And the way trapoline is executed is platform specific and > sometimes in the sa_restorer. I'm well aware of all that having fixed a bunch of kernel issues in this exact area back in 2008 and also added the uClibc userspace default sigrestorer.... The point is why is LTP calling raw rt_sigaction syscall at all. It needs to call sigaction which is the posix API and see how it behaves. I can understand the usage of raw syscalls for things which won't be present in a libc (e.g newer syscalls or perhaps obsoleted syscalls). sigaction is fairly standard. No ? >> The intent it to test whatever platform libc + kernel combination >> provides and to that end any fixups in LTP seem odd to me. >> So for ARC just call what libc provides and don't claim to support >> more/less. > These testcases are testing kernel syscall and some of the testcases > will not work with the libc wrapper. I understand you want to test the -EFAULT code path in the kernel - but any arch with default sa restorer in libc can't do that at all. They have to provide the default sa restorer to successfully return from signal handler to invoke the sigreturn syscall. Look in uClibc - ARC is not the exception: ARM, Xtensa, MIPS, x86 all use such userspace default restorers and are all plagued by same issue. Before this patch, we had rt_sigaction01 crash on ARC - this was because it passed the test itself, but for sigreturn it went off to lala land since there was no restorer. So it failed anyways. With this patch we have that as passing: actually 4 passes, 2 failures. I don't care abt this +4/-2. More importantly this is true reflection of how ARC GNU stack behaves. If you call sigaction(num, -1, 0) it will NOT return -EFAULT but crash. -rt_sigaction01 rt_sigaction01 -rt_sigprocmask01 rt_sigprocmask01 -rt_sigsuspend01 rt_sigsuspend01 +rt_sigaction02 rt_sigaction02 +rt_sigaction03 rt_sigaction03 -signal_test_04 signal_test_04 > So NACK on the patch. But this patch only changes how ARC works - so whats the problem ? There are already arch specifix quirks in this file, there so how does one more hurt. -Vineet
On Monday 07 March 2016 04:29 PM, Cyril Hrubis wrote: > We also check that we got EINVAL if the sigsetsize is wrong, which > cannot be done with the libc wrapper. > > And we several testscases for the libc wrapper as well. Two in > testcases/syscalls/sigaction/ directory and much more in > testcases/open_posix_testsuite/conformance/interfaces/sigaction/. > > So both raw syscall and libc call are covered in LTP. Fair enough - so LTP was a "Linux" test project - testing the raw kernel ABI makes sense to check for correctness. Although users will complain that sigaction(n, -1, 0) doesn't return with -EFAULT but crashes. Oh well ! >>>> So for ARC just call what libc provides and don't claim to support >>>> more/less. > I do not understand, looking at: > > https://github.com/foss-for-synopsys-dwc-arc-processors/uClibc/blob/314dd3440686d6225ef1b93b5c5bdce852747a6b/libc/sysdeps/linux/arc/sigaction.c > > All you have to do in sa_restorer for arc is to call __NR_rt_sigreturn. > > Looking at the INTERNAL_SYSCALL_NCS it's just a few lines of inline > assembler. So all you have to do to support this properly in LTP is to > create restorer function taking the assembler and passing NR_sigreturn > as syscall number to it. Will do in next patch ! Thx, -Vineet
diff --git a/include/lapi/rt_sigaction.h b/include/lapi/rt_sigaction.h index 3a5a763ce094..12af3c3b7b27 100644 --- a/include/lapi/rt_sigaction.h +++ b/include/lapi/rt_sigaction.h @@ -155,6 +155,13 @@ static void __attribute__((used)) __sigreturn_stub(void) static int ltp_rt_sigaction(int signum, const struct sigaction *act, struct sigaction *oact, size_t sigsetsize) { +#ifdef __arc__ + /* + * No playing games with various internals of sigaction / fields + * just use whatever libc + kernel provide + */ + return sigaction(signum, act, oact); +#else int ret; struct kernel_sigaction kact, koact; struct kernel_sigaction *kact_p = NULL; @@ -218,6 +225,7 @@ static int ltp_rt_sigaction(int signum, const struct sigaction *act, } return ret; +#endif } #endif /* LTP_RT_SIGACTION_H */
ARC uses the no-legacy syscall ABI where there is no need for kernel_sigaction interworking. Further we rely on default SA_RESTORER in libc for sigreturn. To me all the song-and-dance in ltp_rt_sigaction() doesn't make sense. The intent it to test whatever platform libc + kernel combination provides and to that end any fixups in LTP seem odd to me. So for ARC just call what libc provides and don't claim to support more/less. e.g. rt_sigaction02 fails for ARC, since libc sigaction does following | int __libc_sigaction (int sig, const struct sigaction *act, struct sigaction *oact) | { | struct sigaction kact; | | if (act && !(act->sa_flags & SA_RESTORER)) { | kact.sa_restorer = __default_rt_sa_restorer; So we don't get the EFAULT for @act == -1 which kernel would provide. Instead we get a segv rightawat. But that's fine, that is how our system works. Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- include/lapi/rt_sigaction.h | 8 ++++++++ 1 file changed, 8 insertions(+)