RFR: 8252324: Signal related code should be shared among POSIX platforms

[Thomas Stüfe]

Hi Gerard,

first off, have said it already, but great work! This is a really good
cleanup.

About your last commit (
https://github.com/openjdk/jdk/pull/157/commits/cc13700d7d3f15927e22d92d9f5ec9a0739ef9a1
Add AIX specific SA code): I think you can drop that again. The version
beforehand looks good to me. I did some archeology and all this coding was
forked from Linux and introduced by Sun for JEP167:

https://bugs.openjdk.java.net/browse/JDK-8005849

It introduces also those "RANDOMLY_.." variables and uses them in Linux and
BSD coding. We just forked that for AIX. Later that coding was rewritten in
Linux, but the changes were not ported to AIX, hence the diff.

--

Weirdly enough, that exact version for JDK-8005849 I cannot even find in
the current jdk git or mercurial depots. I see jep167 introduced in
mercurial with:

changeset:   18025:b7bcf7497f93
user:        sla
date:        Mon Jun 10 11:30:51 2013 +0200
summary:     8005849: JEP 167: Event-Based JVM Tracing

But that one seems to be already the later version, not the initial one I
see at https://bugs.openjdk.java.net/browse/JDK-8005849. Who knows, the
history may be lost in the course of the hg forest consolidation. Maybe
Steffan Larson may know.

--

Anyway, I think all that coding is not needed for AIX. The fact that it
exists is only an effect of AIX missing code after the initial fork.

--https://bugs.openjdk.java.net/browse/JDK-8005849

Also, if you feel like it, you also can remove the remaining unused
definitions of "RANDOMLY_..." in os_linux.cpp and os_bsd.cpp.

--

I wish you had done the move to a new file (os_posix -> signals_posix) in a
separate change, that would have made the review easier. But alas, it is
what it is. I eyed the changes and they seem to be okay. A couple of places
have #ifdef AIX still but we can go through them later. Also, you still
have JDK-8252533 outstanding which will do away with some of the AIX
specific sections.

For all I see this seems fine.

I think roll back the last commit and let us test the final version for a
night or two in our systems, and then this is good to go.

Thanks, Thomas

On Wed, Sep 23, 2020 at 9:17 PM Gerard Ziemski <gziemski at openjdk.java.net>
wrote:

> On Wed, 23 Sep 2020 13:11:42 GMT, Gerard Ziemski <gziemski at openjdk.org>
> wrote:
>
> > > _Mailing list message from [Doerr, Martin](mailto:martin.doerr at sap.com)
> on
> > > [hotspot-runtime-dev](mailto:hotspot-runtime-dev at openjdk.java.net):_
> Hi Gerard,
> > > sorry for the long delay. It took time to get our nightly tests
> working again on AIX.
> > > I have seen an issue, but it may be unrelated to your change. We'll
> retest it.
> >
> > I will take another look at AIX changes to see if I missed something.
> >
> > > Note that there's still unused code left in os_aix.cpp (see below).
> > > Thanks again for taking care of AIX. I appreciate having more shared
> POSIX code.
> >
> > Thank you for the diff, I'll use it and update the webrev.
>
> I took another look at the changes and I have concerns over the following
> methods:
>
> PosixSignals::SR_handler
> PosixSignals::do_suspend
> PosixSignals::do_resume
>
> They seem to differ substantially in parts from the other POSIX platforms.
> In my early webrevs I have accounted for
> those changes using "#if defined(AIX)", but during pre-reviews you asked
> me to revert to the common POSIX code.
>
> Can you please take a look again and tell me if you are OK with the
> following SA related changes:
>
> - using POSIX semaphores
> - using the common POSIX code
>
> I will upload, what I think (based only on the code diffs between the
> platforms as I lack AIX platform understanding)
> the AIX platform needs here.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/157
>