RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

Hi Magnus,

I appreciate what you are trying to do here but there are reasons this 
kind of cleanup keeps getting deferred :) - it's not trivial and deals 
with really old code that isn't always clear. You may have bitten off 
more than you want to chew here. (And I'm strapped for time to work 
through this with you - sorry.)

On the positive side you can use the pthread functions on Solaris so no 
need to special case for that.

However there are still a number of issues:

   52 #ifndef NSIG
   53   #define NSIG SIGRTMAX
   54 #endif

SIGRTMAX is Solaris only, but even on Solaris NSIG might be defined 
(it's conditional in sys/signal.h but I don't know when the conditions 
are enabled).

  56 static struct sigaction sact[NSIG];

SIGRTMAX is a macro not a constant so you can't declare a static array 
using it. You would need to malloc the array as done on Solaris.

  58 static __thread bool reentry = false; /* prevent reentry deadlock 
(per-thread) */

I didn't realize this was in the OSX version as we don't unconditionally 
use compiler-based thread locals. I would make this field and its use 
OSX specific

148 #ifdef SOLARIS
  149     sact[sig].sa_flags = SA_NODEFER;
  150     if (sig != SIGILL && sig != SIGTRAP && sig != SIGPWR) {
  151       sact[sig].sa_flags |= SA_RESETHAND;
  152     }
  153 #else

I'd have to do some research to see why this is needed so best to keep it.

178 #ifdef SOLARIS
  179     if (is_sigset && sigblocked) {
  180       /* We won't honor the SIG_HOLD request to change the signal 
mask */
  181       oldhandler = SIG_HOLD;
  182     }
  183 #endif

Ditto.

More below ...

On 27/03/2018 7:42 PM, Magnus Ihse Bursie wrote:
> When I was about to update jsig.c, I noticed that the four copies for 
> aix, linux, macosx and solaris were basically the same, with only small 
> differences. Some of the differences were not even well motivated, but 
> were likely the result of this code duplication causing the code to 
> diverge.
> 
> A better solution is to unify them all into a single unix version, with 
> the few platform-specific changes handled on a per-platform basis.
> 
> I've made the following notable changes:
> 
> * I have removed the version check for Solaris. All other platforms seem 
> to do fine without it, and in general, we don't mistrust other JDK 
> libraries. An alternative is to add this version check to all other 
> platforms instead. If you think this is the correct course of action, 
> let me know and I'll fix it.

That's fine. I actually thought this had gone. Or it may be we have a 
bug to clean this up. Which reminds me - there may be a number of 
existing bugs that get handled by the changes you're making. But there 
may still be open bugs that need fixing.

> * Solaris used to have a dynamically allocated sact, instead of a 
> statically allocated array as all other platforms have. It's not likely 
> to be large, and the size is known at compile time, so there seems to be 
> no good reason for this.

See above. If you didn't get a compile-time error here then it's likely 
NSIG was already defined.

 > cat sig.c
#include <signal.h>

static int sigs[SIGRTMAX];

int main(int argc, char* argv[]) {
   sigs[0] = 1;
}

  > CC -c sig.c
"sig.c", line 3: Error: An integer constant expression is required 
within the array subscript operator.
1 Error(s) detected.


> * Linux and macosx used a uint32_t/uint64_t instead of sigset_t for 
> jvmsigs, as aix and solaris do. This is a less robust solution, and the 
> added checks show that it has failed in the past. Now all platforms use 
> sigset_t/sigismember().

Probably historical (macosx copied linux) and a good change to make. 
That said I have to wonder why we didn't make it when we put in the 
guards on the size.

> Also worth noting:
> 
> * Solaris is not using pthreads, but it's own thread library, which 
> accounts for most of the #ifdef SOLARIS.

Not needed as above.

> * In general, if an implementation was needed on one platform, but has 
> no effect or is harmless on others, I've kept it on all platforms 
> instead of sprinkling the code with #ifdefs.

That depends on your own knowledge/assumptions about the behaviour. I'd 
want to check each such case to verify that is the case. As above the 
use of __thread with "reentry" is not something to use unconditionally 
without closer examination.

> To facilitate code review, here is a specially crafted webrev that shows 
> the differences compared to each of the individual, original per-OS 
> versions of jsig.c:
> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01

Appreciated, but still very hard to see when a feature from one OS is 
now being used by all.

I'd be less concerned if we had extensive testing here but I don't think 
we do, so any issues only turn up when something that's been working 
since Java 1.4 suddenly stops working. What testing was done?

Thanks,
David
-----

> Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
> WebRev: 
> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03
> 
> /Magnus