Patchwork Allow IRIX Ada bootstrap with C++

login
register
mail settings
Submitter Rainer Orth
Date July 20, 2011, 3:58 p.m.
Message ID <yddfwm1lz9i.fsf@manam.CeBiTec.Uni-Bielefeld.DE>
Download mbox | patch
Permalink /patch/105755/
State New
Headers show

Comments

Rainer Orth - July 20, 2011, 3:58 p.m.
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.
Arnaud Charlet - July 20, 2011, 4:06 p.m.
> With this patch, bootstrap continued.  Ok for mainline if it passes?

OK, thanks.

Arno
Eric Botcazou - July 23, 2011, 7:37 p.m.
> 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.
Rainer Orth - July 25, 2011, 5:50 p.m.
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
Eric Botcazou - July 25, 2011, 9:34 p.m.
> 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.
Rainer Orth - July 26, 2011, 9:14 a.m.
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
Eric Botcazou - July 26, 2011, 9:22 a.m.
> 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.

Patch

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);