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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Tue Apr 3 13:40:20 UTC 2018



On 2018-03-29 14:51, Thomas Stüfe wrote:
> Hi Magnus,
>
> On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
>
>
>     On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie
>     <magnus.ihse.bursie at oracle.com
>     <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>
>         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
>>         <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?
>
>
>     A simple bounds check would suffice for me. At the start of the
>     external sigaction() and sigset(), just do a:
>
>     if (sig < 0 || sig >= NSIG) {
>       return -1;
>     }
>
Okay. I've added something like this (a cast was also needed, and we 
should set errno if returning an error, to mimick system behaviour).

>
>     In the external signal(), do a:
>
>     if (sig < 0 || sig >= NSIG) {
>     return SIGERR;
>     }
>
>>
>>         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?
>
>
>     Sure!
>
>>
>>         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.
>
>
>     You are almost there. Lets finish this.
>
Oookay, I'll give it a few more cycles :)

>
>     The source looks good to me, if you add above mentioned bounds
>     checks. I'll build it on AIX later today (I just found out your
>     8200357 - which I need to build on AIX - does not apply to hs, and
>     I need to switch to jdk/jdk. Sigh. The unification of hs/jdk
>     cannot come too early.)
>
>
> I'm about to give up.
>
> I cannot build jdk/jdk on AIX since it misses a number of fixes for 
> all include changes which happened in hotspot lately.
> On jdk/hs your libjsig change won't apply.
>
> So I am stuck here and a bit out of time. If you want to press it, go 
> on push it and we fix any follow up errors on AIX after easter. I am 
> fine with the look of the change (modulo the sig bounds check 
> mentioned earlier).

I'm in no hurry to push this.

Here's an updated webrev with the bounds check in place. Let me know 
if/when you can try it on AIX, and/or if you need help to get the patch 
to apply. You might also have noticed that the signal tests are being 
open sourced, right now, which will definitively help you. (Just a lucky 
coincidence!)

Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.07

/Magnus



>
> Best Regards,
>
> Thomas
>
>
>     ..Thomas
>
>         /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
>>>
>>>
>>
>>
>
>
>



More information about the hotspot-runtime-dev mailing list