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

Thomas Stüfe thomas.stuefe at gmail.com
Wed Mar 28 19:21:06 UTC 2018


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.

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().

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, Thomas




On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie <
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
>
> 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/
>
> /Magnus
>
>
> Thanks!
>
> Thomas
>
>
>
>
>
> On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie <
> 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
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
>> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/w
>> ebrev.03
>>
>> /Magnus
>>
>
>
>


More information about the hotspot-runtime-dev mailing list