RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Tue Apr 3 13:40:20 UTC 2018
On 2018-03-29 14:51, Thomas Stüfe wrote:
> Hi Magnus,
>
> On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
>
>
> On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie
> <magnus.ihse.bursie at oracle.com
> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>
> On 2018-03-28 21:21, Thomas Stüfe wrote:
>> Hi Magnus,
>>
>> I had a closer look at the changes, especially at jsig.c. It
>> all comes slowly back. The AIX version has been almost
>> comically wrong.
>>
>> About NSIG, I remember that we had coding in our port which
>> needed to know the max number of signals, and this was
>> surprisingly hard since on some platforms. Sometimes NSIG
>> does not contain the real time signals. Or it did not even
>> exist. I think we ended up hardcoding an own max signal limit .
>>
>> So wherever you access sact with a signal number as index,
>> I'd like to see a bounds check. Or at least an assert - since
>> this is plain C, a C-assert would do for me (see
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html
>> <http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html>).
>> This also would guard against user parameter error.
> That's probably a good improvement. Do you think it could be
> handled as a follow-up bug?
>
>
> A simple bounds check would suffice for me. At the start of the
> external sigaction() and sigset(), just do a:
>
> if (sig < 0 || sig >= NSIG) {
> return -1;
> }
>
Okay. I've added something like this (a cast was also needed, and we
should set errno if returning an error, to mimick system behaviour).
>
> In the external signal(), do a:
>
> if (sig < 0 || sig >= NSIG) {
> return SIGERR;
> }
>
>>
>> I also wonder whether this coding could not be simplified
>> quite a bit by not calling the OS versions of signal() at all
>> but instead doing all signal work via OS sigaction: after
>> all, signal() can be implemented in terms of sigaction()
>> (with the flag SA_SIGINFO cleared), and so can sigset(). This
>> would remove the necessity of reentry-handling for macos, and
>> also remove the need for save_signal_handler().
> That also sounds like an excellent suggestion. Do you think it
> could be handled as a follow-up bug?
>
>
> Sure!
>
>>
>> I will test this version on AIX tomorrow. After that, I'll
>> have some vacation, but maybe someone else from my team will
>> take over.
>>
>> I like this work, this is a good simplification.
> Thanks. :)
>
> However, it's a bit outside what I normally do. :) Not that I
> dislike writing some good old C for a change, but I'm really
> trying to focus on cleaning up the flag handling in the build
> system. This was just a side-track when I was going to make a
> change in the jsig.c files (for adding JNIEXPORT) and realized
> it was four almost identical copies. I thought it would be
> trivial to unify them, and if it's trivial, it's better to do
> it yourself right away, than to file a bug on someone else, or
> so my motto goes. :)
>
>
> However, it turned out to be more work than I expected, and
> I'm losing interest in pushing this any further. Still, it
> would be a shame if the work I've done so far go to waste, but
> I'd really prefer it if someone else could pick up this patch
> and finish it.
>
>
> You are almost there. Lets finish this.
>
Oookay, I'll give it a few more cycles :)
>
> The source looks good to me, if you add above mentioned bounds
> checks. I'll build it on AIX later today (I just found out your
> 8200357 - which I need to build on AIX - does not apply to hs, and
> I need to switch to jdk/jdk. Sigh. The unification of hs/jdk
> cannot come too early.)
>
>
> I'm about to give up.
>
> I cannot build jdk/jdk on AIX since it misses a number of fixes for
> all include changes which happened in hotspot lately.
> On jdk/hs your libjsig change won't apply.
>
> So I am stuck here and a bit out of time. If you want to press it, go
> on push it and we fix any follow up errors on AIX after easter. I am
> fine with the look of the change (modulo the sig bounds check
> mentioned earlier).
I'm in no hurry to push this.
Here's an updated webrev with the bounds check in place. Let me know
if/when you can try it on AIX, and/or if you need help to get the patch
to apply. You might also have noticed that the signal tests are being
open sourced, right now, which will definitively help you. (Just a lucky
coincidence!)
Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.07
/Magnus
>
> Best Regards,
>
> Thomas
>
>
> ..Thomas
>
> /Magnus
>
>
>>
>> Thanks, Thomas
>>
>>
>>
>>
>> On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie
>> <magnus.ihse.bursie at oracle.com
>> <mailto:magnus.ihse.bursie at oracle.com>> 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.
>>
>> 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.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05
>> <http://cr.openjdk.java.net/%7Eihse/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/
>> <http://cr.openjdk.java.net/%7Eihse/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