From aecb6270a45b7e99cfbe9b833ba7972879aaa2c0 Mon Sep 17 00:00:00 2001 From: Irwin D'Souza Date: Tue, 2 Apr 2024 13:56:24 -0400 Subject: [PATCH 1/3] Fix acquireVMaccessIfNeeded for Non-Comp Threads Whena a non-comp thread invokes acquireVMaccessIfNeeded, the function silently returns, because it assumes that a non-comp thread should already have had VMAccess. This is particularly problematic when using TR::VMAccessCriticalSection, as it is not at all obvious that the current thread may not actually have VMAccess. Furthermore, there are legitmate circusmtances when a non-comp thread will not have VMAccess and will need to use this API to acquire it. Signed-off-by: Irwin D'Souza --- runtime/compiler/env/VMJ9.cpp | 50 ++++++++++++++++------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/runtime/compiler/env/VMJ9.cpp b/runtime/compiler/env/VMJ9.cpp index 5831a58d16f..acf9f9208df 100644 --- a/runtime/compiler/env/VMJ9.cpp +++ b/runtime/compiler/env/VMJ9.cpp @@ -278,50 +278,46 @@ TR_J9VMBase::instanceOfOrCheckCastNoCacheUpdate(J9Class *instanceClass, J9Class* return ::instanceOfOrCheckCastNoCacheUpdate(instanceClass, castClass); } -// Points to address: what if vmThread is not the compilation thread. // What if the compilation thread does not have the classUnloadMonitor. // What if jitConfig does not exist. // What if we already have the compilationShouldBeInterrupted flag set. // How do we set the error code when we throw an exception -// IMPORTANT: acquireVMAccessIfNeeded could throw an exception, -// hence it is important to call this within a try block. +// IMPORTANT: acquireVMAccessIfNeeded could throw an exception if the vmThread +// **IS** a Compilation Thread; hence it is important to call this within a try +// block. bool acquireVMaccessIfNeeded(J9VMThread *vmThread, TR_YesNoMaybe isCompThread) { - bool haveAcquiredVMAccess = false; - if (TR::Options::getCmdLineOptions() == 0 || // if options haven't been created yet, there is no danger - TR::Options::getCmdLineOptions()->getOption(TR_DisableNoVMAccess)) - return false; // don't need to acquire VM access - // we need to test if the thread has VM access TR_ASSERT(vmThread, "vmThread must be not null"); - // We need to acquire VMaccess only for the compilation thread - if (isCompThread == TR_no) - { - TR_ASSERT((vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS), "Must have vm access if this is not a compilation thread"); - return false; - } - - // scan all compilation threads - J9JITConfig *jitConfig = vmThread->javaVM->jitConfig; - TR::CompilationInfo *compInfo = TR::CompilationInfo::get(jitConfig); - TR::CompilationInfoPerThread *compInfoPT = compInfo->getCompInfoForThread(vmThread); - - // We need to acquire VMaccess only for the compilation thread - if (isCompThread == TR_maybe) + bool haveAcquiredVMAccess = false; + J9JITConfig *jitConfig = vmThread->javaVM->jitConfig; + TR::CompilationInfo *compInfo = TR::CompilationInfo::get(jitConfig); + TR::CompilationInfoPerThread *compInfoPT = isCompThread != TR_no + ? compInfo->getCompInfoForThread(vmThread) + : NULL; + + // If the current thread is not a compilation thread, acquire VM Access + // and return immediately. + if (!compInfoPT) { - if (!compInfoPT) + if (!(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS)) { - TR_ASSERT((vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS), "Must have vm access if this is not a compilation thread"); - return false; + acquireVMAccessNoSuspend(vmThread); + haveAcquiredVMAccess = true; } + return haveAcquiredVMAccess; } // At this point we know we deal with a compilation thread - TR_ASSERT(compInfoPT, "A compilation thread must have an associated compilationInfo"); - if (!(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS)) // I don't already have VM access + if (TR::Options::getCmdLineOptions() == 0 || // if options haven't been created yet, there is no danger + TR::Options::getCmdLineOptions()->getOption(TR_DisableNoVMAccess)) + return false; // don't need to acquire VM access + + // I don't already have VM access + if (!(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS)) { if (0 == vmThread->javaVM->internalVMFunctions->internalTryAcquireVMAccessWithMask(vmThread, J9_PUBLIC_FLAGS_HALT_THREAD_ANY_NO_JAVA_SUSPEND)) { From eb960eb93903c1f9a70d6e7024746f21a08078ce Mon Sep 17 00:00:00 2001 From: Irwin D'Souza Date: Tue, 2 Apr 2024 14:09:49 -0400 Subject: [PATCH 2/3] Enforce VMAccess when calling compileMethod Signed-off-by: Irwin D'Souza --- runtime/compiler/control/CompilationThread.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/runtime/compiler/control/CompilationThread.cpp b/runtime/compiler/control/CompilationThread.cpp index a8f48c22777..12112d980a3 100644 --- a/runtime/compiler/control/CompilationThread.cpp +++ b/runtime/compiler/control/CompilationThread.cpp @@ -5806,12 +5806,16 @@ static void deleteMethodHandleRef(J9::MethodHandleThunkDetails & details, J9VMTh vmThread->javaVM->internalVMFunctions->j9jni_deleteGlobalRef((JNIEnv*)vmThread, (jobject)details.getArgRef(), false); } - +/** + * @attention Must be called with VMAccess in hand. + */ void *TR::CompilationInfo::compileMethod(J9VMThread * vmThread, TR::IlGeneratorMethodDetails & details, void *oldStartPC, TR_YesNoMaybe requireAsyncCompile, TR_CompilationErrorCode *compErrCode, bool *queued, TR_OptimizationPlan * optimizationPlan) { + TR_ASSERT_FATAL(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS, "%p does not have VMAccess\n", vmThread); + TR_J9VMBase * fe = TR_J9VMBase::get(_jitConfig, vmThread); TR_ASSERT(!fe->isAOT_DEPRECATED_DO_NOT_USE(), "We need a non-AOT vm here."); @@ -5952,6 +5956,9 @@ extern bool validateSharedClassAOTHeader(J9JavaVM *javaVM, J9VMThread *curThread, TR::CompilationInfo *compInfo, TR_FrontEnd *fe); #endif +/** + * @attention Must be called with VMAccess in hand. + */ void *TR::CompilationInfo::compileOnSeparateThread(J9VMThread * vmThread, TR::IlGeneratorMethodDetails & details, void *oldStartPC, TR_YesNoMaybe requireAsyncCompile, TR_CompilationErrorCode *compErrCode, From 48359c533d7f08c117f5651693f6bc71221315d0 Mon Sep 17 00:00:00 2001 From: Irwin D'Souza Date: Tue, 2 Apr 2024 14:10:08 -0400 Subject: [PATCH 3/3] Use TR::VMAccessCriticalSection in dumpIPBCDataCallGraph Signed-off-by: Irwin D'Souza --- runtime/compiler/runtime/IProfiler.cpp | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/runtime/compiler/runtime/IProfiler.cpp b/runtime/compiler/runtime/IProfiler.cpp index ef986d71fb0..93e5cc72eed 100644 --- a/runtime/compiler/runtime/IProfiler.cpp +++ b/runtime/compiler/runtime/IProfiler.cpp @@ -50,6 +50,7 @@ #include "env/PersistentInfo.hpp" #include "env/jittypes.h" #include "env/VerboseLog.hpp" +#include "env/VMAccessCriticalSection.hpp" #include "il/Block.hpp" #include "il/DataTypes.hpp" #include "il/Node.hpp" @@ -4814,23 +4815,12 @@ void TR_IProfiler::dumpIPBCDataCallGraph(J9VMThread* vmThread) return; } - // Need to have VM access to block GC from invalidating entries - // Application threads may still add data, but that is safe - // Ideally we should use TR::VMAccessCriticalSection dumpInfoCS(fe); - // here, but this construct wants asserts that an application thread must have vmAccess. - // This might not be true at shutdown when dumpInfo is likely to be called - // - bool haveAcquiredVMAccess = false; - if (!(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS)) // I don't already have VM access - { - acquireVMAccessNoSuspend(vmThread); - haveAcquiredVMAccess = true; - } - J9JavaVM *javaVM = vmThread->javaVM; J9InternalVMFunctions *vmFunctions = javaVM->internalVMFunctions; TR_J9VMBase * fe = TR_J9VMBase::get(javaVM->jitConfig, vmThread); + TR::VMAccessCriticalSection dumpCallGraph(fe); + fprintf(stderr, "Aggregating per method ...\n"); for (int32_t bucket = 0; bucket < TR::Options::_iProfilerBcHashTableSize; bucket++) { @@ -4883,8 +4873,6 @@ void TR_IProfiler::dumpIPBCDataCallGraph(J9VMThread* vmThread) } } aggregationHT.sortByNameAndPrint(fe); - if (haveAcquiredVMAccess) - releaseVMAccessNoSuspend(vmThread); fprintf(stderr, "Finished dumping info\n"); }