Fix GetThreadState with threads in kNative.

We were always returning JVMTI_THREAD_STATE_WAITING for threads in the
kNative state. To prevent any similar problems from happening in the
future we changed it so all thread-states are explicitly enumerated
and handled in a switch statement.

Test: ./test.py --host -j50
Bug: 67784165
Change-Id: I6646b36aa36cb4671bf95777aefc5c88b659e90f

4 files changed, 151 insertions, 31 deletions

diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index 9a809df011..d0913cf859 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -340,47 +340,92 @@ static jint GetJvmtiThreadStateFromInternal(const InternalThreadState& state) {
   jint jvmti_state = JVMTI_THREAD_STATE_ALIVE;
 
   if (state.thread_user_code_suspend_count != 0) {
+    // Suspended can be set with any thread state so check it here. Even if the thread isn't in
+    // kSuspended state it will move to that once it hits a checkpoint so we can still set this.
     jvmti_state |= JVMTI_THREAD_STATE_SUSPENDED;
     // Note: We do not have data about the previous state. Otherwise we should load the previous
     //       state here.
   }
 
   if (state.native_thread->IsInterrupted()) {
+    // Interrupted can be set with any thread state so check it here.
     jvmti_state |= JVMTI_THREAD_STATE_INTERRUPTED;
   }
 
-  if (internal_thread_state == art::ThreadState::kNative) {
-    jvmti_state |= JVMTI_THREAD_STATE_IN_NATIVE;
-  }
-
-  if (internal_thread_state == art::ThreadState::kRunnable ||
-      internal_thread_state == art::ThreadState::kWaitingWeakGcRootRead ||
-      internal_thread_state == art::ThreadState::kSuspended) {
-    jvmti_state |= JVMTI_THREAD_STATE_RUNNABLE;
-  } else if (internal_thread_state == art::ThreadState::kBlocked) {
-    jvmti_state |= JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER;
-  } else {
-    // Should be in waiting state.
-    jvmti_state |= JVMTI_THREAD_STATE_WAITING;
-
-    if (internal_thread_state == art::ThreadState::kTimedWaiting ||
-        internal_thread_state == art::ThreadState::kSleeping) {
-      jvmti_state |= JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT;
-    } else {
-      jvmti_state |= JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
-    }
-
-    if (internal_thread_state == art::ThreadState::kSleeping) {
-      jvmti_state |= JVMTI_THREAD_STATE_SLEEPING;
-    }
-
-    if (internal_thread_state == art::ThreadState::kTimedWaiting ||
-        internal_thread_state == art::ThreadState::kWaiting) {
-      jvmti_state |= JVMTI_THREAD_STATE_IN_OBJECT_WAIT;
-    }
-
-    // TODO: PARKED. We'll have to inspect the stack.
+  // Enumerate all the thread states and fill in the other bits. This contains the results of
+  // following the decision tree in the JVMTI spec GetThreadState documentation.
+  switch (internal_thread_state) {
+    case art::ThreadState::kRunnable:
+    case art::ThreadState::kWaitingWeakGcRootRead:
+    case art::ThreadState::kSuspended:
+      // These are all simply runnable.
+      // kRunnable is self-explanatory.
+      // kWaitingWeakGcRootRead is set during some operations with strings due to the intern-table
+      // so we want to keep it marked as runnable.
+      // kSuspended we don't mark since if we don't have a user_code_suspend_count then it is done
+      // by the GC and not a JVMTI suspension, which means it cannot be removed by ResumeThread.
+      jvmti_state |= JVMTI_THREAD_STATE_RUNNABLE;
+      break;
+    case art::ThreadState::kNative:
+      // kNative means native and runnable. Technically THREAD_STATE_IN_NATIVE can be set with any
+      // state but we don't have the information to know if it should be present for any but the
+      // kNative state.
+      jvmti_state |= (JVMTI_THREAD_STATE_IN_NATIVE |
+                      JVMTI_THREAD_STATE_RUNNABLE);
+      break;
+    case art::ThreadState::kBlocked:
+      // Blocked is one of the top level states so it sits alone.
+      jvmti_state |= JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER;
+      break;
+    case art::ThreadState::kWaiting:
+      // Object.wait() so waiting, indefinitely, in object.wait.
+      jvmti_state |= (JVMTI_THREAD_STATE_WAITING |
+                      JVMTI_THREAD_STATE_WAITING_INDEFINITELY |
+                      JVMTI_THREAD_STATE_IN_OBJECT_WAIT);
+      break;
+    case art::ThreadState::kTimedWaiting:
+      // Object.wait(long) so waiting, with timeout, in object.wait.
+      jvmti_state |= (JVMTI_THREAD_STATE_WAITING |
+                      JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT |
+                      JVMTI_THREAD_STATE_IN_OBJECT_WAIT);
+      break;
+    case art::ThreadState::kSleeping:
+      // In object.sleep. This is a timed wait caused by sleep.
+      jvmti_state |= (JVMTI_THREAD_STATE_WAITING |
+                      JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT |
+                      JVMTI_THREAD_STATE_SLEEPING);
+      break;
+    // TODO We might want to print warnings if we have the debugger running while JVMTI agents are
+    // attached.
+    case art::ThreadState::kWaitingForDebuggerSend:
+    case art::ThreadState::kWaitingForDebuggerToAttach:
+    case art::ThreadState::kWaitingInMainDebuggerLoop:
+    case art::ThreadState::kWaitingForDebuggerSuspension:
+    case art::ThreadState::kWaitingForLockInflation:
+    case art::ThreadState::kWaitingForTaskProcessor:
+    case art::ThreadState::kWaitingForGcToComplete:
+    case art::ThreadState::kWaitingForCheckPointsToRun:
+    case art::ThreadState::kWaitingPerformingGc:
+    case art::ThreadState::kWaitingForJniOnLoad:
+    case art::ThreadState::kWaitingInMainSignalCatcherLoop:
+    case art::ThreadState::kWaitingForSignalCatcherOutput:
+    case art::ThreadState::kWaitingForDeoptimization:
+    case art::ThreadState::kWaitingForMethodTracingStart:
+    case art::ThreadState::kWaitingForVisitObjects:
+    case art::ThreadState::kWaitingForGetObjectsAllocated:
+    case art::ThreadState::kWaitingForGcThreadFlip:
+      // All of these are causing the thread to wait for an indeterminate amount of time but isn't
+      // caused by sleep, park, or object#wait.
+      jvmti_state |= (JVMTI_THREAD_STATE_WAITING |
+                      JVMTI_THREAD_STATE_WAITING_INDEFINITELY);
+      break;
+    case art::ThreadState::kStarting:
+    case art::ThreadState::kTerminated:
+      // We only call this if we are alive so we shouldn't see either of these states.
+      LOG(FATAL) << "Should not be in state " << internal_thread_state;
+      UNREACHABLE();
   }
+  // TODO: PARKED. We'll have to inspect the stack.
 
   return jvmti_state;
 }
diff --git a/test/924-threads/expected.txt b/test/924-threads/expected.txt
index e52955992b..accc78208a 100644
--- a/test/924-threads/expected.txt
+++ b/test/924-threads/expected.txt
@@ -33,6 +33,7 @@ Thread type is class java.lang.Thread
 401 = ALIVE|BLOCKED_ON_MONITOR_ENTER
 e1 = ALIVE|WAITING_WITH_TIMEOUT|SLEEPING|WAITING
 5 = ALIVE|RUNNABLE
+400005 = ALIVE|RUNNABLE|IN_NATIVE
 2 = TERMINATED
 Thread type is class art.Test924$ExtThread
 0 = NEW
@@ -41,6 +42,7 @@ Thread type is class art.Test924$ExtThread
 401 = ALIVE|BLOCKED_ON_MONITOR_ENTER
 e1 = ALIVE|WAITING_WITH_TIMEOUT|SLEEPING|WAITING
 5 = ALIVE|RUNNABLE
+400005 = ALIVE|RUNNABLE|IN_NATIVE
 2 = TERMINATED
 [Thread[FinalizerDaemon,5,system], Thread[FinalizerWatchdogDaemon,5,system], Thread[HeapTaskDaemon,5,system], Thread[ReferenceQueueDaemon,5,system], Thread[TestThread,5,main], Thread[main,5,main]]
 JVMTI_ERROR_THREAD_NOT_ALIVE
diff --git a/test/924-threads/src/art/Test924.java b/test/924-threads/src/art/Test924.java
index 1ff2c3f644..e8e97817b7 100644
--- a/test/924-threads/src/art/Test924.java
+++ b/test/924-threads/src/art/Test924.java
@@ -109,6 +109,7 @@ public class Test924 {
     final CountDownLatch cdl4 = new CountDownLatch(1);
     final CountDownLatch cdl5 = new CountDownLatch(1);
     final Holder h = new Holder();
+    final NativeWaiter w = new NativeWaiter();
     Runnable r = new Runnable() {
       @Override
       public void run() {
@@ -136,6 +137,8 @@ public class Test924 {
           while (!h.flag) {
             // Busy-loop.
           }
+
+          nativeLoop(w.struct);
         } catch (Exception e) {
           throw new RuntimeException(e);
         }
@@ -193,6 +196,11 @@ public class Test924 {
     printThreadState(t);
     h.flag = true;
 
+    // Native
+    w.waitForNative();
+    printThreadState(t);
+    w.finish();
+
     // Dying.
     t.join();
     Thread.yield();
@@ -427,6 +435,31 @@ public class Test924 {
     System.out.println(threadInfo[4] == null ? "null" : threadInfo[4].getClass());  // Context CL.
   }
 
+  public static final class NativeWaiter {
+    public long struct;
+    public NativeWaiter() {
+      struct = nativeWaiterStructAlloc();
+    }
+    public void waitForNative() {
+      if (struct == 0l) {
+        throw new Error("Already resumed from native!");
+      }
+      nativeWaiterStructWaitForNative(struct);
+    }
+    public void finish() {
+      if (struct == 0l) {
+        throw new Error("Already resumed from native!");
+      }
+      nativeWaiterStructFinish(struct);
+      struct = 0;
+    }
+  }
+
+  private static native long nativeWaiterStructAlloc();
+  private static native void nativeWaiterStructWaitForNative(long struct);
+  private static native void nativeWaiterStructFinish(long struct);
+  private static native void nativeLoop(long w);
+
   private static native Thread getCurrentThread();
   private static native Object[] getThreadInfo(Thread t);
   private static native int getThreadState(Thread t);
diff --git a/test/924-threads/threads.cc b/test/924-threads/threads.cc
index e21dcc240e..8caff768c1 100644
--- a/test/924-threads/threads.cc
+++ b/test/924-threads/threads.cc
@@ -35,6 +35,46 @@
 namespace art {
 namespace Test924Threads {
 
+struct WaiterStruct {
+  std::atomic<bool> started;
+  std::atomic<bool> finish;
+};
+
+extern "C" JNIEXPORT jlong JNICALL Java_art_Test924_nativeWaiterStructAlloc(
+    JNIEnv* env, jclass TestClass ATTRIBUTE_UNUSED) {
+  WaiterStruct* s = nullptr;
+  if (JvmtiErrorToException(env,
+                            jvmti_env,
+                            jvmti_env->Allocate(sizeof(WaiterStruct),
+                                                reinterpret_cast<unsigned char**>(&s)))) {
+    return 0;
+  }
+  s->started = false;
+  s->finish = false;
+  return static_cast<jlong>(reinterpret_cast<intptr_t>(s));
+}
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test924_nativeWaiterStructWaitForNative(
+    JNIEnv* env ATTRIBUTE_UNUSED, jclass TestClass ATTRIBUTE_UNUSED, jlong waiter_struct) {
+  WaiterStruct* s = reinterpret_cast<WaiterStruct*>(static_cast<intptr_t>(waiter_struct));
+  while (!s->started) { }
+}
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test924_nativeWaiterStructFinish(
+    JNIEnv* env ATTRIBUTE_UNUSED, jclass TestClass ATTRIBUTE_UNUSED, jlong waiter_struct) {
+  WaiterStruct* s = reinterpret_cast<WaiterStruct*>(static_cast<intptr_t>(waiter_struct));
+  s->finish = true;
+}
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test924_nativeLoop(JNIEnv* env,
+                                                              jclass TestClass ATTRIBUTE_UNUSED,
+                                                              jlong waiter_struct) {
+  WaiterStruct* s = reinterpret_cast<WaiterStruct*>(static_cast<intptr_t>(waiter_struct));
+  s->started = true;
+  while (!s->finish) { }
+  JvmtiErrorToException(env, jvmti_env, jvmti_env->Deallocate(reinterpret_cast<unsigned char*>(s)));
+}
+
 // private static native Thread getCurrentThread();
 // private static native Object[] getThreadInfo(Thread t);