Consolidate all JVMTI jthread decoding.
We had multiple places where we would decode jthreads. This meant that
we did not always check all required constraints before using it. This
CL consolidates all jthread decoding into the
ThreadUtil::GetNativeThread function and adds a helper
ThreadUtil::GetAliveNativeThread function to check the most common
requirements on jthreads passed to JVMTI.
Bug: 66709480
Test: ./test.py --host -j50
Test: cd openjdkjvmti && git grep -W FromManagedThread
Change-Id: Ib6f4bc8510012e0332831bea67e1842a49092917
diff --git a/openjdkjvmti/OpenjdkJvmTi.cc b/openjdkjvmti/OpenjdkJvmTi.cc
index 4339b2b..bac57f9 100644
--- a/openjdkjvmti/OpenjdkJvmTi.cc
+++ b/openjdkjvmti/OpenjdkJvmTi.cc
@@ -1034,14 +1034,13 @@
ENSURE_VALID_ENV(env);
art::Thread* art_thread = nullptr;
if (event_thread != nullptr) {
- // TODO: Need non-aborting call here, to return JVMTI_ERROR_INVALID_THREAD.
+ // TODO The locking around this call is less then what we really want.
art::ScopedObjectAccess soa(art::Thread::Current());
art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
- art_thread = art::Thread::FromManagedThread(soa, event_thread);
-
- if (art_thread == nullptr || // The thread hasn't been started or is already dead.
- art_thread->IsStillStarting()) {
- // TODO: We may want to let the EventHandler know, so it could clean up masks, potentially.
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(event_thread, soa, &art_thread, &err)) {
+ return err;
+ } else if (art_thread->IsStillStarting()) {
return ERR(THREAD_NOT_ALIVE);
}
}
diff --git a/openjdkjvmti/ti_method.cc b/openjdkjvmti/ti_method.cc
index 62603aa..625fb26 100644
--- a/openjdkjvmti/ti_method.cc
+++ b/openjdkjvmti/ti_method.cc
@@ -761,12 +761,10 @@
art::jit::ScopedJitSuspend suspend_jit;
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
GetLocalVariableClosure c(self, depth, slot, type, val);
if (!target->RequestSynchronousCheckpoint(&c)) {
@@ -890,12 +888,10 @@
art::jit::ScopedJitSuspend suspend_jit;
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
SetLocalVariableClosure c(self, depth, slot, type, val);
if (!target->RequestSynchronousCheckpoint(&c)) {
@@ -955,12 +951,10 @@
art::Thread* self = art::Thread::Current();
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
GetLocalInstanceClosure c(self, depth, data);
if (!target->RequestSynchronousCheckpoint(&c)) {
diff --git a/openjdkjvmti/ti_monitor.cc b/openjdkjvmti/ti_monitor.cc
index f92d81e..5a38f46 100644
--- a/openjdkjvmti/ti_monitor.cc
+++ b/openjdkjvmti/ti_monitor.cc
@@ -335,12 +335,10 @@
art::Thread* self = art::Thread::Current();
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
struct GetContendedMonitorClosure : public art::Closure {
public:
@@ -395,7 +393,9 @@
jobject* out_;
};
GetContendedMonitorClosure closure(self, monitor);
- target->RequestSynchronousCheckpoint(&closure);
+ if (!target->RequestSynchronousCheckpoint(&closure)) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
return OK;
}
diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc
index 699f695..d4cc42a 100644
--- a/openjdkjvmti/ti_stack.cc
+++ b/openjdkjvmti/ti_stack.cc
@@ -211,34 +211,6 @@
size_t index = 0;
};
-static jvmtiError GetThread(JNIEnv* env,
- art::ScopedObjectAccessAlreadyRunnable& soa,
- jthread java_thread,
- art::Thread** thread)
- REQUIRES_SHARED(art::Locks::mutator_lock_) // Needed for FromManagedThread.
- REQUIRES(art::Locks::thread_list_lock_) { // Needed for FromManagedThread.
- if (java_thread == nullptr) {
- *thread = art::Thread::Current();
- if (*thread == nullptr) {
- // GetStackTrace can only be run during the live phase, so the current thread should be
- // attached and thus available. Getting a null for current means we're starting up or
- // dying.
- return ERR(WRONG_PHASE);
- }
- } else {
- if (!env->IsInstanceOf(java_thread, art::WellKnownClasses::java_lang_Thread)) {
- return ERR(INVALID_THREAD);
- }
-
- // TODO: Need non-aborting call here, to return JVMTI_ERROR_INVALID_THREAD.
- *thread = art::Thread::FromManagedThread(soa, java_thread);
- if (*thread == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
- }
- }
- return ERR(NONE);
-}
-
jvmtiError StackUtil::GetStackTrace(jvmtiEnv* jvmti_env ATTRIBUTE_UNUSED,
jthread java_thread,
jint start_depth,
@@ -251,19 +223,14 @@
art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
art::Thread* thread;
- jvmtiError thread_error = GetThread(art::Thread::Current()->GetJniEnv(),
- soa,
- java_thread,
- &thread);
- if (thread_error != ERR(NONE)) {
+ jvmtiError thread_error = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(java_thread, soa, &thread, &thread_error)) {
return thread_error;
}
DCHECK(thread != nullptr);
art::ThreadState state = thread->GetState();
- if (state == art::ThreadState::kStarting ||
- state == art::ThreadState::kTerminated ||
- thread->IsStillStarting()) {
+ if (state == art::ThreadState::kStarting || thread->IsStillStarting()) {
return ERR(THREAD_NOT_ALIVE);
}
@@ -714,22 +681,25 @@
art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
art::Thread* thread;
- jvmtiError thread_error = GetThread(art::Thread::Current()->GetJniEnv(),
- soa,
- java_thread,
- &thread);
-
- if (thread_error != ERR(NONE)) {
+ jvmtiError thread_error = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(java_thread, soa, &thread, &thread_error)) {
return thread_error;
}
+
DCHECK(thread != nullptr);
+ art::ThreadState state = thread->GetState();
+ if (state == art::ThreadState::kStarting || thread->IsStillStarting()) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
if (count_ptr == nullptr) {
return ERR(NULL_POINTER);
}
GetFrameCountClosure closure;
- thread->RequestSynchronousCheckpoint(&closure);
+ if (!thread->RequestSynchronousCheckpoint(&closure)) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
*count_ptr = closure.count;
return ERR(NONE);
@@ -793,15 +763,17 @@
art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
art::Thread* thread;
- jvmtiError thread_error = GetThread(art::Thread::Current()->GetJniEnv(),
- soa,
- java_thread,
- &thread);
- if (thread_error != ERR(NONE)) {
+ jvmtiError thread_error = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(java_thread, soa, &thread, &thread_error)) {
return thread_error;
}
DCHECK(thread != nullptr);
+ art::ThreadState state = thread->GetState();
+ if (state == art::ThreadState::kStarting || thread->IsStillStarting()) {
+ return ERR(THREAD_NOT_ALIVE);
+ }
+
if (depth < 0) {
return ERR(ILLEGAL_ARGUMENT);
}
@@ -920,12 +892,10 @@
bool called_method = false;
{
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
if (target != self) {
called_method = true;
@@ -1014,10 +984,11 @@
// have the 'suspend_lock' locked here.
art::ScopedObjectAccess soa(self);
art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
- target = ThreadUtil::GetNativeThread(thread, soa);
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
- } else if (target != self) {
+ jvmtiError err = ERR(INTERNAL);
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
+ }
+ if (target != self) {
// TODO This is part of the spec but we could easily avoid needing to do it. We would just put
// all the logic into a sync-checkpoint.
art::MutexLock tscl_mu(self, *art::Locks::thread_suspend_count_lock_);
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index da1c1bc..907b515 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -161,13 +161,34 @@
}
// Get the native thread. The spec says a null object denotes the current thread.
-art::Thread* ThreadUtil::GetNativeThread(jthread thread,
- const art::ScopedObjectAccessAlreadyRunnable& soa) {
+bool ThreadUtil::GetNativeThread(jthread thread,
+ const art::ScopedObjectAccessAlreadyRunnable& soa,
+ /*out*/ art::Thread** thr,
+ /*out*/ jvmtiError* err) {
if (thread == nullptr) {
- return art::Thread::Current();
+ *thr = art::Thread::Current();
+ return true;
+ } else if (!soa.Env()->IsInstanceOf(thread, art::WellKnownClasses::java_lang_Thread)) {
+ *err = ERR(INVALID_THREAD);
+ return false;
+ } else {
+ *thr = art::Thread::FromManagedThread(soa, thread);
+ return true;
}
+}
- return art::Thread::FromManagedThread(soa, thread);
+bool ThreadUtil::GetAliveNativeThread(jthread thread,
+ const art::ScopedObjectAccessAlreadyRunnable& soa,
+ /*out*/ art::Thread** thr,
+ /*out*/ jvmtiError* err) {
+ if (!GetNativeThread(thread, soa, thr, err)) {
+ return false;
+ } else if (*thr == nullptr || (*thr)->GetState() == art::ThreadState::kTerminated) {
+ *err = ERR(THREAD_NOT_ALIVE);
+ return false;
+ } else {
+ return true;
+ }
}
jvmtiError ThreadUtil::GetThreadInfo(jvmtiEnv* env, jthread thread, jvmtiThreadInfo* info_ptr) {
@@ -182,9 +203,10 @@
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
+ art::Thread* target;
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetNativeThread(thread, soa, &target, &err)) {
+ return err;
}
JvmtiUniquePtr<char[]> name_uptr;
@@ -297,25 +319,18 @@
};
// Return the thread's (or current thread, if null) thread state.
-static InternalThreadState GetNativeThreadState(jthread thread,
- const art::ScopedObjectAccessAlreadyRunnable& soa)
+static InternalThreadState GetNativeThreadState(art::Thread* target)
REQUIRES_SHARED(art::Locks::mutator_lock_)
REQUIRES(art::Locks::thread_list_lock_, art::Locks::user_code_suspension_lock_) {
- art::Thread* self = nullptr;
- if (thread == nullptr) {
- self = art::Thread::Current();
- } else {
- self = art::Thread::FromManagedThread(soa, thread);
- }
InternalThreadState thread_state = {};
- art::MutexLock tscl_mu(soa.Self(), *art::Locks::thread_suspend_count_lock_);
- thread_state.native_thread = self;
- if (self == nullptr || self->IsStillStarting()) {
+ art::MutexLock tscl_mu(art::Thread::Current(), *art::Locks::thread_suspend_count_lock_);
+ thread_state.native_thread = target;
+ if (target == nullptr || target->IsStillStarting()) {
thread_state.art_state = art::ThreadState::kStarting;
thread_state.thread_user_code_suspend_count = 0;
} else {
- thread_state.art_state = self->GetState();
- thread_state.thread_user_code_suspend_count = self->GetUserCodeSuspendCount();
+ thread_state.art_state = target->GetState();
+ thread_state.thread_user_code_suspend_count = target->GetUserCodeSuspendCount();
}
return thread_state;
}
@@ -456,7 +471,12 @@
}
art::ScopedObjectAccess soa(self);
art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
- state = GetNativeThreadState(thread, soa);
+ jvmtiError err = ERR(INTERNAL);
+ art::Thread* target = nullptr;
+ if (!GetNativeThread(thread, soa, &target, &err)) {
+ return err;
+ }
+ state = GetNativeThreadState(target);
if (state.art_state == art::ThreadState::kStarting) {
break;
}
@@ -578,12 +598,10 @@
art::Thread* self = art::Thread::Current();
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
JvmtiGlobalTLSData* global_tls = reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS());
@@ -607,12 +625,10 @@
art::Thread* self = art::Thread::Current();
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = GetNativeThread(thread, soa);
- if (target == nullptr && thread == nullptr) {
- return ERR(INVALID_THREAD);
- }
- if (target == nullptr) {
- return ERR(THREAD_NOT_ALIVE);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
}
JvmtiGlobalTLSData* global_tls = reinterpret_cast<JvmtiGlobalTLSData*>(target->GetCustomTLS());
@@ -731,9 +747,13 @@
{
art::ScopedObjectAccess soa(self);
art::MutexLock thread_list_mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = GetNativeThread(target_jthread, soa);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetAliveNativeThread(target_jthread, soa, &target, &err)) {
+ return err;
+ }
art::ThreadState state = target->GetState();
- if (state == art::ThreadState::kTerminated || state == art::ThreadState::kStarting) {
+ if (state == art::ThreadState::kStarting || target->IsStillStarting()) {
return ERR(THREAD_NOT_ALIVE);
} else {
art::MutexLock thread_suspend_count_mu(self, *art::Locks::thread_suspend_count_lock_);
@@ -789,9 +809,10 @@
{
art::ScopedObjectAccess soa(self);
art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- art::Thread* target = GetNativeThread(thread, soa);
- if (target == nullptr) {
- return ERR(INVALID_THREAD);
+ art::Thread* target = nullptr;
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
} else if (target == self) {
target_is_self = true;
}
@@ -825,16 +846,14 @@
// have the 'suspend_lock' locked here.
art::ScopedObjectAccess soa(self);
art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
- target = GetNativeThread(thread, soa);
- if (target == nullptr) {
- return ERR(INVALID_THREAD);
+ jvmtiError err = ERR(INTERNAL);
+ if (!GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
} else if (target == self) {
// We would have paused until we aren't suspended anymore due to the ScopedObjectAccess so
// we can just return THREAD_NOT_SUSPENDED. Unfortunately we cannot do any real DCHECKs
// about current state since it's all concurrent.
return ERR(THREAD_NOT_SUSPENDED);
- } else if (target->GetState() == art::ThreadState::kTerminated) {
- return ERR(THREAD_NOT_ALIVE);
}
// The JVMTI spec requires us to return THREAD_NOT_SUSPENDED if it is alive but we really
// cannot tell why resume failed.
@@ -859,6 +878,22 @@
} while (true);
}
+static bool IsCurrentThread(jthread thr) {
+ if (thr == nullptr) {
+ return true;
+ }
+ art::Thread* self = art::Thread::Current();
+ art::ScopedObjectAccess soa(self);
+ art::MutexLock mu(self, *art::Locks::thread_list_lock_);
+ art::Thread* target = nullptr;
+ jvmtiError err_unused = ERR(INTERNAL);
+ if (ThreadUtil::GetNativeThread(thr, soa, &target, &err_unused)) {
+ return target == self;
+ } else {
+ return false;
+ }
+}
+
// Suspends all the threads in the list at the same time. Getting this behavior is a little tricky
// since we can have threads in the list multiple times. This generally doesn't matter unless the
// current thread is present multiple times. In that case we need to suspend only once and either
@@ -878,17 +913,12 @@
// running thread. These indexes we need to handle specially since we need to only actually
// suspend a single time.
std::vector<jint> current_thread_indexes;
- art::Thread* self = art::Thread::Current();
for (jint i = 0; i < request_count; i++) {
- {
- art::ScopedObjectAccess soa(self);
- art::MutexLock mu(self, *art::Locks::thread_list_lock_);
- if (threads[i] == nullptr || GetNativeThread(threads[i], soa) == self) {
- current_thread_indexes.push_back(i);
- continue;
- }
+ if (IsCurrentThread(threads[i])) {
+ current_thread_indexes.push_back(i);
+ } else {
+ results[i] = env->SuspendThread(threads[i]);
}
- results[i] = env->SuspendThread(threads[i]);
}
if (!current_thread_indexes.empty()) {
jint first_current_thread_index = current_thread_indexes[0];
diff --git a/openjdkjvmti/ti_thread.h b/openjdkjvmti/ti_thread.h
index 57b1943..ceebff6 100644
--- a/openjdkjvmti/ti_thread.h
+++ b/openjdkjvmti/ti_thread.h
@@ -93,8 +93,24 @@
const jthread* threads,
jvmtiError* results);
- static art::Thread* GetNativeThread(jthread thread,
- const art::ScopedObjectAccessAlreadyRunnable& soa)
+ // Returns true if we decoded the thread and it is alive, false otherwise with an appropriate
+ // error placed into 'err'. A thread is alive if it has had it's 'start' function called and has
+ // (or at least could have) executed managed code and has not yet returned past it's first managed
+ // frame. This means that the thread returned might have IsStillStarting() return true. Code that
+ // does not consider that alive should check manually.
+ static bool GetAliveNativeThread(jthread thread,
+ const art::ScopedObjectAccessAlreadyRunnable& soa,
+ /*out*/ art::Thread** thr,
+ /*out*/ jvmtiError* err)
+ REQUIRES_SHARED(art::Locks::mutator_lock_)
+ REQUIRES(art::Locks::thread_list_lock_);
+
+ // Returns true if we decoded the thread, false otherwise with an appropriate error placed into
+ // 'err'
+ static bool GetNativeThread(jthread thread,
+ const art::ScopedObjectAccessAlreadyRunnable& soa,
+ /*out*/ art::Thread** thr,
+ /*out*/ jvmtiError* err)
REQUIRES_SHARED(art::Locks::mutator_lock_)
REQUIRES(art::Locks::thread_list_lock_);