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

David Holmes david.holmes at oracle.com
Thu Apr 5 10:30:10 UTC 2018


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:

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.

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