Hi Magnus, On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe <thomas.stu...@gmail.com> wrote:
> > > On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie < > magnus.ihse.bur...@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.bur...@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.bur...@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 >>>> >>> >>> >>> >> >> >
