8253742: POSIX signal code cleanup #636

gerard-ziemski · 2020-10-13T14:19:02Z

hi all,

Please review this followup to JDK-8252324 Signal related code should be shared among POSIX platforms, where several issues were identified for a cleanup. This change addresses them all:

#1 David's feedback - removed non POSIX SIGNIFICANT_SIGNAL_MASK code

#2 David's feedback - used unblock_program_error_signals() on all platforms (reverted for JDK-8252533)

#3 David's feedback - used single JVM_handle_posix_signal API for all POSIX platforms (reverted for JDK-8255711)

#4 Coleen's feedback - cleanup header files in src/hotspot/os/posix/signals_posix.hpp

#5 Coleen's feedback - hid SR_signum assignment in src/hotspot/os/posix/signals_posix.hpp to avoid having to include <signal.h>

#6 Coleen's feedback - factored out print_signal_handlers()

#7 Thomas' feedback - factored out common POSIX os::SuspendedThreadTask::internal_do_task()

#8 Thomas's feedback - factored out common POSIX signal initialization code

#9 YaSuenag's feedback - used JVM_handle_posix_signal for the common API

#10 YaSuenag's feedback - unified logging out of the scope for this fix

#11 YaSuenag's feedback - memset usage in PosixSignals::jdk_misc_signal_init() correct?

Progress

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

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8253742: POSIX signal code cleanup

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Download

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

bridgekeeper · 2020-10-13T14:21:22Z

👋 Welcome back gziemski! 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 · 2020-10-13T14:26:34Z

@gerard-ziemski The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

magicus · 2020-10-26T09:56:30Z

/label remove build

openjdk · 2020-10-26T09:57:00Z

@magicus
The build label was successfully removed.

…platform code

openjdk · 2020-10-27T19:37:25Z

⚠️ @gerard-ziemski This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

gerard-ziemski · 2020-11-02T19:28:39Z

Does anyone have a suggestion on how to merge with current JDK in the safest way?

mlbridge · 2020-11-02T19:31:05Z

Webrevs

tstuefe

Hi Gerard,

good job!

But we really should synchronize :)

I am currently working on: https://bugs.openjdk.java.net/browse/JDK-8255711
(see Draft PR: #982)

and https://bugs.openjdk.java.net/browse/JDK-8252533 is also still open (see #839) - still waiting Davids final OK.

So unfortunately there are a number of clashes with your change:

"JVM_handle_xxx_signal()": See my mail to jdk-dev from this morning: https://mail.openjdk.java.net/pipermail/jdk-dev/2020-November/004887.html - we either can get completely rid of this function, which is what my current Draft for JDK-8255711 does. Or we need to retain it for backward compatibility, but if so, we need to retain it with the current interface.

Either way, could you please withhold changes to the hotspot signal handlers for the moment (so, both javaSignalHandler() and the various JVM_handle_xxx_signal() functions)?

I removed some functions you changed - all that signal blocking mask stuff. See #839. Could you hold any changes to those functions until JDK-8252533 is out of the door? Hope to do this tomorrow, with your and Davids approval.

The unification of the signal handler printing stuff and the SR initialization make sense and are a good simplification.

Please fine further remarks remarks inline.

Cheers, Thomas

make/hotspot/symbols/symbols-linux

src/hotspot/os/posix/signals_posix.cpp

src/hotspot/os/aix/os_aix.cpp

src/hotspot/os/posix/signals_posix.cpp

src/hotspot/share/runtime/os.hpp

dholmes-ora · 2020-11-03T09:42:19Z

I hadn't realized that JVM_handle_XXX_signal defined a per-platform "public" entry point to allow external callers of the signal handling function in conjunction with -XX:+AllowUserSignalHandlers. We need to keep these but they can each call JVM_handle_posix_signal as their implementation.

tstuefe · 2020-11-03T10:05:03Z

I hadn't realized that JVM_handle_XXX_signal defined a per-platform "public" entry point to allow external callers of the signal handling function in conjunction with -XX:+AllowUserSignalHandlers. We need to keep these but they can each call JVM_handle_posix_signal as their implementation.

We should disentangle https://bugs.openjdk.java.net/browse/JDK-8255711 and this patch, https://bugs.openjdk.java.net/browse/JDK-8253742.

I started by giving my patch a less generic name ("Fix and unify hotspot signal handlers"). I propose to do the same with this patch, or even split this patch into two smaller parts, since it does two things:

unify diagnostic printing code
unify SR handler setup

As I wrote, I'd prefer to keep changes to JVM_xxx and javaSignalHandler out of this patch completely. I have to change those functions since the point of my patch is signal handler unification.

In turn, I will keep my hands off any other code in signals_posix.xxx to decrease chances of conflict with this patch.

tstuefe · 2020-11-03T10:08:28Z

I hadn't realized that JVM_handle_XXX_signal defined a per-platform "public" entry point to allow external callers of the signal handling function in conjunction with -XX:+AllowUserSignalHandlers. We need to keep these but they can each call JVM_handle_posix_signal as their implementation.

We should disentangle https://bugs.openjdk.java.net/browse/JDK-8255711 and this patch, https://bugs.openjdk.java.net/browse/JDK-8253742.

I started by giving my patch a less generic name ("Fix and unify hotspot signal handlers"). I propose to do the same with this patch, or even split this patch into two smaller parts, since it does two things:

unify diagnostic printing code

unify SR handler setup

As I wrote, I'd prefer to keep changes to JVM_xxx and javaSignalHandler out of this patch completely. I have to change those functions since the point of my patch is signal handler unification.

In turn, I will keep my hands off any other code in signals_posix.xxx to decrease chances of conflict with this patch.

Oh, and yes, I preserve the JVM_handle_xxx_signal entries in my patch, but they are thin wrappers around an internal, posix-specific handler function.

openjdk · 2020-11-03T21:21:09Z

@gerard-ziemski this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8253742
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

gerard-ziemski · 2020-11-12T20:14:19Z

Mailing list message from David Holmes on hotspot-dev:

On 11/11/2020 3:04 am, Gerard Ziemski wrote:

Many thanks Thomas & David for the lesson on the header files!
If I understand it correctly, then we need to add #include "utilities/globalDefinitions.hpp" to signals_posix.hpp, correct?

It is already there.

#ifndef OS_POSIX_SIGNALS_POSIX_HPP
#define OS_POSIX_SIGNALS_POSIX_HPP

#include "memory/allocation.hpp"
#include "utilities/globalDefinitions.hpp"

#include <signal.h>

So you can just delete the include of signal.h

#include "memory/allocation.hpp" has:

#include "memory/allStatic.hpp" #include "utilities/globalDefinitions.hpp"

so in the end "memory/allocation.hpp" is all we need.

mlbridge · 2020-11-13T04:03:57Z

Mailing list message from David Holmes on hotspot-dev:

On 13/11/2020 6:16 am, Gerard Ziemski wrote:

If I understand it correctly, then we need to add `#include "utilities/globalDefinitions.hpp"` to signals_posix.hpp, correct?

It is already there.

#ifndef OS_POSIX_SIGNALS_POSIX_HPP
#define OS_POSIX_SIGNALS_POSIX_HPP

#include "memory/allocation.hpp"
#include "utilities/globalDefinitions.hpp"

#include <signal.h>

So you can just delete the include of signal.h

`#include "memory/allocation.hpp"` has:

`#include "memory/allStatic.hpp"
#include "utilities/globalDefinitions.hpp"`

so in the end `"memory/allocation.hpp"` is all we need.

Yes but we don't like to rely on extreme levels of indirection as they
are completely obscure. globalDefinitions.hpp is a known source of a
whole bunch of includes (which a number of folk find objectionable in
itself) so it is okay to get an indirect include through it.

In all seriousness I would just leave the existing includes alone. If it
ain't broke ... and this is wasting far too many cycles for everyone.

Thanks,
David
-----

openjdk · 2020-11-16T17:15:23Z

@gerard-ziemski 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:

8253742: POSIX signal code cleanup

Reviewed-by: stuefe, dholmes

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

c089214: 8255368: Math.exp() gives wrong result for large values on x86 32-bit platforms
7ecf070: 8256476: Assert in vmIntrinsics::flags_for with -XX:+Verbose
26a1ec1: 8256435: [TESTBUG] java/foreign/TestHandshake.java fails with direct buffer memory OOM
ef3ddb1: 8255448: Fastdebug JVM crashes with Vector API when PrintAssembly is turned on
eb02184: 8134630: make code and comments consistent for stack lock optimization
f751738: 8256484: ZGC: Rename ZRelocationSetSelector::register_garbage_page()
d9dbd5d: 8256430: add linux-x64-optimized to regular testing
9efbb46: 8256426: Shenandoah: Remove superfluous assert is ShBS::load_reference_barrier()
9130ca4: 8256401: ZGC: Improve ZList verification
f2a9d02: 8256370: Add asserts to Reference.getInactive()
... and 113 more: https://git.openjdk.java.net/jdk/compare/17f04fc9e755a83727ac9d13a39b487f99311efd...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.

tstuefe

Hi Gerard,

we are getting closer :)

Cheers, Thomas

src/hotspot/os/posix/signals_posix.hpp

src/hotspot/os/posix/signals_posix.cpp

dholmes-ora

I'm seeing unexpected changes relating to os::fetch_compiled_frame_from_context seemingly due to the "Merge from master".

Otherwise the actual described changes since last commit seem fine.

Once Thomas's last couple of comments are addressed this is done.

Thanks,
David

gerard-ziemski · 2020-11-17T15:38:55Z

I'm seeing unexpected changes relating to os::fetch_compiled_frame_from_context seemingly due to the "Merge from master".

Otherwise the actual described changes since last commit seem fine.

Once Thomas's last couple of comments are addressed this is done.

Thanks,
David

The ucontext_get_pc() API needs to be in os::Posix, not PosixSignals, so that change is OK.

gerard-ziemski · 2020-11-17T15:41:00Z

Thank you Thomas and David for your reviews.

tstuefe

Hi Gerard,

some final nits.

I'll run the change through our system tonight to see if this broke one of our platforms.

Cheers, Thomas

src/hotspot/os/posix/signals_posix.cpp

dholmes-ora · 2020-11-17T22:55:54Z

src/hotspot/os/posix/signals_posix.cpp

@@ -1721,7 +1718,7 @@ void os::SuspendedThreadTask::internal_do_task() {
 int PosixSignals::init() {
  // initialize suspend/resume support - must do this before signal_sets_init()
  if (SR_initialize() != 0) {
-    perror("SR_initialize failed");
+    vm_exit_during_initialization("SR_initialize failed");


You've lost the failure reason (errno) that perror would have provided. SR_initalize only returns non-zero if sigaction fails, which leaves errno set - hence perror() would report it. That said there are only two errors for sigaction (EFAULT or EINVAL) so we really haven't lost anything that useful.
Arguably the vm_exit... should be inside SR_initialize.
But I'm fine with this as-is so we can conclude this PR.

Yes, but this relies on knowing the internal implementation of SR_initialize() (mainly that the last CRT call there was sigaction) and is different from all the other places where we do CRT calls. I'd rather add logging right there if the call fails like we do in other places.

tstuefe

AIX build went through, tests are fine so far. Looks good now. Ship it!

gerard-ziemski · 2020-11-18T15:28:30Z

/integrate

openjdk · 2020-11-18T15:29:45Z

@gerard-ziemski Since your change was applied there have been 136 commits pushed to the master branch:

fa8dce4: 8256365: Clean up vtable initialization code
4e5116c: 8256501: libTestMainKeyWindow fails to build with Xcode 12.2
486d6f6: 8256363: Define toString() for MGF1ParameterSpec
eab170c: 8256415: Shenandoah: Should evacuate/update codecache concurrently when class unloading is off
f504f41: 8256385: C2: fatal error: modified node is not on IGVN._worklist
5bcf898: 8256478: C2 compilation fails with assert(t1->isa_long()) failed: Type must be a long
f7f3447: 8256453: C2: Reduce State footprint
d2ddf07: 8256477: Specialize heap memory segment implementations
9707496: 8256479: [PPC64] C2 crashes when UseVectorByteReverseInstructionsPPC64 used without SuperwordUseVSX
b0b9dd2: 8256318: AArch64: Add support for floating-point absolute difference
... and 126 more: https://git.openjdk.java.net/jdk/compare/17f04fc9e755a83727ac9d13a39b487f99311efd...master

Your commit was automatically rebased without conflicts.

Pushed as commit 50a2c22.

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

Reviewed-by: stuefe, dholmes

gerard-ziemski · 2020-11-18T15:33:35Z

Thank you David and Thomas for your reviews!

gerard-ziemski added 2 commits October 8, 2020 10:58

Remove non POSIX SIGNIFICANT_SIGNAL_MASK code

2cf3ec2

Use unblock_program_error_signals() on all platforms

41d5c43

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 13, 2020

Use JVM_handle_posix_signal for all POSIX platforms

c77f99f

openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org and removed hotspot-runtime hotspot-runtime-dev@openjdk.org labels Oct 23, 2020

Use JVM_handle_posix_signal for all POSIX platforms

0205b0e

openjdk bot removed the build build-dev@openjdk.org label Oct 26, 2020

gerard-ziemski added 2 commits October 27, 2020 14:17

factor out ucontext_get_pc and ucontext_set_pc into their respective …

d3d44d8

…platform code

merge

55352a9

gerard-ziemski added 4 commits October 30, 2020 13:31

Coleen's feedback integrated

79286ca

factor out print_signal_handlers

1be33c1

Factor out do_task into PosixSignals

4bdd18c

Factor out common POSIX signal initialization code

29f5635

gerard-ziemski marked this pull request as ready for review November 2, 2020 19:24

openjdk bot added the rfr Pull request is ready for review label Nov 2, 2020

tstuefe reviewed Nov 2, 2020

View reviewed changes

revert JVM_handle_XXX_signal change

f0bbbcc

Merge master

dbec19c

openjdk bot removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Nov 16, 2020

Fix missing os::print_signal_handlers on Windows, white space on Linux

44d34ba

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 16, 2020

tstuefe suggested changes Nov 16, 2020

View reviewed changes

dholmes-ora approved these changes Nov 17, 2020

View reviewed changes

gerard-ziemski added 2 commits November 17, 2020 09:42

implement Thomas' final feedback

ecdfb7a

remove leftover comment

e7ca88f

tstuefe reviewed Nov 17, 2020

View reviewed changes

src/hotspot/os/posix/signals_posix.cpp Outdated Show resolved Hide resolved

src/hotspot/os/posix/signals_posix.cpp Outdated Show resolved Hide resolved

last tweaks

45dfe8e

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Nov 17, 2020

dholmes-ora approved these changes Nov 17, 2020

View reviewed changes

tstuefe approved these changes Nov 18, 2020

View reviewed changes

openjdk bot closed this Nov 18, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 18, 2020

openjdk-notifier bot referenced this pull request Nov 18, 2020

8253742: POSIX signal code cleanup

50a2c22

Reviewed-by: stuefe, dholmes

gerard-ziemski deleted the JDK-8253742 branch December 7, 2020 17:01

8253742: POSIX signal code cleanup #636

8253742: POSIX signal code cleanup #636

Uh oh!

Conversation

gerard-ziemski commented Oct 13, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Testing

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Oct 13, 2020

Uh oh!

openjdk bot commented Oct 13, 2020

Uh oh!

magicus commented Oct 26, 2020

Uh oh!

openjdk bot commented Oct 26, 2020

Uh oh!

openjdk bot commented Oct 27, 2020

Uh oh!

gerard-ziemski commented Nov 2, 2020

Uh oh!

mlbridge bot commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dholmes-ora commented Nov 3, 2020

Uh oh!

tstuefe commented Nov 3, 2020

Uh oh!

tstuefe commented Nov 3, 2020

Uh oh!

openjdk bot commented Nov 3, 2020

Uh oh!

gerard-ziemski commented Nov 12, 2020

Uh oh!

mlbridge bot commented Nov 13, 2020

Uh oh!

openjdk bot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

gerard-ziemski commented Nov 17, 2020

Uh oh!

gerard-ziemski commented Nov 17, 2020

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dholmes-ora Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

tstuefe Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

gerard-ziemski commented Oct 13, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2020 •

edited

Loading

openjdk bot commented Nov 16, 2020 •

edited

Loading