Pass NULL walkState to jvmti callback for JNI local reference on stack

[LinHu2016]

We could not pass walkState parameter to jvmti callback for JNI local reference on stack (except normal Stack slot), it could cause jvmti crash, so pass walkState == NULL for the case.

fix: #18392
Signed-off-by: hulin [email protected]

[amicic]

Not expert on this, but at least I can see it's consistent with how we call it when we find it as a JNI thread slot (not a stack one).

[amicic]

jenkins compile win jdk21

[amicic]

@fengxue-IS @babsingh

is there a value in specializing code

	case J9GC_ROOT_TYPE_JNI_LOCAL:
		event->type = J9JVMTI_HEAP_EVENT_ROOT;

so that the type is set to ROOT or SLOT respective to if referrer is NULL or non-null?

[amicic]

In other words, the fact that we will now pass NULL as referrer will mislead a callback believing this is a thread root slot, rather then a stack one. Could that a problem for any callback that you can think of?

Perhaps, the answer is that this reference being on a stack is just a side effect of an optimization that a first few are put on the stack rather than on thread local list, but effectively regardless where they are stored they should be treated as if they were stored into the list?

[keithc-ca]

I thought the cast was suspicious when I saw #18379, NULL seems more reasonable, but why should the call on line 651 not also pass NULL?

[amicic]

A valid question. In theory, callbacks being aware that the type is STACK_SLOT could safely cast the referrer parameter back to walkState type and do something about that info/structure (print stack slot address etc), but I don't know if they do.

I'm ok with passing NULL, but don't have the whole picture (history and awareness of current callbacks) to say it's really safe.

[fengxue-IS]

If we pass NULL for STACK_SLOT, it will break the expectation in jvmtiHeap10.c::processStackRoot() function.

[fengxue-IS]

@fengxue-IS @babsingh

is there a value in specializing code

	case J9GC_ROOT_TYPE_JNI_LOCAL:
		event->type = J9JVMTI_HEAP_EVENT_ROOT;

so that the type is set to ROOT or SLOT respective to if referrer is NULL or non-null?

After reviewing the VM code which uses event->type, the only difference between J9JVMTI_HEAP_EVENT_ROOT and J9JVMTI_HEAP_EVENT_STACK is that stack passes walkState where as root passes NULL as referrer.

the only use case that I see in the VM is in wrapObjectIterationCallback which uses walkState to check the slot type to differentiate JNI vs Stack.

This callback is used by jvmtiIterateOverReachableObjects and jvmtiIterateOverObjectsReachableFromObject
specifically in IterateOverReachableObjects which is the only code path that would make use of J9JVMTI_HEAP_EVENT_ROOT. Based on the requirements outlined in the jvmti spec https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#IterateOverReachableObjects, it seems the current behaviour is still not fully correct and we just moved from one issue to another. (JNI_Local should be reported as Stack reference and processed by the stack ref call back)

Given that there is no testing that reflect this problem, I will open a new issue to highlight failure case in the current implementation and work on a PR to correct this.

[keithc-ca]

If we continue to pass walkState to doSlot() and ultimately to the supplied callback, I think void * is a better type for those parameters (as opposed to J9Object *).