| Message ID | yddfwm1lz9i.fsf@manam.CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
> With this patch, bootstrap continued. Ok for mainline if it passes?
OK, thanks.
Arno
> 2011-07-20 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > * init.c [sgi] (__gnat_error_handler): Update sigaction(2) citation. > Correct argument types. > Extract code from reason. > (__gnat_install_handler): Assign to act.sa_sigaction. This breaks signal handling on our IRIX 6.5 machine though.
Eric Botcazou <ebotcazou@adacore.com> writes: >> 2011-07-20 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> >> >> * init.c [sgi] (__gnat_error_handler): Update sigaction(2) citation. >> Correct argument types. >> Extract code from reason. >> (__gnat_install_handler): Assign to act.sa_sigaction. > > This breaks signal handling on our IRIX 6.5 machine though. Same for me ;-( As already noted in the patch submission, there's something fishy going on with the IRIX sighandler stuff: gcc/ada/init.c (__gnat_install_handler) explicitly does not include SA_SIGINFO in sa_flags, which means the handler only gets one arg, sig. Still the installed handler (__gnat_error_handler) accesses args beyond the first. There are two possible solutions: * Actually set SA_SIGINFO. * Punt and cast the second __gnat_error_handler `arg' to an int. Running under gdb, it seems that three args are really passed. I prefer the first, since that's the clean solution. Unfortunately, my question why the current code doesn't set SA_SIGINFO, yet cites a considerable part of the man page about its effects, remained unanswered when I submitted the patch. Both solutions do work for a simple test of the 32-bit null_pointer_deref1 test, but I'll have to perform a full testsuite run and also adapt libjava/include/posix-signals.h. I remember that there were problems when I set SA_SIGINFO there, so maybe irix6-unwind.h needs updates to cope with that. Rainer
> gcc/ada/init.c (__gnat_install_handler) explicitly does not include > SA_SIGINFO in sa_flags, which means the handler only gets one arg, sig. > Still the installed handler (__gnat_error_handler) accesses args beyond > the first. Why would it get only one arg? The comment explicitly says that it gets three. Or is the prototype bogus in this case? > There are two possible solutions: > > * Actually set SA_SIGINFO. > > * Punt and cast the second __gnat_error_handler `arg' to an int. > Running under gdb, it seems that three args are really passed. Why does it not work to just change act.sa_handler to act.sa_sigaction? > I prefer the first, since that's the clean solution. Unfortunately, my > question why the current code doesn't set SA_SIGINFO, yet cites a > considerable part of the man page about its effects, remained unanswered > when I submitted the patch. I'd go for the minimal change that works, including an ugly cast somewhere.
Eric Botcazou <ebotcazou@adacore.com> writes: >> gcc/ada/init.c (__gnat_install_handler) explicitly does not include >> SA_SIGINFO in sa_flags, which means the handler only gets one arg, sig. >> Still the installed handler (__gnat_error_handler) accesses args beyond >> the first. > > Why would it get only one arg? The comment explicitly says that it gets three. > Or is the prototype bogus in this case? It gets three *if SA_SIGINFO is set in act.sa_flags*, which is is not. >> There are two possible solutions: >> >> * Actually set SA_SIGINFO. >> >> * Punt and cast the second __gnat_error_handler `arg' to an int. >> Running under gdb, it seems that three args are really passed. > > Why does it not work to just change act.sa_handler to act.sa_sigaction? That's what I did in my last patch, but without SA_SIGINFO set. This doesn't work since the additional args passed in the sa_handler case are not in any prototype, to g++ rightly complains (and this is an implementation detail I'd not rely upon if it can be avoided). >> I prefer the first, since that's the clean solution. Unfortunately, my >> question why the current code doesn't set SA_SIGINFO, yet cites a >> considerable part of the man page about its effects, remained unanswered >> when I submitted the patch. > > I'd go for the minimal change that works, including an ugly cast somewhere. I'm just testing the SA_SIGINFO path, which seems the correct way if it works. Rainer
> It gets three *if SA_SIGINFO is set in act.sa_flags*, which is is not. That isn't what the comment says though: SA_SIGINFO [...] If cleared and the signal is caught, the first argument is also the signal number but the second argument is the signal code identifying the cause of the signal. The third argument points to a sigcontext_t structure containing the receiving process's context when the signal was delivered. */ > That's what I did in my last patch, but without SA_SIGINFO set. This > doesn't work since the additional args passed in the sa_handler case are > not in any prototype, to g++ rightly complains (and this is an > implementation detail I'd not rely upon if it can be avoided). OK, I see, so there is a single prototype for the 2 variants with 3 args.
diff --git a/gcc/ada/init.c b/gcc/ada/init.c --- a/gcc/ada/init.c +++ b/gcc/ada/init.c @@ -763,16 +763,31 @@ extern struct Exception_Data _abort_sign connecting that handler, with the effects described in the sigaction man page: - SA_SIGINFO [...] - If cleared and the signal is caught, the first argument is - also the signal number but the second argument is the signal - code identifying the cause of the signal. The third argument - points to a sigcontext_t structure containing the receiving - process's context when the signal was delivered. */ + SA_SIGINFO If set and the signal is caught, sig is passed as the + first argument to the signal-catching function. If the + second argument is not equal to NULL, it points to a + siginfo_t structure containing the reason why the + signal was generated [see siginfo(5)]; the third + argument points to a ucontext_t structure containing + the receiving process's context when the signal was + delivered [see ucontext(5)]. If cleared and the signal + is caught, the first argument is also the signal number + but the second argument is the signal code identifying + the cause of the signal. The third argument points to a + sigcontext_t structure containing the receiving + process's context when the signal was delivered. This + is the default behavior (see signal(5) for more + details). Additionally, when SA_SIGINFO is set for a + signal, multiple occurrences of that signal will be + queued for delivery in FIFO order (see sigqueue(3) for + a more detailed explanation of this concept), if those + occurrences of that signal were generated using + sigqueue(3). */ static void -__gnat_error_handler (int sig, int code, sigcontext_t *sc ATTRIBUTE_UNUSED) +__gnat_error_handler (int sig, siginfo_t *reason, void *uc ATTRIBUTE_UNUSED) { + int code = reason == NULL ? 0 : reason->si_code; struct Exception_Data *exception; const char *msg; @@ -859,7 +874,7 @@ __gnat_install_handler (void) exceptions. Make sure that the handler isn't interrupted by another signal that might cause a scheduling event! */ - act.sa_handler = __gnat_error_handler; + act.sa_sigaction = __gnat_error_handler; act.sa_flags = SA_NODEFER + SA_RESTART; sigfillset (&act.sa_mask); sigemptyset (&act.sa_mask);
A bootstrap on IRIX 6.5 only saw a single issue with C++ so far: ada/init.c failed to compile: /vol/gcc/src/hg/trunk/local/gcc/ada/init.c: In function 'void __gnat_install_handler()': /vol/gcc/src/hg/trunk/local/gcc/ada/init.c:862:20: error: invalid conversion from 'void (*)(int, int, sigcontext_t*) {aka void (*)(int, int, sigcontext*)}' to 'void (*)(int)' [-fpermissive] make[3]: *** [ada/init.o] Error 1 There was obviously some confusion here. The __gnat_error_handler comment cites a section of the sigaction(2) man page describing handler arguments with SA_SIGINFO, only to state that this doesn't happen. Apart from that, the section matches neither the IRIX 5.3 nor the 6.5 man pages, which have different argument types. I'm updating the comment and fixing the argument types. I also now need to extract the code from the passed siginfo_t *. With this patch, bootstrap continued. Ok for mainline if it passes? I also wonder why __gnat_error_handler does all this stuff if SA_SIGINFO isn't set and the args never actually passed. Rainer 2011-07-20 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> * init.c [sgi] (__gnat_error_handler): Update sigaction(2) citation. Correct argument types. Extract code from reason. (__gnat_install_handler): Assign to act.sa_sigaction.