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

[David Holmes]

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.

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