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

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


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 build-dev mailing list