On 2018-03-28 21:21, Thomas Stüfe wrote:
Hi Magnus,
I had a closer look at the changes, especially at jsig.c. It all comes
slowly back. The AIX version has been almost comically wrong.
About NSIG, I remember that we had coding in our port which needed to
know the max number of signals, and this was surprisingly hard since
on some platforms. Sometimes NSIG does not contain the real time
signals. Or it did not even exist. I think we ended up hardcoding an
own max signal limit .
So wherever you access sact with a signal number as index, I'd like to
see a bounds check. Or at least an assert - since this is plain C, a
C-assert would do for me (see
http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html).
This also would guard against user parameter error.
That's probably a good improvement. Do you think it could be handled as
a follow-up bug?
I also wonder whether this coding could not be simplified quite a bit
by not calling the OS versions of signal() at all but instead doing
all signal work via OS sigaction: after all, signal() can be
implemented in terms of sigaction() (with the flag SA_SIGINFO
cleared), and so can sigset(). This would remove the necessity of
reentry-handling for macos, and also remove the need for
save_signal_handler().
That also sounds like an excellent suggestion. Do you think it could be
handled as a follow-up bug?
I will test this version on AIX tomorrow. After that, I'll have some
vacation, but maybe someone else from my team will take over.
I like this work, this is a good simplification.
Thanks. :)
However, it's a bit outside what I normally do. :) Not that I dislike
writing some good old C for a change, but I'm really trying to focus on
cleaning up the flag handling in the build system. This was just a
side-track when I was going to make a change in the jsig.c files (for
adding JNIEXPORT) and realized it was four almost identical copies. I
thought it would be trivial to unify them, and if it's trivial, it's
better to do it yourself right away, than to file a bug on someone else,
or so my motto goes. :)
However, it turned out to be more work than I expected, and I'm losing
interest in pushing this any further. Still, it would be a shame if the
work I've done so far go to waste, but I'd really prefer it if someone
else could pick up this patch and finish it.
/Magnus
Thanks, Thomas
On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com <mailto:magnus.ihse.bur...@oracle.com>>
wrote:
On 2018-03-27 18:24, Thomas Stüfe wrote:
Hi Magnus,
just a cursory look, will look in greater detail tomorrow.
This is good, thanks for doing this.
As I wrote offlist, please remove the painfully wrong AIX
"workarounds". I do not know why we did this - the original code
is quite old - my assumption is that dlsym(RTLD_NEXT) was not
working as expected on older AIX versions. Well, it should work
now according to IBMs manpages. Will test later.
Ok.
__thread is not Posix. I would prefer pthread_get/set_specific
instead, which is more portable.
I have surrounded this code with #ifdef MACOSX now.
Here is an updated webrev. It includes the changes requested by
you and David:
* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.
I also assumed that NSIG is available and working on Solaris. I'll
let David decide if he is happy with that. The alternative is to
go back to the Solaris-specific code that allocates sact on the heap.
Webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05
<http://cr.openjdk.java.net/%7Eihse/JDK-8200298-unify-libjsig/webrev.05>
Once again, here is also a webrev that shows the difference
between the original files and the new, unified file. Even if it's
hard to read, it shows what the effects will be for each platform.
Webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/
<http://cr.openjdk.java.net/%7Eihse/JDK-8200298-unify-libjsig/webrev.04/>
/Magnus
Thanks!
Thomas
On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com
<mailto:magnus.ihse.bur...@oracle.com>> 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.
* 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.
* 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().
Also worth noting:
* Solaris is not using pthreads, but it's own thread library,
which accounts for most of the #ifdef SOLARIS.
* 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.
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
<http://cr.openjdk.java.net/%7Eihse/JDK-8200298-unify-libjsig/webrev.01>
Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
<https://bugs.openjdk.java.net/browse/JDK-8200298>
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03
<http://cr.openjdk.java.net/%7Eihse/JDK-8200298-unify-libjsig/webrev.03>
/Magnus
