On 2018-04-05 14:09, David Holmes wrote:
:) V12 looks fine. Sorry I didnt see this before previous email.
Finally! :-)
I'll wait for Thomas to test on AIX as well, then I believe this is
finally ready to push.
/Magnus
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.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
