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 JVMTI_HEAP_REFERENCE_JNI_LOCAL scan/iterate code #18416

Open
fengxue-IS opened this issue Nov 7, 2023 · 3 comments · May be fixed by #18864
Open

Fix JVMTI_HEAP_REFERENCE_JNI_LOCAL scan/iterate code #18416

fengxue-IS opened this issue Nov 7, 2023 · 3 comments · May be fixed by #18864

Comments

@fengxue-IS
Copy link
Contributor

Based on discussion from #18394:

wrapObjectIterationCallback called in jvmtiIterateOverReachableObjects uses event->type to differentiate reference kind and which callback to use which uses (J9WalkState *) referrer casting for stack type.
Previously GC would report JNI local refs in the first 16 slots as STACK_LOCAL (stored on the NativeCallout frame) and any after that as JNI_LOCAL (frame stored in the jniLocalReferences list). The JVMTI code is required to correct JNI refs reported as STACK_LOCAL before calling user stack ref callback while JNI_LOCAL would be incorrectly sent to user's heap root callback.

With the code change from #18378 and #18379, all JNI locals should be correctly reported, but since they still map to J9JVMTI_HEAP_EVENT_ROOT, this cause call jni refs to be sent to user's heap root callback which doesn't match the JVMTI spec https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#IterateOverReachableObjects

Changes required:

  1. map J9GC_ROOT_TYPE_JNI_LOCAL to J9JVMTI_HEAP_EVENT_STACK
  2. refactor wrapObjectIterationCallback to correctly handle jni refs that doesn't come from stack walking
  3. fix processStackRoot code to first recognize references kind rather than relying on walkState.
  4. fix function parameter type(Pass NULL walkState to jvmti callback for JNI local reference on stack #18394 (comment))

I will create a draft PR on the above changes as a starting point for further discussion.
FYI @LinHu2016 @amicic @babsingh @keithc-ca

@babsingh
Copy link
Contributor

As per #18426 (comment), moving this issue to the 0.48 release.

@babsingh
Copy link
Contributor

@fengxue-IS For 0.48, this issue will need to be resolved by the end of this week. What's the current state of this issue? Based on this issue's impact, do we need it to be fixed in 0.48 or can it be pushed to 0.49?

@fengxue-IS
Copy link
Contributor Author

The issue is a legacy one, so it doesn't need to be fixed in 0.48, but I should be the PR comments addressed today, so we can decide to push to 0.49 or not based on the review result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants