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

[Thomas Stüfe]

Hi Magnus,

On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

>
>
> On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie <
> 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/onli
>> nepubs/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;
> }
>
> 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.
>
> 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).

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> 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/w
>>> ebrev.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/w
>>> ebrev.04/
>>>
>>> /Magnus
>>>
>>>
>>> Thanks!
>>>
>>> Thomas
>>>
>>>
>>>
>>>
>>>
>>> On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie <
>>> 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
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
>>>> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/w
>>>> ebrev.03
>>>>
>>>> /Magnus
>>>>
>>>
>>>
>>>
>>
>>
>