RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c
David Holmes
david.holmes at oracle.com
Wed Apr 4 07:59:12 UTC 2018
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.
> 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.
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
>>
>>
>
More information about the hotspot-runtime-dev
mailing list