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

Properly link omrsig and enable OMRPORT_OMRSIG_SUPPORT globally #2631

Merged

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Aug 16, 2018

  1. Properly link omrsig when building constgen and j9ddrgen

    When building constgen and j9ddrgen, omrsig should be linked when
    J9VM_PORT_OMRSIG_SUPPORT is enabled.

  2. Remove OMRPORT_OMRSIG_SUPPORT definitions

    Currently, OMRPORT_OMRSIG_SUPPORT is only defined while building the
    port library. But, OMRPORT_OMRSIG_SUPPORT should be defined globally.
    This will allow omrsig macros such as OMRSIG_SIGNAL and OMRSIG_SIGACTION
    to be used consistently across OpenJ9.

    Removing localized definitions of OMRPORT_OMRSIG_SUPPORT before enabling
    it globally. This will prevent redundant definitions of
    OMRPORT_OMRSIG_SUPPORT when it is enabled globally.

  3. Enable OMRPORT_OMRSIG_SUPPORT globally in OpenJ9

    Currently, OMRPORT_OMRSIG_SUPPORT is only defined while building the
    port library. But, OMRPORT_OMRSIG_SUPPORT should be defined globally.
    This will allow omrsig macros such as OMRSIG_SIGNAL and OMRSIG_SIGACTION
    to be used consistently across OpenJ9. Thus, OMRPORT_OMRSIG_SUPPORT is
    enabled globally.

  4. Fix indentation in hyvm/module.xml

  5. Link omrsig to port and vm only if J9VM_PORT_OMRSIG_SUPPORT is enabled

    Currently, omrsig is linked to port and vm unconditionally. omrsig
    should be linked to port and vm only if J9VM_PORT_OMRSIG_SUPPORT is
    enabled.

Closes: #1889

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

@babsingh
Copy link
Contributor Author

babsingh commented Aug 16, 2018

This PR depends on eclipse/omr#2855 and eclipse/omr#2865.

@babsingh babsingh force-pushed the define_sig_support_globally_final branch from 39a6c74 to 175d842 Compare August 16, 2018 15:53
@babsingh
Copy link
Contributor Author

+@dnakamura to point out if any CMake dependencies were missed?

@DanHeidinga
Copy link
Member

@babsingh I'm not sure I understand the issue here well enough yet to review the code. My naive thought is that the macros are working as designed.

They expand to sigaction/signal everywhere except the lowest layer that has intercepted them.

Can you clarify why you believe the usage is incorrect or problematic?

@babsingh
Copy link
Contributor Author

babsingh commented Aug 23, 2018

They expand to sigaction/signal everywhere except the lowest layer that has intercepted them.

This is true. In omrsig.cpp, signal and sigaction are overwritten:

sighandler_t
signal(int signum, sighandler_t handler) __THROW
#endif /* defined(OMR_OS_WINDOWS) */
{
	return omrsig_signal_internal(signum, handler);
}
int
sigaction(int signum, const struct sigaction *act, struct sigaction *oldact) __THROW
{
	return omrsig_sigaction_internal(signum, act, oldact, false);
}

OMRSIG_SIGNAL and OMRSIG_SIGACTION macros are defined in omrport.h:

#if defined(OMRPORT_OMRSIG_SUPPORT)
#define OMRSIG_SIGNAL(signum, handler) omrsig_primary_signal(signum, handler)
#define OMRSIG_SIGACTION(signum, act, oldact) omrsig_primary_sigaction(signum, act, oldact)
#else /* defined(OMRPORT_OMRSIG_SUPPORT) */
#define OMRSIG_SIGNAL(signum, handler) signal(signum, handler)
#define OMRSIG_SIGACTION(signum, act, oldact) sigaction(signum, act, oldact)
#endif /* defined(OMRPORT_OMRSIG_SUPPORT) */

Calls to omrsig_handler are also surrounded by OMRPORT_OMRSIG_SUPPORT:

#if defined(J9VM_PORT_OMRSIG_SUPPORT)
	/* Chain to application handler */
	if ( !vmThread || (vmThread && (cachedVM->sigFlags & J9_SIG_NO_SIG_CHAIN) == 0) ) {
		omrsig_handler(sig, 0, 0);
	}
#endif /* defined(J9VM_PORT_OMRSIG_SUPPORT) */

omrsig_handler is needed to invoke the application signal handler.

Currently, OMRPORT_OMRSIG_SUPPORT is only enabled when building the port library. So, the above macros will expand to omrsig_primary_signal/omrsig_primary_sigaction within the port library. Outside the port library, the above macros will expand to signal/sigaction.

Within port library, omrsig_handler will be called. Outside port library, omrsig_handler won't be called. omrsig_handler is not called from dmpsup.c. Thus, the application signal handler is not invoked for SIGABRT. OMRPORT_OMRSIG_SUPPORT should be enabled throughout OpenJ9 for signal chaining to work consistently.

Does it matter if we call omrsig_primary_signal/omrsig_primary_sigaction directly in the JVM? If omrsig is not linked, can omrsig_primary_signal/omrsig_primary_sigaction [used in the JVM] and signal/sigaction [used in C/C++ app] be used together? After testing, I found that JVM's signal handling functionality isn't impacted if omrsig_primary_signal/omrsig_primary_sigaction are directly invoked in the JVM.

After OMRPORT_OMRSIG_SUPPORT is consistently enabled in OpenJ9, I will change the above macros as follows:

#define OMRSIG_SIGNAL(signum, handler) signal(signum, handler)
#define OMRSIG_SIGACTION(signum, act, oldact) sigaction(signum, act, oldact)

We don't need to directly call omrsig_primary_signal/omrsig_primary_sigaction since the omrsig library overwrites signal/sigaction.

@babsingh babsingh force-pushed the define_sig_support_globally_final branch from 313fe2d to bb00890 Compare August 28, 2018 16:19
When building constgen and j9ddrgen, omrsig should be linked when
J9VM_PORT_OMRSIG_SUPPORT is enabled.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Currently, OMRPORT_OMRSIG_SUPPORT is only defined while building the
port library. But, OMRPORT_OMRSIG_SUPPORT should be defined globally.
This will allow omrsig macros such as OMRSIG_SIGNAL and OMRSIG_SIGACTION
to be used consistently across OpenJ9.

Removing localized definitions of OMRPORT_OMRSIG_SUPPORT before enabling
it globally. This will prevent redundant definitions of
OMRPORT_OMRSIG_SUPPORT when it is enabled globally.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Currently, OMRPORT_OMRSIG_SUPPORT is only defined while building the
port library. But, OMRPORT_OMRSIG_SUPPORT should be defined globally.
This will allow omrsig macros such as OMRSIG_SIGNAL and OMRSIG_SIGACTION
to be used consistently across OpenJ9. Thus, OMRPORT_OMRSIG_SUPPORT is
enabled globally.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Currently, omrsig is linked to port and vm unconditionally. omrsig
should be linked to port and vm only if J9VM_PORT_OMRSIG_SUPPORT is
enabled.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh force-pushed the define_sig_support_globally_final branch from bb00890 to 31f655c Compare September 13, 2018 13:36
@babsingh
Copy link
Contributor Author

Required OMR changes (eclipse/omr#2855 and eclipse/omr#2865) are available in eclipse/openj9-omr openj9 branch. These changes are ready to merge.

@@ -26,6 +26,9 @@ TEMP_TARGET_DATASIZE := $(if $(findstring -64,$(SPEC)),64,32)
CONFIGURE_ARGS += \
<#if uma.spec.flags.opt_cuda.enabled>
--enable-OMR_OPT_CUDA \
</#if>
<#if uma.spec.flags.port_omrsigSupport.enabled>
--enable-OMRPORT_OMRSIG_SUPPORT \
Copy link
Member

Choose a reason for hiding this comment

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

How does this enable OMRPORT_OMRSIG_SUPPORT in OpenJ9 code? i.e. where does the flag get defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flag will be defined in omrcfg.h.

@pshipton
Copy link
Member

jenkins test sanity xlinux

@pshipton pshipton merged commit 099779d into eclipse-openj9:master Sep 14, 2018
@babsingh babsingh deleted the define_sig_support_globally_final branch August 17, 2020 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants