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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Thu Apr 5 12:51:15 UTC 2018


On 2018-04-05 14:09, David Holmes wrote:
> :) V12 looks fine. Sorry I didnt see this before previous email.

Finally! :-)

I'll wait for Thomas to test on AIX as well, then I believe this is 
finally ready to push.

/Magnus

>
> David
>
> On 5/04/2018 9:51 PM, Magnus Ihse Bursie wrote:
>> On 2018-04-05 13:07, Magnus Ihse Bursie wrote:
>>> On 2018-04-05 12:30, David Holmes wrote:
>>>> On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:
>>>>> 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. :-)
>>>>
>>>> Yes you have restored them in the same locations.
>>>>
>>>> As for correctness ... there are some preexisting issues I spotted. 
>>>> Do you really want to know? ;-) Well only one correctness issue, 
>>>> plus one unnecessary code issue:
>>> Will it ever end?! :-)
>>>
>>>>
>>>> Correctness:
>>>>
>>>> 83 #ifdef SOLARIS
>>>>   84   if (sact == NULL) {
>>>>   85     sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
>>>> (size_t)sizeof(struct sigaction));
>>>>   86     memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
>>>> sigaction));
>>>>   87   }
>>>>   88
>>>>   89   if (sact == NULL) {
>>>>   90     printf("%s\n", "libjsig.so unable to allocate memory");
>>>>   91     exit(0);
>>>>   92   }
>>>>
>>>> The NULL check at line 89 needs to move to line 86 before we do 
>>>> memset on a NULL pointer.
>>>>
>>>> Redundant code:
>>>>
>>>>  142 static void save_signal_handler(int sig, sa_handler_t disp, 
>>>> bool is_sigset) {
>>>>  143   sigset_t set;
>>>>  144
>>>>  145   allocate_sact();
>>>>
>>>> There are two calls to save_signal_handler, both preceded by a call 
>>>> to allocate_sact(), so we don't need to do it again at line 145.
>>>
>>> Ok, I'll fix it while I'm at it.
>>>
>>> New webrev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10
>>>
>>> Also, here is a webrev with the same patch applied to jdk/hs. (I 
>>> have also included the needed changes to how libjsig is compiled, 
>>> which are pushed to jdk/jdk but not yet integrated into jdk/hs). The 
>>> patch file from this webrev can be applied to jdk/hs, so Thomas 
>>> hopefully can test the AIX changes.
>>>
>>> Let's hope this is the final iteration...
>> *LOL* Of course not. I forgot to press save in the editor before 
>> creating the webrev and starting the tests. *arrrghh* I can't believe 
>> this.
>>
>> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.12
>>
>> /Magnus
>>
>>>
>>> /Magnus
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> /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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>



More information about the hotspot-runtime-dev mailing list