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

Wed Mar 28 22:37:17 UTC 2018

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.bursie at oracle.com <mailto:magnus.ihse.bursie at 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.bursie at oracle.com
>>     <mailto:magnus.ihse.bursie at 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
>>
>>
>
>