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.stu...@gmail.com
<mailto:thomas.stu...@gmail.com>> wrote:
On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com
<mailto: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/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.bur...@oracle.com
<mailto: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/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.bur...@oracle.com
<mailto: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
<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
