Re: Allow IRIX Ada bootstrap with C++

Re: Allow IRIX Ada bootstrap with C++

[Arnaud Charlet]

> With this patch, bootstrap continued.  Ok for mainline if it passes?

OK, thanks.

Arno

Allow IRIX Ada bootstrap with C++

[Rainer Orth]

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.

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

Re: Allow IRIX Ada bootstrap with C++

[Eric Botcazou]

> 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

Re: Allow IRIX Ada bootstrap with C++

[Rainer Orth]

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

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Allow IRIX Ada bootstrap with C++

[Eric Botcazou]

> 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

Re: Allow IRIX Ada bootstrap with C++

[Rainer Orth]

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

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Allow IRIX Ada bootstrap with C++

[Eric Botcazou]

> 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.

-- 
Eric Botcazou

Re: Allow IRIX Ada bootstrap with C++

[Rainer Orth]

Eric Botcazou <ebotcazou@adacore.com> writes:

>> 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.

Right.  Even if I can cope with that, I haven't been able to extract all
the required info (pc, gregs, fpregs) from ucontext_t/mcontext_t with
SA_SIGINFO set.  Besides, it doesn't seem possible to distinguish
between the two cases (sa_handler/sa_sigaction).  Therefore I went back
for the following hack, adding comments to explain why it is necessary.

Bootstrapped without regressions on mips-sgi-irix6.5, all signal
handling failures introduced by my previous patch are gone again.

Ok for mainline?

	Rainer


2011-07-26  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* init.c (__gnat_error_handler): Cast reason to int.
	(__gnat_install_handler): Explain sa_sigaction use.

diff --git a/gcc/ada/init.c b/gcc/ada/init.c
--- a/gcc/ada/init.c
+++ b/gcc/ada/init.c
@@ -787,7 +787,11 @@ extern struct Exception_Data _abort_sign
 static void
 __gnat_error_handler (int sig, siginfo_t *reason, void *uc ATTRIBUTE_UNUSED)
 {
-  int code = reason == NULL ? 0 : reason->si_code;
+  /* This handler is installed with SA_SIGINFO cleared, but there's no
+     prototype for the resulting alternative three-argument form, so we
+     have to hack around this by casting reason to the int actually
+     passed.  */
+  int code = (int) reason;
   struct Exception_Data *exception;
   const char *msg;
 
@@ -872,7 +876,11 @@ __gnat_install_handler (void)
 
   /* Setup signal handler to map synchronous signals to appropriate
      exceptions.  Make sure that the handler isn't interrupted by another
-     signal that might cause a scheduling event!  */
+     signal that might cause a scheduling event!
+
+     The handler is installed with SA_SIGINFO cleared, but there's no
+     C++ prototype for the three-argument form, so fake it by using
+     sa_sigaction and casting the arguments instead.  */
 
   act.sa_sigaction = __gnat_error_handler;
   act.sa_flags = SA_NODEFER + SA_RESTART;

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Allow IRIX Ada bootstrap with C++

[Arnaud Charlet]

> Bootstrapped without regressions on mips-sgi-irix6.5, all signal
> handling failures introduced by my previous patch are gone again.
> 
> Ok for mainline?

OK.

Re: Allow IRIX Ada bootstrap with C++

[Andreas Schwab]

Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

> diff --git a/gcc/ada/init.c b/gcc/ada/init.c
> --- a/gcc/ada/init.c
> +++ b/gcc/ada/init.c
> @@ -787,7 +787,11 @@ extern struct Exception_Data _abort_sign
>  static void
>  __gnat_error_handler (int sig, siginfo_t *reason, void *uc ATTRIBUTE_UNUSED)
>  {
> -  int code = reason == NULL ? 0 : reason->si_code;
> +  /* This handler is installed with SA_SIGINFO cleared, but there's no
> +     prototype for the resulting alternative three-argument form, so we
> +     have to hack around this by casting reason to the int actually
> +     passed.  */
> +  int code = (int) reason;
>    struct Exception_Data *exception;
>    const char *msg;
>  
> @@ -872,7 +876,11 @@ __gnat_install_handler (void)
>  
>    /* Setup signal handler to map synchronous signals to appropriate
>       exceptions.  Make sure that the handler isn't interrupted by another
> -     signal that might cause a scheduling event!  */
> +     signal that might cause a scheduling event!
> +
> +     The handler is installed with SA_SIGINFO cleared, but there's no
> +     C++ prototype for the three-argument form, so fake it by using
> +     sa_sigaction and casting the arguments instead.  */
>  
>    act.sa_sigaction = __gnat_error_handler;
>    act.sa_flags = SA_NODEFER + SA_RESTART;

Wouldn't it be cleanest to adjust the prototype of __gnat_error_handler
to reality, and cast it when assigning to sa_handler (not sa_sigaction,
which is only valid if SA_SIGINFO is set)?

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: Allow IRIX Ada bootstrap with C++

[Rainer Orth]

Andreas,

> Wouldn't it be cleanest to adjust the prototype of __gnat_error_handler
> to reality, and cast it when assigning to sa_handler (not sa_sigaction,
> which is only valid if SA_SIGINFO is set)?

probably, provided g++ accepts that.  I'd have to run two bootstraps (C
and C++) to check this.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University