8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library #834

GoeLin · 2022-10-25T11:51:38Z

Resolved signals_posix.cpp because test for do_check is not in 17.
That was added by 8283337: Posix signal handler modification warning triggering incorrectly
I add 8283337 to this backport as it is subsumed by 8292695.

Resolved os.cpp due to context.
Context was changed by "8269466: Factor out the common code for initializing and starting internal VM JavaThreads"

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library
JDK-8283337: Posix signal handler modification warning triggering incorrectly

Reviewers

Man Cao (@caoman - no project role) ⚠️ Review applies to 67611936
Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev pull/834/head:pull/834
$ git checkout pull/834

Update a local copy of the PR:
$ git checkout pull/834
$ git pull https://git.openjdk.org/jdk17u-dev pull/834/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 834

View PR using the GUI difftool:
$ git pr show -t 834

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/834.diff

bridgekeeper · 2022-10-25T11:53:17Z

👋 Welcome back goetz! 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.

openjdk · 2022-10-25T11:54:19Z

This backport pull request has now been updated with issue from the original commit.

GoeLin · 2022-10-25T11:59:04Z

/issue add 8283337

openjdk · 2022-10-25T11:59:54Z

@GoeLin
Adding additional issue to issue list: 8283337: Posix signal handler modification warning triggering incorrectly.

mlbridge · 2022-10-25T12:00:25Z

Webrevs

GoeLin · 2022-10-25T19:33:26Z

Pre-submit test: timeout of javac

caoman · 2022-10-29T06:10:15Z

Thanks for the backport. Looks like https://bugs.openjdk.org/browse/JDK-8292695 didn't have "jdk17u-fix-request", and I just added the label.

The change to HotSpot looks good, but should we also include the TestBreakSignalThreadDump.java test in the backport?

GoeLin · 2022-10-30T08:44:56Z

Hi,
Thanks for looking at my change!
I removed the label again. It is not yet ready for labeling. The label should only be applied once the change is ready. Here the review is still missing.
You are right, I forgot the test. I will add it.
Best regards,
Goetz

caoman

Thanks for the update. LGTM now. Not an official Reviewer though.

TheRealMDoerr

Looks basically good. Only one comment change is missing.

TheRealMDoerr · 2022-11-01T11:23:44Z

src/hotspot/os/posix/signals_posix.cpp

@@ -1246,7 +1241,8 @@ void set_signal_handler(int sig, bool do_check = true) {

  // Save handler setup for later checking


JDK-8283337 changed this comment to: // Save handler setup for possible later checking

Adapt comment

TheRealMDoerr

Thumbs up! Thanks for backporting!

openjdk · 2022-11-02T09:33:02Z

@GoeLin 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:

8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library
8283337: Posix signal handler modification warning triggering incorrectly

Reviewed-by: manc, mdoerr

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 23 new commits pushed to the master branch:

e609398: 8287011: Improve container information
d5fedc5: 8244670: convert clhsdb "whatis" command from javascript to java
5fa5392: 8273578: javax/swing/JMenu/4515762/bug4515762.java fails on macOS 12
0698930: 8295288: Some vm_flags tests associate with a wrong BugID
9fd15d5: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
6a45f75: 8280948: Write a regression test for JDK-4659800
aa98d09: 8291456: com/sun/jdi/ClassUnloadEventTest.java failed with: Wrong number of class unload events: expected 10 got 4
9732004: 8292879: com/sun/jdi/ClassUnloadEventTest.java failed due to classes not unloading
8194120: 8292880: Improve debuggee logging for com/sun/jdi/ClassUnloadEventTest.java
5977ce5: 8288302: Shenandoah: SIGSEGV in vm maybe related to jit compiling xerces
... and 13 more: https://git.openjdk.org/jdk17u-dev/compare/cb9289ed7589e2cfbe8ac21ab6f3039ee09216ea...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.

GoeLin · 2022-11-03T14:17:44Z

/integrate

openjdk · 2022-11-03T14:20:20Z

Going to push as commit ff14ed4.
Since your change was applied there have been 27 commits pushed to the master branch:

f2a10fd: 8265489: Stress test times out because of long ObjectSynchronizer::monitors_iterate(...) operation
092be50: 8287740: NSAccessibilityShowMenuAction not working for text editors
b37d7df: 8275569: Add linux-aarch64 to test-make profiles
eef3c96: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
e609398: 8287011: Improve container information
d5fedc5: 8244670: convert clhsdb "whatis" command from javascript to java
5fa5392: 8273578: javax/swing/JMenu/4515762/bug4515762.java fails on macOS 12
0698930: 8295288: Some vm_flags tests associate with a wrong BugID
9fd15d5: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
6a45f75: 8280948: Write a regression test for JDK-4659800
... and 17 more: https://git.openjdk.org/jdk17u-dev/compare/cb9289ed7589e2cfbe8ac21ab6f3039ee09216ea...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-03T14:20:50Z

@GoeLin Pushed as commit ff14ed4.

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

Backport 45ff10cc68296c7c73d0eafe6fcc9946ab98267e

cf04cd8

openjdk bot changed the title ~~Backport 45ff10cc68296c7c73d0eafe6fcc9946ab98267e~~ 8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library Oct 25, 2022

openjdk bot added backport rfr Pull request is ready for review labels Oct 25, 2022

Add TestBreakSignalThreadDump I forgot in the original commit

6761193

caoman approved these changes Oct 30, 2022

View reviewed changes

TheRealMDoerr reviewed Nov 1, 2022

View reviewed changes

Update signals_posix.cpp

4159c44

Adapt comment

TheRealMDoerr approved these changes Nov 2, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 2, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 3, 2022

openjdk bot closed this Nov 3, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 3, 2022

GoeLin deleted the goetz_backport_8292695 branch November 3, 2022 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library #834

8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library #834

GoeLin commented Oct 25, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

GoeLin commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

mlbridge bot commented Oct 25, 2022 •

edited

Loading

GoeLin commented Oct 25, 2022

caoman commented Oct 29, 2022

GoeLin commented Oct 30, 2022

caoman left a comment

TheRealMDoerr left a comment

TheRealMDoerr Nov 1, 2022

TheRealMDoerr left a comment

openjdk bot commented Nov 2, 2022

GoeLin commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

		@@ -1246,7 +1241,8 @@ void set_signal_handler(int sig, bool do_check = true) {

		// Save handler setup for later checking

8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library #834

8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library #834

Conversation

GoeLin commented Oct 25, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

GoeLin commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

mlbridge bot commented Oct 25, 2022 • edited Loading

Webrevs

GoeLin commented Oct 25, 2022

caoman commented Oct 29, 2022

GoeLin commented Oct 30, 2022

caoman left a comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

TheRealMDoerr Nov 1, 2022

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 2, 2022

GoeLin commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

GoeLin commented Oct 25, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 25, 2022 •

edited

Loading