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.

Sign up for GitHub

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

Jump to bottom

JVM Signal Handling Support [Part 2] #1832

Merged
merged 5 commits into from
May 17, 2018
Merged

JVM Signal Handling Support [Part 2] #1832

merged 5 commits into from
May 17, 2018
+64 −58

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented May 3, 2018 •
edited
Loading

5 commits added to extend/improve JVM signal handling support.

  1. Add new function registerOSHandler

    registerOSHandler allows the JVM to register a signal handler function
    with the OS via omrsig_register_os_handler. OMR signal source code to
    restore the original OS handler during shutdown won't work properly if
    we directly register a handler with the OS using OMRSIG_SIGACTION or
    OMRSIG_SIGNAL. By using omrsig_register_os_handler, we make sure that
    the original OS handler is properly cached and restored by OMR.

    registerOSHandler is added to J9InternalFunctions since it will be
    invoked from "jvm.c::JVM_RegisterSignal".

  2. Fix comments and formatting

  3. Use registerOSHandler in JVM_RegisterSignal

    Use registerOSHandler in JVM_RegisterSignal instead of directly
    registering a handler using OMRSIG_SIGACTION or OMRSIG_SIGNAL. This will
    fix a potential OMR bug related to caching and restoring original OS
    handlers during shutdown.

  4. Fix usage of omrsig_set_async_signal_handler

    omrsig_set_async_signal_handler's signature has been changed to return
    an int32_t since it sets return value to OMRPORT_SIG_ERROR (-1) in some
    cases.

    OpenJ9 source code has been amended to accept an int32_t return value
    from omrsig_set_async_signal_handler.

  5. Fix code to remove predefinedHandlerWrapper

    Since j9sig_set_single_async_signal_handler was used to register
    predefinedHandlerWrapper, j9sig_set_single_async_signal_handler must be
    used to remove/free predefinedHandlerWrapper.

Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

@babsingh
Copy link
Contributor Author

babsingh commented May 3, 2018

Jenkins test sanity depends eclipse-omr/omr#2524

@babsingh babsingh changed the title JVM Signal Handling Support [Part 2] WIP: JVM Signal Handling Support [Part 2] May 3, 2018
@babsingh babsingh force-pushed the signal_register_changes branch from 0660934 to 1d726ad Compare May 3, 2018 21:03
@pshipton pshipton added comp:vm depends:omr Pull request is dependent on a corresponding change in OMR labels May 3, 2018
@babsingh babsingh force-pushed the signal_register_changes branch 2 times, most recently from a5a799e to 574ec5d Compare May 4, 2018 00:54
@babsingh babsingh changed the title WIP: JVM Signal Handling Support [Part 2] JVM Signal Handling Support [Part 2] May 4, 2018
@babsingh
Copy link
Contributor Author

babsingh commented May 4, 2018

@pshipton these changes can be reviewed.

@pshipton
Copy link
Member

pshipton commented May 4, 2018

I didn't see any concerns.

DanHeidinga
DanHeidinga reviewed May 4, 2018
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good. I'm hoping the code duplication can be removed but even that's pretty minor

runtime/vm/jvminit.c
@@ -6323,7 +6325,8 @@ predefinedHandlerWrapper(struct J9PortLibrary *portLibrary, U_32 gpType, void *g
}

IDATA
registerPredefinedHandler(J9JavaVM *vm, U_32 signal, void **oldOSHandler) {
registerPredefinedHandler(J9JavaVM *vm, U_32 signal, void **oldOSHandler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need two copies of this code? Could this one call registerOSHandler with predefinedHandlerWrapper as the newOSHandler? Or can we push knowledge of which handler to pass to the caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registerPredefinedHandler sets OS handler as omrsignal.c::masterASynchSignalHandler [not exposed externally], and adds predefinedHandlerWrapper to the asyncHandlerList list. predefinedHandlerWrapper is not an OS handler.

registerOSHandler registers the user-provided OS handler with specified signal.

we can merge registerOSHandler and registerPredefinedHandler. use newOSHandler as J9_PRE_DEFINED_HANDLER_CHECK(2) to specify the predefinedHandlerWrapper. should i proceed with this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. The two functions are subtly different. Better to leave them as two different calls.

@DanHeidinga
Copy link
Member

Once the OMR side is merged and a build passes, this should be good to go.

@pshipton pshipton added this to the Release 0.9.0 milestone May 9, 2018
@babsingh babsingh force-pushed the signal_register_changes branch from 574ec5d to 6632592 Compare May 14, 2018 14:49
@babsingh
Copy link
Contributor Author

OMR changes have been merged: eclipse-omr/omr#2524. They should be in the OpenJ9 pipeline by the end of the day.

babsingh added 5 commits May 14, 2018 16:49
@babsingh
Add new function registerOSHandler
b0d7840 
registerOSHandler allows the JVM to register a signal handler function
with the OS via omrsig_register_os_handler. OMR signal source code to
restore the original OS handler during shutdown won't work properly if
we directly register a handler with the OS using OMRSIG_SIGACTION or
OMRSIG_SIGNAL. By using omrsig_register_os_handler, we make sure that
the original OS handler is properly cached and restored by OMR.

registerOSHandler is added to J9InternalFunctions since it will be
invoked from "jvm.c::JVM_RegisterSignal".

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Fix comments and formatting
d6833aa 
Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Use registerOSHandler in JVM_RegisterSignal
fdd18bf 
Use registerOSHandler in JVM_RegisterSignal instead of directly
registering a handler using OMRSIG_SIGACTION or OMRSIG_SIGNAL. This will
fix a potential OMR bug related to caching and restoring original OS
handlers during shutdown.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Fix usage of omrsig_set_async_signal_handler
3008dd5 
omrsig_set_async_signal_handler's signature has been changed to return
an int32_t since it sets return value to OMRPORT_SIG_ERROR (-1) in some
cases.

OpenJ9 source code has been amended to accept an int32_t return value
from omrsig_set_async_signal_handler.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Fix code to remove predefinedHandlerWrapper
ab9b544 
Since j9sig_set_single_async_signal_handler was used to register
predefinedHandlerWrapper, j9sig_set_single_async_signal_handler must be
used to remove/free predefinedHandlerWrapper.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh force-pushed the signal_register_changes branch from 6632592 to ab9b544 Compare May 14, 2018 20:49
@pshipton
Copy link
Member

jenkins test all xlinux,win jdk10

@pshipton
Copy link
Member

jenkins test all aix jdk8

@pshipton
Copy link
Member

The Windows failures are timeouts due to not having any Windows machines to test on right now.

@pshipton
Copy link
Member

I'm thinking Windows machines will be available shortly, #1662. There was one but it got rebooted and didn't restart properly.

@pshipton pshipton mentioned this pull request May 16, 2018
Closed
@pshipton
Copy link
Member

Do you want to do any squashing before this is merged?

@babsingh
Copy link
Contributor Author

Each commit is independent and covers a unique issue so no squashing is needed.

@babsingh babsingh closed this May 17, 2018
@babsingh babsingh reopened this May 17, 2018
@babsingh
Copy link
Contributor Author

while posting the last comment, i closed the pr by mistake. when i reopened the pr, all the jobs restarted.

@pshipton
Copy link
Member

Windows fails because there are no machines.

@pshipton pshipton merged commit 1ac0311 into eclipse-openj9:master May 17, 2018
@babsingh babsingh deleted the signal_register_changes branch August 17, 2020 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga left review comments

@pshipton pshipton pshipton approved these changes

Assignees
No one assigned
Labels
comp:vm depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Milestone
Release 0.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@babsingh @pshipton @DanHeidinga