-
Notifications
You must be signed in to change notification settings - Fork 721
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
Virtual threads support for jvmtiGetFrameCount #15753
Conversation
Try to fetch the new changes from #15690. If it still fails to compile, can you post the compilation error here? |
Actually the error was |
9f741ca
to
5751c74
Compare
If needed, request @EricYangIBM's help for running the below tests:
Instructions which were used for https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/hotspot/jtreg/serviceability/jvmti/thread/GetThreadState
|
5751c74
to
1afcf92
Compare
back to seg fault now:
|
the crash is expected as |
4986887
to
a086b7a
Compare
a086b7a
to
f951801
Compare
with #15788, the GetFrameCount test passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@thallium can you remove the |
@hangshao0 can you take a look |
Can you update the commit message and the description here including more detailed explanation about the solution ? |
Done |
Did you push the change ? I don't see any change. |
Sorry I thought you only want to update the PR message, but I don't see any need to update the commit message? You mean adding the message related to |
You need to update both the commit message and the description of this PR. You can look at other PRs as an example. Also I suggest to mention the issue number in both places. If there is no issue for |
f951801
to
8fea479
Compare
Is it good now? |
c91dc35
to
e282302
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor comment
e282302
to
7955359
Compare
runtime/jvmti/jvmtiStackFrame.c
Outdated
j9object_t contObject = (j9object_t)J9VMJAVALANGVIRTUALTHREAD_CONT(currentThread, threadObject); | ||
internalVMFunctions->walkContinuationStackFrames(currentThread, contObject, &walkState); | ||
} else { | ||
Assert_JVMTI_true(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can add a new Assert_JVMTI_unreachable
to j9jvmti.tdf
Adds `Assert_JVMTI_unreachable` to mark places as unreachable Adds `walkContinuationStackFrames` to internal functions so jvmti code can use it. Adds a check for whether it's a virtual thread and not yielded or terminated. If it is, use `walkContinuationStackFrames` to walk the continuation object. Issue: eclipse-openj9#15183 eclipse-openj9#15759 Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
7955359
to
2aa6979
Compare
jenkins compile win jdk8 |
jenkins test sanity amac jdk19 |
jenkins test sanity plinux jdk19 |
* move new tracepoint to the end * fix illegal C code (move declaration) Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>
My concerns above are addressed by #15813. |
re #15753 (comment): @thallium Did the GetFrameCount/framecnt01 test in the above comment pass with these changes? |
It’s mostly due to getStackTrace hasn’t been updated, it passed with my PR on updating getStackTrace |
Adds
walkContinuationStackFrames
to internal functions so jvmti code can use it.Adds a check for whether it's a virtual thread and not yielded or terminated.
If it is, use
walkContinuationStackFrames
to walk the continuation object.Issue: #15183 #15759