Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal #1587

Closed
wants to merge 1 commit into from

Conversation

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Dec 3, 2020

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1587/head:pull/1587
$ git checkout pull/1587

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 3, 2020

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Dec 3, 2020

/csr needed
/label add hotspot-runtime

@openjdk openjdk bot added the csr label Dec 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@dholmes-ora this pull request will not be integrated until the CSR request JDK-8257573 for issue JDK-8257572 has been approved.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@dholmes-ora
The hotspot-runtime label was successfully added.

@dholmes-ora dholmes-ora marked this pull request as ready for review Dec 3, 2020
@openjdk openjdk bot added the rfr label Dec 3, 2020
@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Dec 3, 2020

/label add build

@openjdk openjdk bot added the build label Dec 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@dholmes-ora
The build label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2020

Webrevs

magicus
magicus approved these changes Dec 3, 2020
Copy link
Member

@magicus magicus left a comment

As we say in Swedish: Äntligen! :-)

Build changes look good.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2020

Mailing list message from David Holmes on build-dev:

On 3/12/2020 9:19 pm, Magnus Ihse Bursie wrote:

On Thu, 3 Dec 2020 04:34:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David

As we say in Swedish: ?ntligen! :-)

Build changes look good.

Thanks for the review Magnus!

David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2020

Mailing list message from David Holmes on build-dev:

On 3/12/2020 9:26 pm, Alan Bateman wrote:

On Thu, 3 Dec 2020 04:34:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David

Marked as reviewed by alanb (Reviewer).

Thanks for the Review Alan!

David

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up on the jsig.c change. No comment on the Lib.gmk change.

@openjdk openjdk bot removed the csr label Dec 4, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 4, 2020

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal

Reviewed-by: ihse, alanb, dcubed, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 71 new commits pushed to the master branch:

  • ecd7e47: 8257793: Shenandoah: SATB barrier should only filter out already strongly marked oops
  • e08b9ed: 8257820: Remove gc/ergonomics/TestMinHeapSize.java as it is too brittle
  • 637b0c6: 8246778: Compiler implementation for Sealed Classes (Second Preview)
  • 09707dd: 8252807: The jdk.jfr.Recording.getStream does not work when toDisk is disabled
  • 04ce8e3: 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle
  • 5a03e47: 8255560: Class::isRecord should check that the current class is final and not abstract
  • 8e8e584: 8257588: Make os::_page_sizes a bitmask
  • 566d77a: 8254802: ThrowingPushPromisesAsStringCustom.java fails in "try throwing in GET_BODY"
  • f5a582c: 8257575: C2: "failed: only phis" assert failure in loop strip mining verification
  • d05401d: 8256679: Update serialization javadoc once JOSS changes for records are complete
  • ... and 61 more: https://git.openjdk.java.net/jdk/compare/02a0a027f49765e6d9103ef3169008d33176edd9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 4, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 5, 2020

Mailing list message from David Holmes on build-dev:

Hi Erik,

On 4/12/2020 12:07 am, Erik Joelsson wrote:

On Thu, 3 Dec 2020 04:34:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David

make/modules/java.base/Lib.gmk line 131:

129: ifeq ($(call isTargetOsType, unix), true)
130: ifeq ($(STATIC_BUILD), false)
131: LIBJSIG_CFLAGS += -DHOTSPOT_VM_DISTRO='"$(HOTSPOT_VM_DISTRO)"'

In general, I would prefer if a flag like this was just inlined in the SetupJdkLibrary call, as there are no complex conditionals for setting it. Looking a bit further, the variable LIBJSIG_CFLAGS is dead and not set anywhere in the build, so could also just be removed and replaced with your new -D flag.

I thought about just inlining it but it seemed "obvious" that
LIBJSIG_CFLAGS existed exactly for this purpose, so I simply set it
there - taking care to expand it in case it was set directly on the
command-line. It also followed what is done for LIBJLI_CFLAGS.

I can change it if you insist but this code will be very short-lived as
I can remove it again in 17 once I no longer need the deprecation warning.

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 5, 2020

Mailing list message from David Holmes on build-dev:

On 4/12/2020 12:42 am, Daniel D.Daugherty wrote:

On Thu, 3 Dec 2020 04:34:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David

Thumbs up on the jsig.c change. No comment on the Lib.gmk change.

Thanks for the review Dan!

David

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Dec 5, 2020

Hi David,

I am fine with this, but since this is possibly a breaking change would this not need a release note?

..Thomas

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 6, 2020

Mailing list message from David Holmes on build-dev:

Hi Thomas,

On 6/12/2020 1:26 am, Thomas Stuefe wrote:

On Thu, 3 Dec 2020 14:40:26 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago and supported three different Linux signal API's: sigset, signal and sigaction:

https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html

Only sigaction is a Posix supported API for multi-threaded processes, that we can use cross-platform. Both signal and sigset are obsolete and have undefined behaviour in a multi-threaded process. From the Linux man pages:

sigset: This API is obsolete: new applications should use the POSIX signal API (sigaction(2), sigprocmask(2), etc.)

signal: The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

We should deprecate the use of signal and sigset in JDK 16 with a view to their removal in JDK 17.

A CSR request has been filed.

Testing: hotspot/jtreg/runtime/signal tests

Thanks,
David

Thumbs up on the jsig.c change. No comment on the Lib.gmk change.

Hi David,

I am fine with this, but since this is possibly a breaking change would this not need a release note?

Well it isn't breaking anything at this stage as we are just issuing a
deprecation warning. :) But yes I have filed a release note as well.

Thanks,
David

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Dec 6, 2020

Hi David,
I am fine with this, but since this is possibly a breaking change would this not need a release note?

Well it isn't breaking anything at this stage as we are just issuing a
deprecation warning. :) But yes I have filed a release note as well.

Thanks,
David

Hi David,

:) Thanks. The warning might elude customers until JDK17 comes around and things stop working. With signal chaining being out of scope for the typical java consultant, the expertise may have gone so customers may need to look for someone changing and recompiling their native solution. Or bug vendors to do so.

Cheers, Thomas

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 7, 2020

Mailing list message from Erik Joelsson on build-dev:

On 2020-12-04 21:22, David Holmes wrote:

Hi Erik,

On 4/12/2020 12:07 am, Erik Joelsson wrote:

In general, I would prefer if a flag like this was just inlined in
the SetupJdkLibrary call, as there are no complex conditionals for
setting it. Looking a bit further, the variable LIBJSIG_CFLAGS is
dead and not set anywhere in the build, so could also just be removed
and replaced with your new -D flag.

I thought about just inlining it but it seemed "obvious" that
LIBJSIG_CFLAGS existed exactly for this purpose, so I simply set it
there - taking care to expand it in case it was set directly on the
command-line. It also followed what is done for LIBJLI_CFLAGS.

I can change it if you insist but this code will be very short-lived
as I can remove it again in 17 once I no longer need the deprecation
warning.

If you think there is a case for overriding this on the command line,
then sure, we can keep it. From what I can see, this is just a left over
from when some more complicated logic was needed, or these flags needed
to be reused somewhere else. In the case of libjli, we reuse the flags
in several different versions of the SetupJdkLibrary call for libjli.
That said, I won't insist strongly on this.

/Erik

erikj79
erikj79 approved these changes Dec 7, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 7, 2020

Mailing list message from David Holmes on build-dev:

On 7/12/2020 11:35 pm, Erik Joelsson wrote:

On 2020-12-04 21:22, David Holmes wrote:

Hi Erik,

On 4/12/2020 12:07 am, Erik Joelsson wrote:

In general, I would prefer if a flag like this was just inlined in
the SetupJdkLibrary call, as there are no complex conditionals for
setting it. Looking a bit further, the variable LIBJSIG_CFLAGS is
dead and not set anywhere in the build, so could also just be removed
and replaced with your new -D flag.

I thought about just inlining it but it seemed "obvious" that
LIBJSIG_CFLAGS existed exactly for this purpose, so I simply set it
there - taking care to expand it in case it was set directly on the
command-line. It also followed what is done for LIBJLI_CFLAGS.

I can change it if you insist but this code will be very short-lived
as I can remove it again in 17 once I no longer need the deprecation
warning.

If you think there is a case for overriding this on the command line,
then sure, we can keep it. From what I can see, this is just a left over
from when some more complicated logic was needed, or these flags needed
to be reused somewhere else. In the case of libjli, we reuse the flags
in several different versions of the SetupJdkLibrary call for libjli.
That said, I won't insist strongly on this.

Thanks Erik I appreciate that as I just want to get this pushed ASAP.

David

/Erik

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Dec 7, 2020

/integrate

@openjdk openjdk bot closed this Dec 7, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 7, 2020
@dholmes-ora dholmes-ora deleted the 8257572 branch Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@dholmes-ora Since your change was applied there have been 85 commits pushed to the master branch:

  • f92745d: 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
  • 6937d9f: 8257799: Update JLS cross-references in java.compiler
  • a5297bd: 8254939: macOS: unused function 'replicate4_imm'
  • 36c0600: 8257805: Add compiler/blackhole tests to tier1
  • 395b6bd: 8257817: Shenandoah: Don't race with conc-weak-in-progress flag in weak-LRB
  • a265c20: 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource
  • e3793e5: 8257514: Fix the issues in jdk.jpackage identified by SpotBugs
  • bbc44f5: 8257186: Size of heap segments is not computed correctlyFix overflow in size computation for heap segments
  • b4b9828: 8254784: javac should reject records with @SafeVarargs applied to varargs record component
  • dcf63f8: 8257788: Class fields could be local in the SunJSSE provider
  • ... and 75 more: https://git.openjdk.java.net/jdk/compare/02a0a027f49765e6d9103ef3169008d33176edd9...master

Your commit was automatically rebased without conflicts.

Pushed as commit 149a02f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment