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