[vm, service] Fix parameter handling for profiler timeline RPCs.

Add missing required parameter "tags" to internal invocation in Dart_WriteProfileToTimeline.

Add service tests for accessing the profiler samples as timeline events.

Fix the profiler for threads spawned from the Mac eventhandler. Compare 5d02929fa0.

Bug: https://github.com/dart-lang/sdk/issues/26416
Bug: https://github.com/dart-lang/sdk/issues/36830
Change-Id: I5b3cb8f3530da081057090879c6ab820deba1510
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100988
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/observatory/tests/service/get_cpu_profile_timeline_rpc_test.dart

@@ -15,40 +15,60 @@ fib(n) {
 
 testeeDo() {
   print("Testee doing something.");
-  fib(25);
+  fib(30);
   print("Testee did something.");
 }
 
+Future checkTimeline(Isolate isolate, Map params) async {
+  print(params);
+  var result =
+      await isolate.invokeRpcNoUpgrade('_getCpuProfileTimeline', params);
+  print(result);
+  expect(result['type'], equals('_CpuProfileTimeline'));
+
+  var isString = new isInstanceOf<String>();
+  var isInt = new isInstanceOf<int>();
+
+  Map frames = result['stackFrames'];
+  expect(frames.length, greaterThan(10), reason: "Should have many samples");
+  for (Map frame in frames.values) {
+    expect(frame['category'], isString);
+    expect(frame['name'], isString);
+    if (frame['parent'] != null) {
+      expect(frames.containsKey(frame['parent']), isTrue);
+    }
+  }
+
+  List events = result['traceEvents'];
+  expect(events.length, greaterThan(10), reason: "Should have many samples");
+  for (Map event in events) {
+    expect(event['ph'], equals('P'));
+    expect(event['pid'], isInt);
+    expect(event['tid'], isInt);
+    expect(event['ts'], isInt);
+    expect(event['cat'], equals("Dart"));
+    expect(frames.containsKey(event['sf']), isTrue);
+  }
+}
+
 var tests = <IsolateTest>[
-  (Isolate isolate) async {
-    var params = {'tags': 'VMUser'};
-    var result =
-        await isolate.invokeRpcNoUpgrade('_getCpuProfileTimeline', params);
-    print(result);
-    expect(result['type'], equals('_CpuProfileTimeline'));
-
-    var isString = new isInstanceOf<String>();
-    var isInt = new isInstanceOf<int>();
-
-    Map frames = result['stackFrames'];
-    for (Map frame in frames.values) {
-      expect(frame['category'], isString);
-      expect(frame['name'], isString);
-      if (frame['parent'] != null) {
-        expect(frames.containsKey(frame['parent']), isTrue);
-      }
-    }
-
-    List events = result['traceEvents'];
-    for (Map event in events) {
-      expect(event['ph'], equals('P'));
-      expect(event['pid'], isInt);
-      expect(event['tid'], isInt);
-      expect(event['ts'], isInt);
-      expect(event['cat'], equals("Dart"));
-      expect(frames.containsKey(event['sf']), isTrue);
-    }
-  },
+  (Isolate i) => checkTimeline(i, {'tags': 'VMUser'}),
+  (Isolate i) => checkTimeline(i, {'tags': 'VMUser', 'code': true}),
+  (Isolate i) => checkTimeline(i, {'tags': 'VMUser', 'code': false}),
+  (Isolate i) => checkTimeline(i, {'tags': 'VMOnly'}),
+  (Isolate i) => checkTimeline(i, {'tags': 'VMOnly', 'code': true}),
+  (Isolate i) => checkTimeline(i, {'tags': 'VMOnly', 'code': false}),
+  (Isolate i) => checkTimeline(i, {'tags': 'None'}),
+  (Isolate i) => checkTimeline(i, {'tags': 'None', 'code': true}),
+  (Isolate i) => checkTimeline(i, {'tags': 'None', 'code': false}),
 ];
 
-main(args) async => runIsolateTests(args, tests, testeeBefore: testeeDo);
+var vmArgs = [
+  '--profiler=true',
+  '--profile-vm=false', // So this also works with DBC and KBC.
+  '--timeline_recorder=ring',
+  '--timeline_streams=Profiler'
+];
+
+main(args) async =>
+    runIsolateTests(args, tests, testeeBefore: testeeDo, extraArgs: vmArgs);

runtime/observatory/tests/service/write_cpu_profile_timeline_rpc_test.dart

@@ -0,0 +1,69 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:observatory/service_io.dart';
+import 'package:unittest/unittest.dart';
+
+import 'test_helper.dart';
+
+fib(n) {
+  if (n < 0) return 0;
+  if (n == 0) return 1;
+  return fib(n - 1) + fib(n - 2);
+}
+
+testeeDo() {
+  print("Testee doing something.");
+  fib(30);
+  print("Testee did something.");
+}
+
+var tests = <IsolateTest>[
+  (Isolate isolate) async {
+    var params = {'tags': 'None'};
+    var result =
+        await isolate.invokeRpcNoUpgrade('_writeCpuProfileTimeline', params);
+    print(result);
+    expect(result['type'], equals('Success'));
+
+    result = await isolate.vm.invokeRpcNoUpgrade('_getVMTimeline', {});
+    expect(result['type'], equals('_Timeline'));
+    expect(result['traceEvents'], new isInstanceOf<List>());
+
+    var events = result['traceEvents'];
+    print(events);
+    var profilerSampleEvents = result['traceEvents'].where((event) {
+      return event['name'] == "Dart CPU sample";
+    }).toList();
+
+    var isString = new isInstanceOf<String>();
+    var isInt = new isInstanceOf<int>();
+
+    expect(profilerSampleEvents.length, greaterThan(10),
+        reason: "Should have many samples");
+    for (Map event in profilerSampleEvents) {
+      print(event);
+
+      // Sadly this is an "Instant" event because there is no way to add a
+      // proper "Sample" event in Fuchsia's tracing.
+      expect(event['ph'], equals('i'));
+
+      expect(event['pid'], isInt);
+      expect(event['tid'], isInt);
+      expect(event['ts'], isInt);
+      expect(event['cat'], equals("Profiler"));
+      expect(event['args']['backtrace'], isString);
+    }
+  },
+];
+
+var vmArgs = [
+  '--profiler=true',
+  '--profile-vm=false', // So this also works with DBC and KBC.
+  '--timeline_recorder=ring',
+  '--timeline_streams=Profiler'
+];
+
+main(args) async =>
+    runIsolateTests(args, tests, testeeBefore: testeeDo, extraArgs: vmArgs);

runtime/vm/dart_api_impl.cc

@@ -1305,12 +1305,18 @@ DART_EXPORT bool Dart_WriteProfileToTimeline(Dart_Port main_port,
 
   const intptr_t kBufferLength = 512;
   char method[kBufferLength];
+
+  // clang-format off
   intptr_t method_length = snprintf(method, kBufferLength, "{"
       "\"jsonrpc\": \"2.0\","
       "\"method\": \"_writeCpuProfileTimeline\","
       "\"id\": \"\","
-      "\"params\": {\"isolateId\": \"isolates/%" Pd64 "\"}"
-    "}", main_port);
+      "\"params\": {"
+      "  \"isolateId\": \"isolates/%" Pd64 "\","
+      "  \"tags\": \"None\""
+      "}"
+  "}", main_port);
+  // clang-format on
   ASSERT(method_length <= kBufferLength);
 
   char* response = NULL;

runtime/vm/os_thread_macos.cc

@@ -14,6 +14,7 @@
 #include <mach/task_info.h>    // NOLINT
 #include <mach/thread_act.h>   // NOLINT
 #include <mach/thread_info.h>  // NOLINT
+#include <signal.h>            // NOLINT
 #include <sys/errno.h>         // NOLINT
 #include <sys/sysctl.h>        // NOLINT
 #include <sys/types.h>         // NOLINT
@@ -21,6 +22,7 @@
 #include "platform/address_sanitizer.h"
 #include "platform/assert.h"
 #include "platform/safe_stack.h"
+#include "platform/signal_blocker.h"
 #include "platform/utils.h"
 
 namespace dart {
@@ -86,6 +88,20 @@ class ThreadStartData {
   DISALLOW_COPY_AND_ASSIGN(ThreadStartData);
 };
 
+// Spawned threads inherit their spawner's signal mask. We sometimes spawn
+// threads for running Dart code from a thread that is blocking SIGPROF.
+// This function explicitly unblocks SIGPROF so the profiler continues to
+// sample this thread.
+static void UnblockSIGPROF() {
+  sigset_t set;
+  sigemptyset(&set);
+  sigaddset(&set, SIGPROF);
+  int r = pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+  USE(r);
+  ASSERT(r == 0);
+  ASSERT(!CHECK_IS_BLOCKING(SIGPROF));
+}
+
 // Dispatch to the thread start function provided by the caller. This trampoline
 // is used to ensure that the thread is properly destroyed if the thread just
 // exits.
@@ -105,7 +121,7 @@ static void* ThreadStart(void* data_ptr) {
   if (thread != NULL) {
     OSThread::SetCurrent(thread);
     thread->set_name(name);
-
+    UnblockSIGPROF();
     // Call the supplied thread start function handing it its parameters.
     function(parameter);
   }

runtime/vm/service.cc

@@ -263,6 +263,14 @@ static bool CheckCompilerDisabled(Thread* thread, JSONStream* js) {
   return false;
 }
 
+static bool CheckProfilerDisabled(Thread* thread, JSONStream* js) {
+  if (Profiler::sample_buffer() == NULL) {
+    js->PrintError(kFeatureDisabled, "Profiler is disabled.");
+    return true;
+  }
+  return false;
+}
+
 static bool GetIntegerId(const char* s, intptr_t* id, int base = 10) {
   if ((s == NULL) || (*s == '\0')) {
     // Empty string.
@@ -3815,13 +3823,12 @@ static const MethodParameter* get_cpu_profile_params[] = {
     NULL,
 };
 
-static const MethodParameter* write_cpu_profile_timeline_params[] = {
-    RUNNABLE_ISOLATE_PARAMETER,
-    NULL,
-};
-
 // TODO(johnmccutchan): Rename this to GetCpuSamples.
 static bool GetCpuProfile(Thread* thread, JSONStream* js) {
+  if (CheckProfilerDisabled(thread, js)) {
+    return true;
+  }
+
   Profile::TagOrder tag_order =
       EnumMapper(js->LookupParam("tags"), tags_enum_names, tags_enum_values);
   intptr_t extra_tags = 0;
@@ -3846,6 +3853,10 @@ static const MethodParameter* get_cpu_profile_timeline_params[] = {
 };
 
 static bool GetCpuProfileTimeline(Thread* thread, JSONStream* js) {
+  if (CheckProfilerDisabled(thread, js)) {
+    return true;
+  }
+
   Profile::TagOrder tag_order =
       EnumMapper(js->LookupParam("tags"), tags_enum_names, tags_enum_values);
   int64_t time_origin_micros =
@@ -3858,7 +3869,19 @@ static bool GetCpuProfileTimeline(Thread* thread, JSONStream* js) {
   return true;
 }
 
+static const MethodParameter* write_cpu_profile_timeline_params[] = {
+    RUNNABLE_ISOLATE_PARAMETER,
+    new EnumParameter("tags", true, tags_enum_names),
+    new Int64Parameter("timeOriginMicros", false),
+    new Int64Parameter("timeExtentMicros", false),
+    NULL,
+};
+
 static bool WriteCpuProfileTimeline(Thread* thread, JSONStream* js) {
+  if (CheckProfilerDisabled(thread, js)) {
+    return true;
+  }
+
   Profile::TagOrder tag_order =
       EnumMapper(js->LookupParam("tags"), tags_enum_names, tags_enum_values);
   int64_t time_origin_micros =
@@ -3868,6 +3891,7 @@ static bool WriteCpuProfileTimeline(Thread* thread, JSONStream* js) {
   bool code_trie = BoolParameter::Parse(js->LookupParam("code"), true);
   ProfilerService::AddToTimeline(tag_order, time_origin_micros,
                                  time_extent_micros, code_trie);
+  PrintSuccess(js);  // The "result" is a side-effect in the timeline.
   return true;
 }