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

Thu Apr 5 09:52:31 UTC 2018

On 2018-04-05 10:30, David Holmes wrote:
> On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:
>>
>>
>> On 2018-04-04 09:59, David Holmes wrote:
>>> Hi Magnus,
>>>
>>> Sorry for the delay ...
>>>
>>> On 28/03/2018 8:15 PM, Magnus Ihse Bursie 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.
>>>
>>> That all seems good.
>> Good. :)
>>
>>>
>>>> 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.
>>>
>>> Unfortunately NSIG is a problem on Solaris:
>>>
>>> http://austingroupbugs.net/view.php?id=741
>>>
>>> It's use also precludes the use of the real-time signals - which 
>>> limits Linux as well. But I'm not completely clear on exactly how 
>>> signals may be numbered in their entirety so I wouldn't necessarily 
>>> suggest trying to use SIGRTMAX+1 as the number of available signals, 
>>> other than on Solaris.
>> Ok. I hope I understand you correctly. I have replaced NSIG with 
>> MAX_SIGNALS, which is defined as NSIG on all platforms except 
>> Solaris, where it is defined as SIGRTMAX+1.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08
>>
>> (8th time's a charm..?)
>
> Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
> not a constant expression. That's why Solaris uses malloc.

Duh. Right.

Oooookay. Like this, then?

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

I have restored the calls to allocate_sact() in the same locations as in 
the original solaris version. Hopefully, those are correct. :-)

/Magnus
>
> David
>
>
>> /Magnus
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> 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 
>>>>> <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
>>>>>
>>>>>
>>>>
>>