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.

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




Reply via email to