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

Fix compile and test errors with OSX omrintrospect #6467

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

EricYangIBM
Copy link
Contributor

@EricYangIBM EricYangIBM commented Apr 8, 2022

  • Change clock_gettime() to gettimeofday() as the former is unavailable in earlier versions of OSX
  • Disable backtrace functions for AArch64 OSX as they haven't been fully implemented yet

Signed-off-by: Eric Yang eric.yang@ibm.com

Signed-off-by: Eric Yang <eric.yang@ibm.com>
@babsingh
Copy link
Contributor

babsingh commented Apr 8, 2022

What is the intent of this change? nmap/nmap#180 and its references recommend not to use gettimeofday.

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Apr 8, 2022

clock_gettime() is not available pre OSX 10.12. I will have to add other changes to this PR as #6267 will be reverted due to test failures on aarch64 eclipse-openj9/openj9#14892

@keithc-ca
Copy link
Member

What is the intent of this change? nmap/nmap#180 and its references recommend not to use gettimeofday.

If we want to avoid clock_gettime(), https://github.com/eclipse/omr/blob/master/port/unix/omrtime.c#L94 will need to change as well. For now though, this change seems reasonable to me.

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Apr 8, 2022

Ran aarch64_mac tests and failed tests now pass https://hyc-runtimes-jenkins.swg-devops.com/job/Test_openjdk11_j9_sanity.functional_aarch64_mac_Personal/7/tapResults/
Except for cmdLineTester_CryptoTest_0 which isn't related to my PRs

@EricYangIBM
Copy link
Contributor Author

This is with eclipse-openj9/openj9#14201

@babsingh
Copy link
Contributor

babsingh commented Apr 8, 2022

jenkins build osx

@knn-k
Copy link
Contributor

knn-k commented Apr 11, 2022

The reason for the cmdLineTester_CryptoTest_0 failure on AArch64 macOS above is the OpenSSL build settings. You don't need to care about it.

@EricYangIBM
Copy link
Contributor Author

eclipse-openj9/openj9#14201 fixes the aarch64_mac failures in eclipse-openj9/openj9#14892 (this PR only fixes a build error), however the openj9 PR cannot be merged since that would enable javacores for osx while the implementation isn't promoted (blocked right now). Would it make sense to split eclipse-openj9/openj9#14201 and merge its dmpsup.c and gphandle.c changes first?

@babsingh
Copy link
Contributor

Would it make sense to split eclipse-openj9/openj9#14201 and merge its dmpsup.c and gphandle.c changes first?

yes. fyi @pshipton @0xdaryl.

@EricYangIBM
Copy link
Contributor Author

Just ran https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/12638/ with the second commit on this PR and the tests pass as well. Would it be better to merge this PR instead?

@babsingh
Copy link
Contributor

with the second commit on this PR and the tests pass as well

Does this mean that the dmpsup.c and gphandle.c changes in eclipse-openj9/openj9#14201 are no longer needed?

@EricYangIBM
Copy link
Contributor Author

Yes. It seems like something in backtrace_sigprotect is causing the failures

@babsingh
Copy link
Contributor

babsingh commented Apr 11, 2022

Would it be better to merge this PR instead?

Yes, it will be better to isolate all changes within OMR if changes to downstream projects can be avoided.

Instead of just disabling backtrace_sigprotect, will it be better to disable all omrosbacktrace_impl.c code in order to avoid unexpected problems on OSX aarch64 until support on this platform is fully implemented and tested?

uintptr_t
omrintrospect_backtrace_thread_raw(OMRPortLibrary *portLibrary, J9PlatformThread *threadInfo, J9Heap *heap, void *signalInfo)
{
#if defined(OMR_ARCH_X86)
   ... existing code ...
#elif defined(OMR_ARCH_AARCH64) /* defined(OMR_ARCH_X86) */
   return 0;
#else /* defined(OMR_ARCH_AARCH64) */
#error Unsupported processor
#endif /* defined(OMR_ARCH_X86) */
}

uintptr_t
omrintrospect_backtrace_symbols_raw(OMRPortLibrary *portLibrary, J9PlatformThread *threadInfo, J9Heap *heap)
{
#if defined(OMR_ARCH_X86)
   ... existing code ...
#elif defined(OMR_ARCH_AARCH64) /* defined(OMR_ARCH_X86) */
   return 0;
#else /* defined(OMR_ARCH_AARCH64) */
#error Unsupported processor
#endif /* defined(OMR_ARCH_X86) */
}

@babsingh
Copy link
Contributor

@EricYangIBM EricYangIBM changed the title Change clock_gettime() to gettimeofday() on OSX Fix compile and test errors with OSX omrintrospect Apr 11, 2022
port/osx/omrosbacktrace_impl.c Outdated Show resolved Hide resolved
port/osx/omrosbacktrace_impl.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

jenkins build osx

@EricYangIBM
Copy link
Contributor Author

https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/12658/
Existing aarch64mac failures are known issues

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

lgtm. @keithc-ca @0xdaryl, for final review and merge.

@0xdaryl 0xdaryl self-assigned this Apr 14, 2022
@pshipton
Copy link
Member

FYI OMR promotion to OpenJ9 was blocked by #6267, and should be unblocked by this PR.

@0xdaryl
Copy link
Contributor

0xdaryl commented Apr 14, 2022

Looks fine to me. @keithc-ca : could you formally approve please?

@@ -42,6 +42,7 @@

#include "omrintrospect.h"

#if !defined(OMR_ARCH_AARCH64)
Copy link
Member

Choose a reason for hiding this comment

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

I would have preferred that the condition here matched the condition that enables use of these functions, that is defined(OMR_ARCH_X86) instead of !defined(OMR_ARCH_AARCH64), but given those are the only two architectures on which macOS is supported, the conditions are equivalent for all practical purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am going to implement the backtrace feature for AArch64 macOS sometime later.
I would like this PR to be merged soon.

Copy link
Member

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

My previous minor comment can be addressed later, certainly by adding support for aarch64.

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.

6 participants