Extend OMR Signal API [Part 2] #2524

babsingh · 2018-05-03T19:59:03Z

9 new commits have been added to support OMR Signal API extension/improvement.

Add new OMR Signal API: omrsig_register_os_handler

omrsig_register_os_handler will register a handler with the OS. Stubs
have been added for omrsig_register_os_handler.

On Unix variants, we cache the original OS handler in oldActions while
registering the first handler with the OS. The original OS handler is
restored during shutdown.

If OMRSIG_SIGACTION is called outside OMR, then the original OS handler
will be overwritten without being cached in oldActions. Subsequently,
the oldActions may cache a user handler instead of the original OS
handler. During shutdown, we may not restore the original OS handler
incorrectly.

The new API, omrsig_register_os_handler, will let a user register a
handler with the OS while properly caching the original OS handler. This
will allow us to properly restore the original OS handler during
shutdown.
Re-format comments' layout
Rename signalsWithMasterHandlers and masterHandlerMonitor

signalsWithMasterHandlers is used to determine if we have registered a
handler with a signal. With the introduction of
omrsig_register_os_handler, a user can register handlers other than the
master handlers. signalsWithMasterHandlers has been renamed to
signalsWithHandlers to better represent its purpose.

masterHandlerMonitor is acquired whenever a handler is registered with
the OS. Since a user can register any arbitrary handler with the OS
using omrsig_register_os_handler, masterHandlerMonitor is renamed to
registerHandlerMonitor to better represent its purpose.

After restoring the original OS handler in sig_full_shutdown,
oldActions[index].restore should be reset to 0.
Add implementation of omrsig_register_os_handler [Unix]

Implementation of omrsig_register_os_handler has been added in
unix/omrsignal.c.

Supporting entry and exit trace points have been added.

registerSignalHandlerWithOS returns OMRPORT_SIG_ERROR for OS signals
that are included in signalMap.
Fix omrsig_set_async_signal_handler signature

Currently, omrsig_set_async_signal_handler returns an uint32_t.

But, unix and ztpf implementations of omrsig_set_async_signal_handler
set return value to OMRPORT_SIG_ERROR (-1).

In order to allow negative return values,
omrsig_set_async_signal_handler's signature is changed to return an
int32_t.
Use error macros instead of undocumented constants for errors

In omrsig_set_async_signal_handler and
omrsig_set_single_async_signal_handler, constants such as 1 and -1 are
directly used as error codes. Using constants directly leads to
inconsistency in implementation across platforms.

Now onwards, omrsig_set_async_signal_handler and
omrsig_set_single_async_signal_handler will consistently return
OMRPORT_SIG_ERROR (-1) for error.
Cache info if a master handler is registered with a signal

omrsig_protect and omrsig_set_*_async_signal_handler may be invoked
multiple times. If a master handler is already registered with a signal,
then we don't want to re-register the master handler with that signal.

If a master handler is registered with a signal, then this information
is cached in signalsWithMasterHandlers. The port library signal bit is
set in signalsWithMasterHandlers.

When a user overrides the master handler by invoking
omrsig_register_os_handler, then the port library signal bit is un-set
in signalsWithMasterHandlers.

omrsig_set_single_async_signal_handler returns the oldOSHandler. The
oldOSHandler can be the master handler if the user re-invokes
omrsig_set_single_async_signal_handler for a signal. So, a user can
invoke omrsig_register_os_handler using a master handler. In this case,
we must set signalsWithMasterHandlers bits accordingly in
omrsig_register_os_handler.

This allows us to prevent OS calls to re-register master signal
handlers.
Properly handle NULL values for oldOSHandler

omrsig_set_single_async_signal_handler and omrsig_register_os_handler
have been updated to properly handle NULL values for oldOSHandler.
Function description has been updated for both of them. A trace point
was accessing oldOSHandler without a NULL check. A NULL check has been
added before the trace point accesses oldOSHandler.
Remove unnecessary write barrier

At the end of registerSignalHandlerWithOS, a write barrier is issued
before updating signalsWithHandlers. According to the comment, the write
barrier is needed since registerHandlerMonitor is not acquired before
updating signalsWithHandlers at some places.

It has been confirmed that registerHandlerMonitor is acquired every time
signalsWithHandlers is accessed. So, the write barrier at the end of
registerSignalHandlerWithOS is no longer required.

In sig_full_shutdown, registerHandlerMonitor is not acquired before
updating signalsWithHandlers. This is an exception since no other code
should execute when OMR is shutting down.

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

charliegracie · 2018-05-04T12:56:42Z

@genie-omr build all

babsingh · 2018-05-04T16:56:18Z

@charliegracie @pshipton these changes can be reviewed.

pshipton · 2018-05-04T17:50:44Z

port/unix/omrsignal.c

@@ -1193,11 +1259,8 @@ registerSignalHandlerWithOS(OMRPortLibrary *portLibrary, uint32_t portLibrarySig
 		}
 	}

-	/* CMVC 96193 signalsWithMasterHandlers is checked without acquiring the masterHandlerMonitor */
-	issueWriteBarrier();


According to the comment, the problem is not that registerSignalHandlerWithOS is called without acquiring registerHandlerMonitor, the problem is that signalsWithHandlers or signalsWithMasterHandlers is used elsewhere without first acquiring registerHandlerMonitor, and all the actions previous to the write barrier need to be finished before the bit is set.

CMVC 96193 contains the following reasoning. The idea was to avoid unnecessary acquires and improve performance, change sig_protect to improve perf.

"We have seen j9thread_monitor_entry and exit for the masterHandlerMonitor mutex contributing 914
instructions twice in the newDirectByteBuffer.

High-level logic is
acquire masterHandlerMonitor mutex
check for -Xrs flag
if not set then call registerMasterHandlers
release masterHandlerMonitor mutex

If we can test the -Xrs flag first and only acquire/release the mutex if we are going to call
registerMasterHandlers then we can strip 1828 instructions from the newDirectByteBuffer pathlength.
"

will undo this change.

the following commit: Remove unnecessary write barrier, has been removed from the pull request.

hmm, I don't think it needed to be removed. I was commenting because you seemed to misunderstand the reason for the write barrier, and providing the context for the barrier. Looking at the sig_protect code, it doesn't use signalsWithHandlers to check the -Xrs flag, but signalOptionsGlobal. I assume the code was changed at some point. Perhaps there is opportunity for future optimization of the monitors, but this should be left out of this change.

signalsWithHandlers and signalsWithMasterHandlers are always accessed with registerHandlerMonitor. there is a comment about this next to their description: Access to this must be protected by the registerHandlerMonitor. should i re-add the commit for removing the write barrier?

update: in sig_full_shutdown, registerHandlerMonitor is not acquired while changing signalsWithHandlers and signalsWithMasterHandlers but the globalMonitor is acquired.

Re-adding it is fine with me.

pshipton · 2018-05-04T18:27:28Z

port/common/omrsignal.c

@@ -225,6 +225,51 @@ omrsig_map_portlib_signal_to_os_signal(struct OMRPortLibrary *portLibrary, uint3
 	return OMRPORT_SIG_ERROR;
 }

+/**
+ * @brief Register a handler with the OS. For an invalid portlibSignalFlag, an error is returned.
+ * portlibSignalFlag is invalid if it is zero or if multiple signal bits are specified. If


Its also invalid if it sets an unsupported signal

pshipton · 2018-05-04T18:37:27Z

port/common/omrsignal.c

+ * signals, which are supported on a platform. This function will support all signals listed in
+ * omrsignal.c::signalMap.
+ *
+ * On Unix variants, sigaction will be used to register the handler. SA_SIGINFO flag is set.


Please remove use of "will", "we", "us".
i.e.
will be -> is
will let -> lets or allows
we cache the original OS handler -> cache the original OS handler
This will allow us to properly restore the original OS handler during shutdown
-> The original OS handler is properly restored during shutdown

pshipton · 2018-05-04T18:42:11Z

port/unix/omrsignal.c

@@ -89,6 +89,15 @@ static struct {
 */
 static uint32_t signalsWithHandlers;

+/* Records the (port library defined) signals for which we have registered a master


Avoid using "we"
for which we have registered a master handler -> for which a master handler is registered.
then we should avoid -> then avoid

pshipton · 2018-05-04T18:44:02Z

port/unix/omrsignal.c

+			if ((newOSHandler == (void *)masterSynchSignalHandler)
+				|| (newOSHandler == (void *)masterASynchSignalHandler)
+			) {
+				/* User-specified handler is a master handler. So, we should set the portlibSignalFlag bit in


pshipton · 2018-05-04T18:44:08Z

port/unix/omrsignal.c

+				 */
+				signalsWithMasterHandlers |= portlibSignalFlag;
+			} else {
+				/* If the user-specified handler is not a master handler, then we should un-set the


avoid "we" and "will". There are more places, I'm not going to point them all out.

babsingh · 2018-05-04T21:18:25Z

@pshipton

Re-added commit: Remove unnecessary write barrier.
Removed usage of we, will and us from comments added in this pull request.

pshipton · 2018-05-07T13:13:41Z

Please fix the commit comment for "Remove unnecessary write barrier" to properly explain why the barrier is no longer needed.

babsingh · 2018-05-07T14:53:24Z

@pshipton I have updated the commit comment for Remove unnecessary write barrier.

omrsig_register_os_handler will register a handler with the OS. Stubs have been added for omrsig_register_os_handler. On Unix variants, we cache the original OS handler in oldActions while registering the first handler with the OS. The original OS handler is restored during shutdown. If OMRSIG_SIGACTION is called outside OMR, then the original OS handler will be overwritten without being cached in oldActions. Subsequently, the oldActions may cache a user handler instead of the original OS handler. During shutdown, we may not restore the original OS handler incorrectly. The new API, omrsig_register_os_handler, will let a user register a handler with the OS while properly caching the original OS handler. This will allow us to properly restore the original OS handler during shutdown. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

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

signalsWithMasterHandlers is used to determine if we have registered a handler with a signal. With the introduction of omrsig_register_os_handler, a user can register handlers other than the master handlers. signalsWithMasterHandlers has been renamed to signalsWithHandlers to better represent its purpose. masterHandlerMonitor is acquired whenever a handler is registered with the OS. Since a user can register any arbitrary handler with the OS using omrsig_register_os_handler, masterHandlerMonitor is renamed to registerHandlerMonitor to better represent its purpose. After restoring the original OS handler in sig_full_shutdown, oldActions[index].restore should be reset to 0. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

Implementation of omrsig_register_os_handler has been added in unix/omrsignal.c. Supporting entry and exit trace points have been added. registerSignalHandlerWithOS returns OMRPORT_SIG_ERROR for OS signals that are included in signalMap. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

Currently, omrsig_set_async_signal_handler returns an uint32_t. But, unix and ztpf implementations of omrsig_set_async_signal_handler set return value to OMRPORT_SIG_ERROR (-1). In order to allow negative return values, omrsig_set_async_signal_handler's signature is changed to return an int32_t. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

In omrsig_set_async_signal_handler and omrsig_set_single_async_signal_handler, constants such as 1 and -1 are directly used as error codes. Using constants directly leads to inconsistency in implementation across platforms. Now onwards, omrsig_set_async_signal_handler and omrsig_set_single_async_signal_handler will consistently return OMRPORT_SIG_ERROR (-1) for error. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

omrsig_protect and omrsig_set_*_async_signal_handler may be invoked multiple times. If a master handler is already registered with a signal, then we don't want to re-register the master handler with that signal. If a master handler is registered with a signal, then this information is cached in signalsWithMasterHandlers. The port library signal bit is set in signalsWithMasterHandlers. When a user overrides the master handler by invoking omrsig_register_os_handler, then the port library signal bit is un-set in signalsWithMasterHandlers. omrsig_set_single_async_signal_handler returns the oldOSHandler. The oldOSHandler can be the master handler if the user re-invokes omrsig_set_single_async_signal_handler for a signal. So, a user can invoke omrsig_register_os_handler using a master handler. In this case, we must set signalsWithMasterHandlers bits accordingly in omrsig_register_os_handler. This allows us to prevent OS calls to re-register master signal handlers. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

omrsig_set_single_async_signal_handler and omrsig_register_os_handler have been updated to properly handle NULL values for oldOSHandler. Function description has been updated for both of them. A trace point was accessing oldOSHandler without a NULL check. A NULL check has been added before the trace point accesses oldOSHandler. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

At the end of registerSignalHandlerWithOS, a write barrier is issued before updating signalsWithHandlers. According to the comment, the write barrier is needed since registerHandlerMonitor is not acquired before updating signalsWithHandlers at some places. It has been confirmed that registerHandlerMonitor is acquired every time signalsWithHandlers is accessed. So, the write barrier at the end of registerSignalHandlerWithOS is no longer required. In sig_full_shutdown, registerHandlerMonitor is not acquired before updating signalsWithHandlers. This is an exception since no other code should execute when OMR is shutting down. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

babsingh · 2018-05-08T21:15:05Z

@charliegracie Peter has reviewed these changes. can you please review/merge this PR?

charliegracie · 2018-05-14T13:57:55Z

@genie-omr build all

ashu-mehra · 2018-05-15T12:53:25Z

@babsingh I think the new trace points added in this have CRLF line endings, which would break f87abc3 that changed files to have Unix line-endings.

eclipse-omr#2529 changed all files to have Linux line-ends (no CRLF pairs). I had copied old trace points which had CRLF pairs. While rebasing, CRLF pairs were unnoticeable. Removing unintentionally added CRLF pairs in eclipse-omr#2524. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

eclipse-omr#2529 changed all files to have Linux line-ends (no CRLF pairs). I had copied old trace points which had CRLF pairs. While rebasing, CRLF pairs were unnoticeable. Removing CRLF pairs which were introduced unintentionally in eclipse-omr#2524. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

babsingh · 2018-05-15T14:48:33Z

Removed CRLF pairs in tracepoints: #2552

eclipse-omr#2529 changed all files to have Linux line-ends (no CRLF pairs). I had copied old trace points which had CRLF pairs. While rebasing, CRLF pairs were unnoticeable. Removing CRLF pairs which were introduced unintentionally in eclipse-omr#2524. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

babsingh changed the title ~~Extend OMR Signal API [Part 2]~~ WIP: Extend OMR Signal API [Part 2] May 3, 2018

babsingh mentioned this pull request May 3, 2018

JVM Signal Handling Support [Part 2] eclipse-openj9/openj9#1832

Merged

babsingh force-pushed the signal_register_changes branch 4 times, most recently from bbc8b5e to 621826d Compare May 4, 2018 00:30

babsingh changed the title ~~WIP: Extend OMR Signal API [Part 2]~~ Extend OMR Signal API [Part 2] May 4, 2018

pshipton reviewed May 4, 2018

View reviewed changes

babsingh force-pushed the signal_register_changes branch from c8ed32f to 909316e Compare May 4, 2018 18:09

pshipton reviewed May 4, 2018

View reviewed changes

babsingh force-pushed the signal_register_changes branch 4 times, most recently from 5122262 to 785c625 Compare May 4, 2018 21:11

0xdaryl added the comp:port label May 7, 2018

babsingh force-pushed the signal_register_changes branch from 785c625 to ad80164 Compare May 7, 2018 14:51

pshipton approved these changes May 7, 2018

View reviewed changes

babsingh added 3 commits May 7, 2018 16:29

Re-format comments' layout

0657638

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

babsingh force-pushed the signal_register_changes branch from ad80164 to c1c1bab Compare May 7, 2018 20:31

babsingh added 4 commits May 7, 2018 16:35

babsingh added 2 commits May 7, 2018 16:35

babsingh force-pushed the signal_register_changes branch from c1c1bab to 75f00a1 Compare May 7, 2018 20:35

charliegracie merged commit 53cb267 into eclipse-omr:master May 14, 2018

babsingh mentioned this pull request May 15, 2018

[ci skip] Fix line-ends #2552

Merged

babsingh mentioned this pull request May 28, 2018

Add 0.9.0 release notes eclipse-openj9/openj9#1996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend OMR Signal API [Part 2] #2524

Extend OMR Signal API [Part 2] #2524

babsingh commented May 3, 2018 •

edited

Loading

charliegracie commented May 4, 2018

babsingh commented May 4, 2018

pshipton May 4, 2018 •

edited

Loading

pshipton May 4, 2018

babsingh May 4, 2018

babsingh May 4, 2018

pshipton May 4, 2018

babsingh May 4, 2018 •

edited

Loading

pshipton May 4, 2018

pshipton May 4, 2018

pshipton May 4, 2018

pshipton May 4, 2018

pshipton May 4, 2018

pshipton May 4, 2018

babsingh commented May 4, 2018 •

edited

Loading

pshipton commented May 7, 2018 •

edited

Loading

babsingh commented May 7, 2018

babsingh commented May 8, 2018

charliegracie commented May 14, 2018

ashu-mehra commented May 15, 2018

babsingh commented May 15, 2018

Extend OMR Signal API [Part 2] #2524

Extend OMR Signal API [Part 2] #2524

Conversation

babsingh commented May 3, 2018 • edited Loading

charliegracie commented May 4, 2018

babsingh commented May 4, 2018

pshipton May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh commented May 4, 2018 • edited Loading

pshipton commented May 7, 2018 • edited Loading

babsingh commented May 7, 2018

babsingh commented May 8, 2018

charliegracie commented May 14, 2018

ashu-mehra commented May 15, 2018

babsingh commented May 15, 2018

babsingh commented May 3, 2018 •

edited

Loading

pshipton May 4, 2018 •

edited

Loading

babsingh May 4, 2018 •

edited

Loading

babsingh commented May 4, 2018 •

edited

Loading

pshipton commented May 7, 2018 •

edited

Loading