Add a walk state flag to skip methods with the JvmtiMountTransition annotation #17758
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nnotation Since it's only related to JVMTI, a walk state flag is added to enable the behaviour when doing JVMTI-related stack walk. Related: eclipse-openj9#17490 Signed-off-by: Gengchen Tuo <[email protected]>
|
@babsingh FYI |
|
The current title exceeds the allowed character limit. Revised commit message and PR description: |
babsingh
requested changes
Jul 10, 2023
Comment on lines
+377
to
379
| #define J9_IS_JVMTI_MOUNT_TRANSITION(method) \ | ||
| ((NULL != (method)) && (J9_ARE_ANY_BITS_SET(getExtendedModifiersDataFromROMMethod(J9_ROM_METHOD_FROM_RAM_METHOD(method)), CFR_METHOD_EXT_JVMTIMOUNTTRANSITION_ANNOTATION))) | ||
| #if defined(OPENJ9_BUILD) |
There was a problem hiding this comment.
Add a new line and wrap with JAVA_SPEC_VERSION >= 20 to avoid compilation errors since CFR_METHOD_EXT_JVMTIMOUNTTRANSITION_ANNOTATION is only defined in JDK20+.
Suggested change
| #define J9_IS_JVMTI_MOUNT_TRANSITION(method) \ | |
| ((NULL != (method)) && (J9_ARE_ANY_BITS_SET(getExtendedModifiersDataFromROMMethod(J9_ROM_METHOD_FROM_RAM_METHOD(method)), CFR_METHOD_EXT_JVMTIMOUNTTRANSITION_ANNOTATION))) | |
| #if defined(OPENJ9_BUILD) | |
| #if JAVA_SPEC_VERSION >= 20 | |
| #define J9_IS_JVMTI_MOUNT_TRANSITION(method) \ | |
| ((NULL != (method)) && (J9_ARE_ANY_BITS_SET(getExtendedModifiersDataFromROMMethod(J9_ROM_METHOD_FROM_RAM_METHOD(method)), CFR_METHOD_EXT_JVMTIMOUNTTRANSITION_ANNOTATION))) | |
| #endif /* JAVA_SPEC_VERSION >= 20 */ | |
| #if defined(OPENJ9_BUILD) |
Comment on lines
+484
to
+489
| /* Skip methods with JvmtiMountTransition annotation */ | ||
| if (J9_ARE_ALL_BITS_SET(walkState->flags, J9_STACKWALK_SKIP_JVMTI_MOUNT_TRANSITION_FRAMES) | ||
| && J9_IS_JVMTI_MOUNT_TRANSITION(walkState->method) | ||
| ) { | ||
| return J9_STACKWALK_KEEP_ITERATING; | ||
| } |
There was a problem hiding this comment.
To be consistent with #16654, the code changes in this PR should be wrapped with JAVA_SPEC_VERSION >= 20.
Suggested change
| /* Skip methods with JvmtiMountTransition annotation */ | |
| if (J9_ARE_ALL_BITS_SET(walkState->flags, J9_STACKWALK_SKIP_JVMTI_MOUNT_TRANSITION_FRAMES) | |
| && J9_IS_JVMTI_MOUNT_TRANSITION(walkState->method) | |
| ) { | |
| return J9_STACKWALK_KEEP_ITERATING; | |
| } | |
| #if JAVA_SPEC_VERSION >= 20 | |
| /* Skip methods with JvmtiMountTransition annotation. */ | |
| if (J9_ARE_ALL_BITS_SET(walkState->flags, J9_STACKWALK_SKIP_JVMTI_MOUNT_TRANSITION_FRAMES) | |
| && J9_IS_JVMTI_MOUNT_TRANSITION(walkState->method) | |
| ) { | |
| return J9_STACKWALK_KEEP_ITERATING; | |
| } | |
| #endif /* JAVA_SPEC_VERSION >= 20 */ |
| @@ -401,6 +401,7 @@ extern "C" { | |||
| #define J9_STACKWALK_CACHE_METHODS 0x400 | |||
| #define J9_STACKWALK_CACHE_MASK 0x700 | |||
| #define J9_STACKWALK_SKIP_HIDDEN_FRAMES 0x800 | |||
| #define J9_STACKWALK_SKIP_JVMTI_MOUNT_TRANSITION_FRAMES 0x1000 | |||
There was a problem hiding this comment.
Macro definitions don't need JAVA_SPEC_VERSION >= 20.
|
++ @gacholio for a second set of eyes. |
|
This stuff really doesn't belong in the stack walker (I know previous hacks for hidden have made it in). Really, it should be handled in the frame iterators, much like we do to hide the old reflect implementation). |
gacholio
reviewed
Jul 10, 2023
| @@ -1813,7 +1813,7 @@ UDATA | |||
| findDecompileInfo(J9VMThread *currentThread, J9VMThread *targetThread, UDATA depth, J9StackWalkState *walkState) | |||
| { | |||
| walkState->walkThread = targetThread; | |||
| walkState->flags = J9_STACKWALK_ITERATE_FRAMES | J9_STACKWALK_INCLUDE_NATIVES | J9_STACKWALK_VISIBLE_ONLY | J9_STACKWALK_RECORD_BYTECODE_PC_OFFSET | J9_STACKWALK_MAINTAIN_REGISTER_MAP; | |||
| walkState->flags = J9_STACKWALK_ITERATE_FRAMES | J9_STACKWALK_INCLUDE_NATIVES | J9_STACKWALK_VISIBLE_ONLY | J9_STACKWALK_RECORD_BYTECODE_PC_OFFSET | J9_STACKWALK_MAINTAIN_REGISTER_MAP | J9_STACKWALK_SKIP_JVMTI_MOUNT_TRANSITION_FRAMES; | |||
There was a problem hiding this comment.
This is certainly a place that should not be changed - this wants the true view of the stack, not a user-facing one.
|
The only place using the new flag is using it incorrectly as far as I can see, so we should probably just abandon this PR. |
babsingh
pushed a commit
to babsingh/openj9
that referenced
this pull request
Jul 26, 2023
Java methods tagged with the JvmtiMountTransition annotation are involved in transitioning between carrier to virtual threads, and vice-versa. These methods are not part of the thread's scope. So, they are skipped during introspection by the following JVMTI functions: - ForceEarlyReturn - NotifyFramePop - Local Variable (GetOrSetLocal) This PR allows us to match the RI in jvmti/vthread/MethodExitTest. Related eclipse-openj9#17490 Closes eclipse-openj9#17758 Co-authored-by: Gengchen Tuo <[email protected]> Signed-off-by: Babneet Singh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since it's only related to JVMTI, a walk state flag is added to enable the behaviour when doing JVMTI-related stack walk.
Related: #17490