[m-dev.] for review: cleanup signal setting.

[Fergus Henderson]

On 18-May-1998, Tyson Dowd <trd at stimpy.cs.mu.oz.au> wrote:
> On 15-May-1998, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > On 14-May-1998, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> > > -static void
> > > -checked_signal(int sig, void (*handler)(int))
> > > -{
> > > -	/*
> > > -	** We really need sigaction and SA_RESTART, otherwise profiling signals
> > > -	** might interrupt I/O, causing a profiled program to get I/O errors.
> > > -	** But if we haven't got it, I guess we just have to punt...
> > > -	*/
> > > -#ifndef SA_RESTART
> > > -#define SA_RESTART 0
> > > -#endif
> > 
> > I don't think your log message provided any rationale for removing that
> > bit of code.  Did that just get unintentionally lost in the reorganization?
> 
> No, it didn't include any rationale, but it wasn't unintentional.
> 
> My rationale was:
> 
> 	- The given comment did not justify defining SA_RESTART.

Well, the reason is that SA_RESTART is used by the code below.

> 	- The code for profiling included this definition, but it was
> 	  not used in setup_signals for the memory zones. 

I think it was.  Let me quote your diff:

-       act.sa_flags = SA_SIGINFO | SA_RESTART;
-       if (sigemptyset(&act.sa_mask) != 0) {
-               perror("Mercury runtime: cannot set clear signal mask");
-               exit(1);
-       }
-
-       act.SIGACTION_FIELD = complex_bushandler;
-       if (sigaction(SIGBUS, &act, NULL) != 0) {
-               perror("Mercury runtime: cannot set SIGBUS handler");
-               exit(1);
-       }
-
-       act.SIGACTION_FIELD = complex_segvhandler;
-       if (sigaction(SIGSEGV, &act, NULL) != 0) {
-               perror("Mercury runtime: cannot set SIGSEGV handler");
-               exit(1);
-       }

> 	- Its abscence never caused problems in setup_signals.
> 	- I assume this defintion was added because of a missing #include
> 	  or somesuch, which has since gone away.
> 	- If there was a more serious problem, I expect it would be
> 	  commented or occur elsewhere.
> 	- If removing this code causes a problem, I'll at least be able
> 	  to diagnose it, and then explain the SA_RESTART defintion
> 	  in a comment.

The reason that removing this code didn't cause any problems is because
the code in configure.in which tests for sigaction() actually includes
a check for SA_RESTART.

This is arguably a bug, because it means that on systems
which support sigaction() with extended info about fault
address etc. but not SA_RESTART, Mercury will not make
use of the extended info but instead will just use
signal() instead of sigaction().
I don't know off-hand if there are any systems which
fall into this category, but sigaction() is part of POSIX.1,
whereas SA_RESTART is not, so such systems are not unlikely.

We should add the

	#ifndef SA_RESTART 
	#define SA_RESTART 0
	#endif

lines to the various places in configure.in which use sigaction(),
and put it back in runtime/mercury_memory_handlers.c.
(Could you please do this?)

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.