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 jvmtiGetStackTrace to support virtual thread #15788

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

thallium
Copy link
Contributor

@thallium thallium commented Aug 29, 2022

Refactors vm->internalVMFunctions to vmFuncs
Adds a continuation object parameter to jvmtiInternalGetStackTrace.
If targetThread is not null, the targetThread will be used, otherwise
check if continuation object is not null and use that.

In jvmtiGetStackTrace, adds a check for virtual thread, call
jvmtiInternalGetStackTrace with the corresponding arguments depending
on whether the thread is virtual or not.

Adds an assertion to getVMThread to ensure that targetThread will be
NULL for a virtual thread so the extra call to IS_VIRTUAL_THREAD can be
avoided.

Updates jvmtiGetFrameCount and jvmtiGetFrameLocation to remove the
unnecessary IS_VIRTUAL_THREAD call.

Updates the coding style of surrounding code

Issues: #15759 #15183

Signed-off-by: Gengchen Tuo gengchen.tuo@ibm.com

@thallium
Copy link
Contributor Author

@fengxue-IS can you please take a look?

Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

@thallium
Copy link
Contributor Author

thallium commented Aug 29, 2022

LGTM, can you verify that this change passes all the test under https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace

Actually 4 tests failed, 2 are PopFrame failed caused by JVMTI_ERROR_THREAD_NOT_SUSPENDED, and two are unmatched stack trace.

@fengxue-IS
Copy link
Contributor

for unmatched stacktrace, if it is due to Continuation.enter/Continuation.enter0/Continuation.yield0 then that is expected.
PopFrame failed is expected as that have not been updated

@thallium
Copy link
Contributor Author

In test case 5:
expected: 11, actual: 10 (no stacktrace printed)
In test case 3:
expected:

    {"Ljava/lang/Object;", "wait", "()V"},
    {"Lgetstacktr03;", "dummy", "()V"},
    {"Lgetstacktr03;", "chain", "()V"},
    {"Lgetstacktr03$Task;", "run", "()V"},
    {"Ljava/lang/Thread;", "run", "()V"}

got:

 0: java/lang/Object: wait(JI)V
 1: java/lang/Object: wait()V
 2: getstacktr03: dummy()V
 3: getstacktr03: chain()V
 4: getstacktr03$Task: run()V
 5: java/lang/Thread: run()V

expected:

     {"Ljava/lang/Object;", "wait", "()V"},
    {"Lgetstacktr03;", "dummy", "()V"},
    {"Lgetstacktr03;", "chain", "()V"},
    {"Lgetstacktr03$Task;", "run", "()V"},
    {"Ljava/lang/VirtualThread;", "run", "(Ljava/lang/Runnable;)V"},
    {"Ljava/lang/VirtualThread$VThreadContinuation;", "lambda$new$0", "(Ljava/lang/VirtualThread;Ljava/lang/Runnable;)V"},
    {"Ljava/lang/VirtualThread$VThreadContinuation$$Lambda$31.0x0000000800098810;", "run", "()V"},
    {"Ljdk/internal/vm/Continuation;", "enter0", "()V"},
    {"Ljdk/internal/vm/Continuation;", "enter", "(Ljdk/internal/vm/Continuation;Z)V"}

got:

 0: java/lang/Object: wait(JI)V
 1: java/lang/Object: wait()V
 2: getstacktr03: dummy()V
 3: getstacktr03: chain()V
 4: getstacktr03$Task: run()V
 5: java/lang/VirtualThread: run(Ljava/lang/Runnable;)V
 6: java/lang/VirtualThread$VThreadContinuation: lambda$new$0(Ljava/lang/VirtualThread;Ljava/lang/Runnable;)V
 7: java/lang/VirtualThread$VThreadContinuation$$Lambda$6/0x00000000140577d8: run()V
 8: jdk/internal/vm/Continuation: execute(Ljdk/internal/vm/Continuation;)V

@fengxue-IS
Copy link
Contributor

Based on the stacktrace posted, looks like the unmatched portion is with Object.wait() and Continuation.enter which both uses OpenJ9's implementation, so we can treat those as passed.

@hangshao0 hangshao0 added comp:vm project:loom Used to track Project Loom related work labels Aug 30, 2022
@hangshao0 hangshao0 added this to the Java 19 milestone Aug 30, 2022
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
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
Copy link
Contributor

babsingh commented Sep 6, 2022

Also, remove markdown syntax from the commit message, and make sure the commit message body is wrapped at 72 chars, where reasonable. Ref: https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md#commit-guidelines.

@thallium thallium force-pushed the jvmtiGetStackTrace_PR branch 3 times, most recently from e46b1eb to f5c5eda Compare September 7, 2022 19:37
@babsingh
Copy link
Contributor

babsingh commented Sep 7, 2022

jenkins test sanity win,xlinux jdk11,jdk19

Refactors vm->internalVMFunctions to vmFuncs
Adds a continuation object parameter to jvmtiInternalGetStackTrace.
If targetThread is not null, the targetThread will be used, otherwise
check if continuation object is not null and use that.

In jvmtiGetStackTrace, adds a check for virtual thread, call
jvmtiInternalGetStackTrace with the corresponding arguments depending
on whether the thread is virtual or not.

Adds an assertion to getVMThread to ensure that targetThread will be
NULL for a virtual thread so the extra call to IS_VIRTUAL_THREAD can be
avoided.

Updates jvmtiGetFrameCount and jvmtiGetFrameLocation to remove the
unnecessary IS_VIRTUAL_THREAD call.

Updates the coding style of surrounding code

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

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@babsingh
Copy link
Contributor

babsingh commented Sep 8, 2022

jenkins test sanity win,xlinux jdk11,jdk19

@babsingh
Copy link
Contributor

babsingh commented Sep 8, 2022

@thallium PR builds have passed. Did your local reruns of the JVMTI serviceability tests from the extension repo pass with this PR?

@thallium
Copy link
Contributor Author

thallium commented Sep 8, 2022

@thallium PR builds have passed. Did your local reruns of the JVMTI serviceability tests from the extension repo pass with this PR?

Results stays the same: GetFrameCount and GetFrameLocation passed and GetStackTrace 3/7 passed as expected.

@babsingh babsingh merged commit 0ed4cfa into eclipse-openj9:master Sep 8, 2022
babsingh added a commit to babsingh/openj9 that referenced this pull request Sep 12, 2022
- Define vmFuncs only within the scope of its usage.
- Initialize local variables.
- Fix formatting.
- Use Assert_JVMTI_unreachable instead of Assert_JVMTI_true.

Related: eclipse-openj9#15788, eclipse-openj9#15813

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm jdk19 project:loom Used to track Project Loom related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants