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
