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:07:54 UTC 2018


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

/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