Address review comments

Add assertion to j9gc_ext calls Update formatting Signed-off-by: Jack Lu <[email protected]>
eclipse-openj9 · Sep 25, 2024 · 10c72e1 · 10c72e1
1 parent 2b0aa8c
commit 10c72e1
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 41 deletions.
diff --git a/runtime/gc_base/ReferenceChainWalker.cpp b/runtime/gc_base/ReferenceChainWalker.cpp
@@ -80,6 +80,7 @@ j9gc_ext_reachable_objects_do(
 	uintptr_t walkFlags)
 {
 	MM_EnvironmentBase *env = MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);
+	Assert_MM_mustHaveExclusiveVMAccess(env->getOmrVMThread());
 
 	/* Make sure the heap is walkable (flush TLH's, secure heap integrity) */
 	vmThread->javaVM->memoryManagerFunctions->j9gc_flush_caches_for_walk(vmThread->javaVM);
@@ -117,6 +118,7 @@ j9gc_ext_reachable_from_object_do(
 	uintptr_t walkFlags)
 {
 	MM_EnvironmentBase *env = MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);
+	Assert_MM_mustHaveExclusiveVMAccess(env->getOmrVMThread());
 
 	/* Make sure the heap is walkable (flush TLH's, secure heap integrity) */
 	vmThread->javaVM->memoryManagerFunctions->j9gc_flush_caches_for_walk(vmThread->javaVM);
@@ -661,7 +663,7 @@ MM_ReferenceChainWalker::doStackSlot(J9Object **slotPtr, void *walkState, const
 			((J9StackWalkState *)walkState)->walkThread->threadObject = _vThreadObject;
 		}
 #endif /* JAVA_SPEC_VERSION >= 19 */
-		J9MM_StackSlotDescriptor stackSlotDescriptor = { ((J9StackWalkState *)walkState)->walkThread, (J9StackWalkState *)walkState };
+		J9MM_StackSlotDescriptor stackSlotDescriptor = {((J9StackWalkState *)walkState)->walkThread, (J9StackWalkState *)walkState};
 		if (J9_STACKWALK_SLOT_TYPE_JNI_LOCAL == ((J9StackWalkState *)walkState)->slotType) {
 			doSlot(slotPtr, J9GC_ROOT_TYPE_JNI_LOCAL, -1, (J9Object *)&stackSlotDescriptor);
 		} else {
@@ -693,12 +695,11 @@ MM_ReferenceChainWalker::doVMThreadSlot(J9Object **slotPtr, GC_VMThreadIterator
 	case vmthreaditerator_state_slots:
 		doSlot(slotPtr, J9GC_ROOT_TYPE_THREAD_SLOT, -1, NULL);
 		break;
-	case vmthreaditerator_state_jni_slots:
-		{
-			J9MM_StackSlotDescriptor stackSlotDescriptor = {vmThreadIterator->getVMThread(), NULL};
-			doSlot(slotPtr, J9GC_ROOT_TYPE_JNI_LOCAL, -1, (J9Object*)&stackSlotDescriptor);
-			break;
-		}
+	case vmthreaditerator_state_jni_slots: {
+		J9MM_StackSlotDescriptor stackSlotDescriptor = {vmThreadIterator->getVMThread(), NULL};
+		doSlot(slotPtr, J9GC_ROOT_TYPE_JNI_LOCAL, -1, (J9Object *)&stackSlotDescriptor);
+		break;
+	}
 #if defined(J9VM_INTERP_HOT_CODE_REPLACEMENT)
 	case vmthreaditerator_state_monitor_records:
 		if (isHeapObject(slotValue) && !_heap->objectIsInGap(slotValue)) {

diff --git a/runtime/gc_base/ReferenceChainWalker.hpp b/runtime/gc_base/ReferenceChainWalker.hpp
@@ -75,8 +75,8 @@ class MM_ReferenceChainWalker : public MM_RootScanner
 	bool _isTerminating; /**< Set when no more callbacks should be queued */
 	bool _shouldPreindexInterfaceFields; /**< if true, indexes interface fields of the class being visited before class and superclass fields, otherwise, returns them in the order they appear in an object instance (CMVC 142897) */
 #if JAVA_SPEC_VERSION >= 19
-	bool _includeVThreadObject; /**< Set when VirtualThread object is needed by callback function. */
-	J9Object *_vThreadObject; /**< Cached object ref of VirtualThread object. */
+	bool _includeVThreadObject; /**< Set when VirtualThread object is needed by callback function */
+	J9Object *_vThreadObject; /**< Cached object ref of VirtualThread object */
 #endif /* JAVA_SPEC_VERSION >= 19 */
 	MM_ReferenceChainWalkerMarkMap *_markMap;	/**< Mark Map created for Reference Chain Walker */
 	MM_Heap *_heap; /**< Cached pointer to the heap */
@@ -271,6 +271,7 @@ class MM_ReferenceChainWalker : public MM_RootScanner
 #if JAVA_SPEC_VERSION >= 19
 	void includeVThreadObject() { _includeVThreadObject = true; }
 #endif /* JAVA_SPEC_VERSION >= 19 */
+
 	/**
 	 * Added to support bi-modal interface indexing in JVMTI (CMVC 142897).
 	 * Detail:  heap10 requires no pre-indexing in order to preserve Java5 behaviour but heap11 requires pre-indexing to pass a Java6 JCK

diff --git a/runtime/gc_include/HeapIteratorAPI.h b/runtime/gc_include/HeapIteratorAPI.h
@@ -165,8 +165,8 @@ typedef struct J9MM_HeapRootSlotDescriptor {
 } J9MM_HeapRootSlotDescriptor;
 
 typedef struct J9MM_StackSlotDescriptor {
-	J9VMThread *vmThread; /**< Thread containing the stack slot. */
-	J9StackWalkState *walkState; /**< WalkState that found the slot, or null if found directly in thread local tables. */
+	J9VMThread *vmThread; /**< Thread containing the stack slot */
+	J9StackWalkState *walkState; /**< WalkState that found the slot, or null if found directly in thread local tables */
 } J9MM_StackSlotDescriptor;
 
 typedef jvmtiIterationControl (*rootIteratorCallBackFunc)(void* ptr, J9MM_HeapRootSlotDescriptor *rootDesc, void *userData);

diff --git a/runtime/jvmti/jvmtiHeap.c b/runtime/jvmti/jvmtiHeap.c
@@ -840,16 +840,16 @@ wrap_heapReferenceCallback(J9JavaVM * vm, J9JVMTIHeapData * iteratorData)
  *
  */
 static UDATA
-jvmtiHeapFollowRefs_getStackData(J9JVMTIHeapData * iteratorData, J9MM_StackSlotDescriptor *stackSlotDescriptor)
+jvmtiHeapFollowRefs_getStackData(J9JVMTIHeapData *iteratorData, J9MM_StackSlotDescriptor *stackSlotDescriptor)
 {
 	J9JVMTIHeapEvent * e = &iteratorData->event;
 	jint slot = -1;
 	jint depth = -1;
 	J9Method *ramMethod = NULL;
 	jmethodID method = (jmethodID) -1;
-	J9JVMTIObjectTag search;
-	J9JVMTIObjectTag *result;
-	jlong threadID;
+	J9JVMTIObjectTag search = {NULL, 0};
+	J9JVMTIObjectTag *result = NULL;
+	jlong threadID = 0;
 	J9StackWalkState *walkState = stackSlotDescriptor->walkState;
 
 	if (NULL != walkState) {
@@ -860,15 +860,15 @@ jvmtiHeapFollowRefs_getStackData(J9JVMTIHeapData * iteratorData, J9MM_StackSlotD
 		/* Convert internal slot type to JVMTI type */
 
 		switch (walkState->slotType) {
-			case J9_STACKWALK_SLOT_TYPE_JNI_LOCAL:
-				e->refKind = JVMTI_HEAP_REFERENCE_JNI_LOCAL;
-				break;
-			case J9_STACKWALK_SLOT_TYPE_METHOD_LOCAL:
-				e->refKind = JVMTI_HEAP_REFERENCE_STACK_LOCAL;
-				break;
-			default:
-				/* Do not callback with stack slot types other then JNI or STACK Local */
-				return 0;
+		case J9_STACKWALK_SLOT_TYPE_JNI_LOCAL:
+			e->refKind = JVMTI_HEAP_REFERENCE_JNI_LOCAL;
+			break;
+		case J9_STACKWALK_SLOT_TYPE_METHOD_LOCAL:
+			e->refKind = JVMTI_HEAP_REFERENCE_STACK_LOCAL;
+			break;
+		default:
+			/* Do not callback with stack slot types other then JNI or STACK Local. */
+			return 0;
 		}
 
 		/* If there's no method, slot, method and depth are set to -1 by default. */
@@ -889,10 +889,10 @@ jvmtiHeapFollowRefs_getStackData(J9JVMTIHeapData * iteratorData, J9MM_StackSlotD
 	if (NULL != search.ref) {
 		result = hashTableFind(iteratorData->env->objectTagTable, &search);
 
-		/* Figure out the Thread ID */
+		/* Retrieve the Thread ID. */
 		threadID = J9VMJAVALANGTHREAD_TID(iteratorData->currentThread, stackSlotDescriptor->vmThread->threadObject);
 	} else {
-		/* walking a Continuation, ref to threadObject is lost, set 0 for tag/ID. */
+		/* Set 0 for tag/ID since ref to threadObject can be lost while walking a continuation. */
 		result = NULL;
 		threadID = 0;
 	}

diff --git a/runtime/jvmti/jvmtiHeap10.c b/runtime/jvmti/jvmtiHeap10.c
@@ -462,38 +462,37 @@ processObjectRef(J9JVMTIObjectIteratorData * data, J9JVMTIObjectTag * entry, jlo
 
 
 static jvmtiIterationControl
-processStackRoot(J9JVMTIObjectIteratorData * data, J9JVMTIObjectTag * entry, jlong size, jlong classTag, jvmtiHeapRootKind kind, J9MM_StackSlotDescriptor *stackSlotDescriptor)
+processStackRoot(J9JVMTIObjectIteratorData *data, J9JVMTIObjectTag *entry, jlong size, jlong classTag, jvmtiHeapRootKind kind, J9MM_StackSlotDescriptor *stackSlotDescriptor)
 {
 	jint slot = -1;
 	jint depth = -1;
 	J9Method *ramMethod = NULL;
 	jmethodID method = (jmethodID) -1;
-	J9JVMTIObjectTag search;
-	J9JVMTIObjectTag *result;
+	J9JVMTIObjectTag search = {NULL, 0};
+	J9JVMTIObjectTag *result = NULL;
 
 	if (NULL != stackSlotDescriptor->walkState) {
 		J9StackWalkState *walkState = stackSlotDescriptor->walkState;
 		slot = (jint) walkState->slotIndex;
 		depth = (jint) walkState->framesWalked;
 		ramMethod = walkState->method;
 
-		/* Convert internal slot type to JVMTI type */
+		/* Convert internal slot type to JVMTI type. */
 
 		switch (walkState->slotType) {
-			case J9_STACKWALK_SLOT_TYPE_JNI_LOCAL:
-				kind = JVMTI_HEAP_ROOT_JNI_LOCAL;
-				break;
-			case J9_STACKWALK_SLOT_TYPE_METHOD_LOCAL:
-				kind = JVMTI_HEAP_ROOT_STACK_LOCAL;
-				break;
-			default:
-				kind = JVMTI_HEAP_ROOT_JNI_LOCAL;
-				ramMethod = NULL;
-				break;
+		case J9_STACKWALK_SLOT_TYPE_JNI_LOCAL:
+			kind = JVMTI_HEAP_ROOT_JNI_LOCAL;
+			break;
+		case J9_STACKWALK_SLOT_TYPE_METHOD_LOCAL:
+			kind = JVMTI_HEAP_ROOT_STACK_LOCAL;
+			break;
+		default:
+			kind = JVMTI_HEAP_ROOT_JNI_LOCAL;
+			ramMethod = NULL;
+			break;
 		}
 
 		/* If there's no method, slot, method and depth are set to -1 by default. */
-
 		if (NULL != ramMethod) {
 			/* Cheating here - should be current thread, but the walk thread will do. */
 			method = getCurrentMethodID(walkState->walkThread, ramMethod);