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

Update jvmtiGetOrSetLocal and jvmtiNotifyFramePop to support virtual threads #15857

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

thallium
Copy link
Contributor

@thallium thallium commented Sep 9, 2022

Adds virtual thread support for GetOrSetLocal and NotifyFramePop

As walkState->walkThread will be used by some jit functions after
stackwalk, we can't use walkContinuationStackFrames which will cause
dangling pointer. Instead we need to stack-allocate manually if a
J9VMContinuation needs to be walked, so we created a helper
getJ9VMContinuationToWalk to get the J9VMContinuation.

Also fix coding style of surrounding code

Issues: #15759 #15183

Co-authored-by: Babneet Singh sbabneet@ca.ibm.com
Signed-off-by: Gengchen Tuo gengchen.tuo@ibm.com

@thallium thallium marked this pull request as draft September 9, 2022 14:23
@thallium
Copy link
Contributor Author

thallium commented Sep 9, 2022

currently the serviceability/jvmti/GetLocalVariable/GetLocalVars fails with the following output:

 test_local_byte: BEGIN

 GetLocalInt: JVMTI error (0)
 GetLocalInt got value from a local byte as expected
 GetLocalLong: JVMTI error (0)
 FAIL: GetLocalLong failed to return JVMTI_ERROR_INVALID_SLOT for local byte
 GetLocalFloat: JVMTI error (0)
 GetLocalFloat got value from a local byte as expected
 GetLocalDouble: JVMTI error (0)
 FAIL: GetLocalDouble failed to return JVMTI_ERROR_INVALID_SLOT for local byte
 GetLocalObject: JVMTI error (0)
 FAIL: GetLocalObject failed to return JVMTI_ERROR_TYPE_MISMATCH for local byte

 test_local_byte: END


 test_local_object: BEGIN

 GetLocalInt: JVMTI error (0)
 FAIL: GetLocalInt failed to return JVMTI_ERROR_TYPE_MISMATCH for local object
 GetLocalLong: JVMTI error (0)
 FAIL: GetLocalLong failed to return JVMTI_ERROR_TYPE_MISMATCH for local object
 GetLocalFloat: JVMTI error (0)
 FAIL: GetLocalFloat failed to return JVMTI_ERROR_TYPE_MISMATCH for local object
 GetLocalDouble: JVMTI error (0)
 FAIL: GetLocalDouble failed to return JVMTI_ERROR_TYPE_MISMATCH for local object
 GetLocalObject: JVMTI error (0)
 GetLocalObject got value from a local object as expected

 test_local_object: END


 test_local_double: BEGIN

 GetLocalInt: JVMTI error (0)
 GetLocalInt got value from a local double as expected
 GetLocalLong: JVMTI error (0)
 GetLocalLong got value from a local double as expected
 GetLocalFloat: JVMTI error (0)
 GetLocalFloat got value from a local double as expected
 GetLocalDouble: JVMTI error (0)
 GetLocalDouble got value from a local double as expected
 GetLocalObject: JVMTI error (0)
 FAIL: GetLocalObject failed to return JVMTI_ERROR_TYPE_MISMATCH for local double

 test_local_double: END


 test_local_integer: BEGIN

 GetLocalInt: JVMTI error (0)
 GetLocalInt got value from a local int as expected
 GetLocalFloat: JVMTI error (0)
 GetLocalFloat got value from a local int as expected
 GetLocalObject: JVMTI error (0)
 FAIL: GetLocalObject failed to return JVMTI_ERROR_TYPE_MISMATCH for local double

 test_local_integer: END


 test_local_invalid: BEGIN

 GetLocalInt: JVMTI error (0)
 FAIL: GetLocalInt failed to return JVMTI_ERROR_INVALID_SLOT for local invalid
 GetLocalLong: JVMTI error (35)
 GetLocalLong returned JVMTI_ERROR_INVALID_SLOT for local invalid as expected
 GetLocalFloat: JVMTI error (0)
 FAIL: GetLocalFloat failed to return JVMTI_ERROR_INVALID_SLOT for local invalid
 GetLocalDouble: JVMTI error (35)
 GetLocalDouble returned JVMTI_ERROR_INVALID_SLOT for local invalid as expected

 test_local_invalid: END

@thallium
Copy link
Contributor Author

thallium commented Sep 9, 2022

@fengxue-IS @babsingh FYI

runtime/jvmti/jvmtiLocalVariable.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiStackFrame.c Show resolved Hide resolved
@fengxue-IS fengxue-IS added the project:loom Used to track Project Loom related work label Sep 13, 2022
@thallium thallium force-pushed the jvmtiGetOrSetLocal branch 2 times, most recently from d84ea07 to 9b15ac8 Compare September 16, 2022 20:06
@babsingh
Copy link
Contributor

See the top commit in https://github.com/babsingh/openj9/commits/loom_jvmti_8. It has the revised version of the changes in this PR. JVMTI GetOrSetLocal and NotifyFramePop functions need to satisfy the following condition: If thread is NULL, the current thread is used. In this PR, the following code will seg-fault for a null input thread: j9object_t threadObject = J9_JNI_UNWRAP_REFERENCE(thread). It will be quicker to cherry-pick my commit in this PR after reviewing my changes; just add me as a co-author similar to #15914.

The following virtual thread tests successfully passed for my commit:

Note: Testing had to be run with -Xint -Xgcpolicy:nogc since segfaults originating from the GC and hangs from the JIT were seen.

@babsingh
Copy link
Contributor

re #15857 (comment):

Test in question:

The above test is not specific to virtual threads. It has been around since 2018. Even without the changes in this PR, the failure reported in #15857 (comment) will be seen. Currently, this failure is outside the scope of the work to support virtual threads in JVMTI functions. @thallium Please verify that this test fails without the changes in this PR, and then open a Github issue to document the failure. This failure will be addressed in the future.

@babsingh
Copy link
Contributor

babsingh commented Sep 19, 2022

@tajila As per #15857 (comment), I will end up being a co-author in this PR. So, I won't be able to merge this PR. Also, the changes in this PR are very similar to the changes in #15914 in terms of functionality. So, the same reviewer can review both PRs.

Note: There will be merge conflicts between #15857 and #15914 due to some overlap of code.

@thallium thallium changed the title [WIP] Update jvmtiGetOrSetLocal and jvmtiNotifyFramePop to support virtual threads Update jvmtiGetOrSetLocal and jvmtiNotifyFramePop to support virtual threads Sep 19, 2022
@thallium thallium marked this pull request as ready for review September 19, 2022 16:00
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.

will crash for JDK18 and before. suggested ifdefs and minor formatting nitpicks.

runtime/jvmti/jvmtiStackFrame.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiStackFrame.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiStackFrame.c Show resolved Hide resolved
babsingh added a commit to babsingh/openj9 that referenced this pull request Sep 20, 2022
Support for virtual threads added in the following JVMTI functions:
- GetOwnedMonitorInfo
- GetOwnedMonitorStackDepthInfo
- GetCurrentContendedMonitor

Related changes: eclipse-openj9#15857

GetThreadState and GetThreadInfo allow a NULL input thread. In such
a case, they should look at the current thread's object. Because using
the NULL input thread causes a seg fault. This has been fixed.

Related issue: eclipse-openj9#15759

Co-authored-by: Gengchen Tuo <gengchen.tuo@ibm.com>
Co-authored-by: Jack Lu <Jack.S.Lu@ibm.com>
Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/openj9 that referenced this pull request Sep 21, 2022
Support for virtual threads added in the following JVMTI functions:
- GetOwnedMonitorInfo
- GetOwnedMonitorStackDepthInfo
- GetCurrentContendedMonitor

Related changes: eclipse-openj9#15857

GetThreadState and GetThreadInfo allow a NULL input thread. In such
a case, they should look at the current thread's object. Because using
the NULL input thread causes a seg fault. This has been fixed.

Related issue: eclipse-openj9#15759

Co-authored-by: Gengchen Tuo <gengchen.tuo@ibm.com>
Co-authored-by: Jack Lu <Jack.S.Lu@ibm.com>
Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
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.

#15914 was merged. Please resolve the merge conflicts and the last minor grammar nitpick. Then, I will launch the PR builds.

runtime/jvmti/jvmtiStackFrame.c Outdated Show resolved Hide resolved
Adds virtual thread support for GetOrSetLocal and NotifyFramePop

As walkState->walkThread will be used by some jit functions after
stackwalk, we can't use walkContinuationStackFrames which will cause
dangling pointer. Instead we need to stack-allocate manually if a
J9VMContinuation needs to be walked, so we created a helper
getJ9VMContinuationToWalk to get the J9VMContinuation.

Also fix coding style of surrounding code

Issues: eclipse-openj9#15759 eclipse-openj9#15183

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com>
Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@babsingh
Copy link
Contributor

jenkins test sanity win,xlinux jdk19

@babsingh
Copy link
Contributor

jenkins test sanity win,amac jdk11

@babsingh babsingh merged commit c1b2e52 into eclipse-openj9:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project:loom Used to track Project Loom related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants