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

[Tyson Dowd]

On 18-May-1998, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> 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.

But this is silently doing the wrong thing!  Time profiling needs
SA_RESTART.  If we don't have it, you'll get occasional errors
while profiling.   Silently setting SA_RESTART is just asking for
someone to go nuts trying to find this bug in future.

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

There is *no* definition there.  If SA_RESTART is not defined, we
will get a compile error.  Nobody has complained about such errors
in the past, so I am fairly sure SA_RESTART is defined on all
systems we have used so far.

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

Yes, this might be a problem.  We should add another test
to configuration to see if this is the case, and handle it
differently.  For example, in this case time profiling might
not be possible.  

But unless we have a problem with this, it looks like work
that we don't have to do, and we can't test anyway.

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

Not until you've convinced me this is the right thing to do.
So far it looks like it's a kludge that might cause errors, and
doesn't solve any existing problems.

-- 
       Tyson Dowd           # So I asked Sarah: what's the serial number on
                            # your computer? She replied:
     trd at cs.mu.oz.au        #          A-C-2-4-0-V-/-5-0-H-Z
http://www.cs.mu.oz.au/~trd #