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

David Holmes david.holmes at oracle.com
Thu Apr 5 12:09:14 UTC 2018


:) V12 looks fine. Sorry I didnt see this before previous email.

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