[Fuchsia] Enable CPU profiling for the standalone VM

runtime/vm/thread_interrupter_fuchsia.cc

Index: runtime/vm/thread_interrupter_fuchsia.cc
diff --git a/runtime/vm/thread_interrupter_fuchsia.cc b/runtime/vm/thread_interrupter_fuchsia.cc
index 2e9b7b98642f39533c3a6080f51e33327eced470..e449ddd517b6579243a97c4306e9f3f8a94206bd 100644
--- a/runtime/vm/thread_interrupter_fuchsia.cc
+++ b/runtime/vm/thread_interrupter_fuchsia.cc
@@ -11,6 +11,7 @@
 #include <magenta/status.h>
 #include <magenta/syscalls.h>
 #include <magenta/syscalls/debug.h>
+#include <magenta/syscalls/object.h>
 #include <magenta/types.h>
 
 #include "vm/flags.h"
@@ -31,56 +32,91 @@ DECLARE_FLAG(bool, trace_thread_interrupter);
 // When MG-430 is resolved, the code below should be rewritten to use whatever
 // feature is added for it.
 
-// TODO(MG-795): The profiler is currently off by default on Fuchsia because
-// suspending a thread that is in a call to pthread_cond_wait() causes
-// pthread_cond_wait() to return ETIMEDOUT.
+// A scope within which a target thread is suspended. When the scope is exited,
+// the thread is resumed and its handle is closed.
+class ThreadSuspendScope {
+ public:
+  explicit ThreadSuspendScope(mx_handle_t thread_handle)
+      : thread_handle_(thread_handle), suspended_(true) {
+    mx_status_t status = mx_task_suspend(thread_handle);
+    // If a thread is somewhere where suspend is impossible, mx_task_suspend()
+    // can return ERR_NOT_SUPPORTED.
+    if ((status != NO_ERROR) && (status != ERR_NOT_SUPPORTED)) {
+      if (FLAG_trace_thread_interrupter) {
+        OS::PrintErr("ThreadInterrupter: mx_task_suspend failed: %s\n",
+            mx_status_get_string(status));
+      }
+      suspended_ = false;
+    }
+  }
+
+  ~ThreadSuspendScope() {
+    if (suspended_) {
+      mx_status_t status = mx_task_resume(thread_handle_, 0);
+      if (status != NO_ERROR) {
+        // If we fail to resume a thread, then it's likely the program will
+        // hang. Crash instead.
+        FATAL1("mx_task_resume failed: %s", mx_status_get_string(status));
+      }
+    }
+    mx_handle_close(thread_handle_);
+  }
+
+  bool suspended() const { return suspended_; }
+
+ private:
+  mx_handle_t thread_handle_;
+  bool suspended_;
+
+  DISALLOW_ALLOCATION();
+  DISALLOW_COPY_AND_ASSIGN(ThreadSuspendScope);
+};
 
 class ThreadInterrupterFuchsia : public AllStatic {
  public:
   static bool GrabRegisters(mx_handle_t thread, InterruptedThreadState* state) {
-    // TODO(zra): Enable this when mx_thread_read_state() works on suspended
-    // threads.
-    while (false) {
-      char buf[MX_MAX_THREAD_STATE_SIZE];
-      uint32_t regset_size = MX_MAX_THREAD_STATE_SIZE;
-      mx_status_t status = mx_thread_read_state(
-          thread, MX_THREAD_STATE_REGSET0, &buf[0], regset_size, &regset_size);
-      if (status != NO_ERROR) {
+    char buf[MX_MAX_THREAD_STATE_SIZE];

[dje, 2017/06/02 17:09:06]

This won't necessarily have sufficient alignment, will it?

[zra, 2017/06/02 17:46:47]

The docs don't mention alignment requirements for this buffer.

[dje, 2017/06/02 18:40:07]

The requirements are more related to how the contents of the buffer are used.
You're casting a pointer to a plain array of chars to a pointer to something
with higher alignment requirements and then dereferencing the pointer.

There are several solutions. You could just declare the buffer to be
mx_${arch}_general_regs_t (suitably ifdef'd of course). Or you could make the
buffer a union of the char array and a uint64_t (which is sufficient for this
particular case). Or you could use __attribute__((aligned(sizeof(uint64_t))) on
buf. Or make buf an array of uint64_t instead of chars.

[zra, 2017/06/02 20:39:01]

Done.

+    uint32_t regset_size = MX_MAX_THREAD_STATE_SIZE;
+    mx_status_t status = mx_thread_read_state(
+        thread, MX_THREAD_STATE_REGSET0, &buf[0], regset_size, &regset_size);
+    if (status != NO_ERROR) {
+      if (FLAG_trace_thread_interrupter) {
         OS::PrintErr("ThreadInterrupter failed to get registers: %s\n",
-                     mx_status_get_string(status));
-        return false;
+                   mx_status_get_string(status));
       }
+      return false;
+    }
 #if defined(TARGET_ARCH_X64)
-      mx_x86_64_general_regs_t* regs =
-          reinterpret_cast<mx_x86_64_general_regs_t*>(&buf[0]);
-      state->pc = static_cast<uintptr_t>(regs->rip);
-      state->fp = static_cast<uintptr_t>(regs->rbp);
-      state->csp = static_cast<uintptr_t>(regs->rsp);
-      state->dsp = static_cast<uintptr_t>(regs->rsp);
+    mx_x86_64_general_regs_t* regs =
+        reinterpret_cast<mx_x86_64_general_regs_t*>(&buf[0]);
+    state->pc = static_cast<uintptr_t>(regs->rip);
+    state->fp = static_cast<uintptr_t>(regs->rbp);
+    state->csp = static_cast<uintptr_t>(regs->rsp);
+    state->dsp = static_cast<uintptr_t>(regs->rsp);
 #elif defined(TARGET_ARCH_ARM64)
-      mx_arm64_general_regs_t* regs =
-          reinterpret_cast<mx_arm64_general_regs_t*>(&buf[0]);
-      state->pc = static_cast<uintptr_t>(regs->pc);
-      state->fp = static_cast<uintptr_t>(regs->r[FPREG]);
-      state->csp = static_cast<uintptr_t>(regs->sp);
-      state->dsp = static_cast<uintptr_t>(regs->r[SPREG]);
-      state->lr = static_cast<uintptr_t>(regs->lr);
+    mx_arm64_general_regs_t* regs =
+        reinterpret_cast<mx_arm64_general_regs_t*>(&buf[0]);
+    state->pc = static_cast<uintptr_t>(regs->pc);
+    state->fp = static_cast<uintptr_t>(regs->r[FPREG]);
+    state->csp = static_cast<uintptr_t>(regs->sp);
+    state->dsp = static_cast<uintptr_t>(regs->r[SPREG]);
+    state->lr = static_cast<uintptr_t>(regs->lr);
 #else
 #error "Unsupported architecture"
 #endif
-    }
     return true;
   }
 
 
   static void Interrupt(OSThread* os_thread) {
+    ASSERT(os_thread->id() != MX_KOID_INVALID);
     ASSERT(!OSThread::Compare(OSThread::GetCurrentThreadId(), os_thread->id()));
     mx_status_t status;
 
     // Get a handle on the target thread.
-    mx_koid_t target_thread_koid = os_thread->id();
+    const mx_koid_t target_thread_koid = os_thread->id();
     if (FLAG_trace_thread_interrupter) {
-      OS::Print("ThreadInterrupter: interrupting thread with koid=%d\n",
+      OS::PrintErr("ThreadInterrupter: interrupting thread with koid=%d\n",
                 target_thread_koid);
     }
     mx_handle_t target_thread_handle;
@@ -88,56 +124,81 @@ class ThreadInterrupterFuchsia : public AllStatic {
                                  MX_RIGHT_SAME_RIGHTS, &target_thread_handle);
     if (status != NO_ERROR) {
       if (FLAG_trace_thread_interrupter) {
-        OS::Print("ThreadInterrupter failed to get the thread handle: %s\n",
+        OS::PrintErr("ThreadInterrupter: mx_object_get_child failed: %s\n",
                   mx_status_get_string(status));
       }
-      FATAL1("mx_object_get_child failed: %s", mx_status_get_string(status));
+      return;
     }
     if (target_thread_handle == MX_HANDLE_INVALID) {
-      FATAL("ThreadInterrupter got an invalid target thread handle!");
+      if (FLAG_trace_thread_interrupter) {
+        OS::PrintErr("ThreadInterrupter: mx_object_get_child gave an invalid "
+                  "thread handle!");
+      }
+      return;
+    }
+
+    // This scope suspends the thread. When we exit the scope, the thread is
+    // resumed, and the thread handle is closed.
+    ThreadSuspendScope tss(target_thread_handle);
+    if (!tss.suspended()) {
+      return;
     }
 
-    // Pause the target thread.
-    status = mx_task_suspend(target_thread_handle);
+    // Check that the thread is suspended.
+    mx_info_thread_t thread_info;
+    status = mx_object_get_info(target_thread_handle,
+                                MX_INFO_THREAD,
+                                &thread_info,
+                                sizeof(thread_info),
+                                NULL,
+                                NULL);
     if (status != NO_ERROR) {
       if (FLAG_trace_thread_interrupter) {
-        OS::Print("ThreadInterrupter failed to suspend thread %ld: %s\n",
-                  static_cast<intptr_t>(os_thread->id()),
-                  mx_status_get_string(status));
+        OS::PrintErr("ThreadInterrupter: mx_object_get_info failed: %s\n",
+            mx_status_get_string(status));
       }
-      mx_handle_close(target_thread_handle);
-      FATAL1("mx_task_suspend failed: %s", mx_status_get_string(status));
+      return;
     }
-
-    // TODO(zra): Enable this when mx_thread_read_state() works on suspended
-    // threads.
-    while (false) {
-      // Grab the target thread's registers.
-      InterruptedThreadState its;
-      if (!GrabRegisters(target_thread_handle, &its)) {
-        // Failed to get thread registers.
-        status = mx_task_resume(target_thread_handle, 0);
-        if (status != NO_ERROR) {
-          FATAL1("mx_task_resume failed: %s", mx_status_get_string(status));
-        }
-        mx_handle_close(target_thread_handle);
-        return;
-      }
-      // Currently we sample only threads that are associated
-      // with an isolate. It is safe to call 'os_thread->thread()'
-      // here as the thread which is being queried is suspended.
-      Thread* thread = os_thread->thread();
-      if (thread != NULL) {
-        Profiler::SampleThread(thread, its);
+    if (thread_info.state != MX_THREAD_STATE_SUSPENDED) {

[zra, 2017/06/02 16:51:23]

@dje: Frequently, after calling mx_task_suspend() on a thread, that thread's
state will still be MX_THREAD_STATE_RUNNING. Is it expected that that thread
will soon enter the SUSPENDED state? Should I poll to wait for it?

[dje, 2017/06/02 17:09:06]

Yeah, you currently have to poll to wait for the thread to suspend. This is a
good reason to add something better (e.g., waiting for some kind of "signal" (in
quotes to not suggest any particular solution)).

[zra, 2017/06/02 17:46:47]

Changed to poll.

+      if (FLAG_trace_thread_interrupter) {
+        OS::PrintErr("ThreadInterrupter: Thread is not suspended: state = %s\n",
+            ThreadStateGetString(thread_info.state));
       }
+      return;
     }
 
-    // Resume the target thread.
-    status = mx_task_resume(target_thread_handle, 0);
-    if (status != NO_ERROR) {
-      FATAL1("mx_task_resume failed: %s", mx_status_get_string(status));
+    // Grab the target thread's registers.
+    InterruptedThreadState its;
+    if (!GrabRegisters(target_thread_handle, &its)) {
+      return;
+    }
+    // Currently we sample only threads that are associated
+    // with an isolate. It is safe to call 'os_thread->thread()'
+    // here as the thread which is being queried is suspended.
+    Thread* thread = os_thread->thread();
+    if (thread != NULL) {
+      Profiler::SampleThread(thread, its);
+    }
+  }
+
+ private:
+  static const char* ThreadStateGetString(uint32_t state) {
+    switch (state) {
+      case MX_THREAD_STATE_NEW:
+        return "MX_THREAD_STATE_NEW";
+      case MX_THREAD_STATE_RUNNING:
+        return "MX_THREAD_STATE_RUNNING";
+      case MX_THREAD_STATE_SUSPENDED:
+        return "MX_THREAD_STATE_SUSPENDED";
+      case MX_THREAD_STATE_BLOCKED:
+        return "MX_THREAD_STATE_BLOCKED";
+      case MX_THREAD_STATE_DYING:
+        return "MX_THREAD_STATE_DYING";
+      case MX_THREAD_STATE_DEAD:
+        return "MX_THREAD_STATE_DEAD";
+      default:
+        return "<Unknown>";
     }
-    mx_handle_close(target_thread_handle);
   }
 };
 
@@ -149,12 +210,12 @@ bool ThreadInterrupter::IsDebuggerAttached() {
 
 void ThreadInterrupter::InterruptThread(OSThread* thread) {
   if (FLAG_trace_thread_interrupter) {
-    OS::Print("ThreadInterrupter suspending %p\n",
+    OS::PrintErr("ThreadInterrupter suspending %p\n",
               reinterpret_cast<void*>(thread->id()));
   }
   ThreadInterrupterFuchsia::Interrupt(thread);
   if (FLAG_trace_thread_interrupter) {
-    OS::Print("ThreadInterrupter resuming %p\n",
+    OS::PrintErr("ThreadInterrupter resuming %p\n",
               reinterpret_cast<void*>(thread->id()));
   }
 }