[vm] Get more helpful errors from Dart_SetVMFlags and Dart_MakeIsolateRunnable.

Bug: https://github.com/flutter/flutter/issues/12939
Change-Id: I76ddf2a6d3bb3775637d2eef87c7875c650de0a4
Reviewed-on: https://dart-review.googlesource.com/29680
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/bin/gen_snapshot.cc

@@ -1530,7 +1530,12 @@ int main(int argc, char** argv) {
 #endif
   }
 
-  Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+  char* error = Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+  if (error != NULL) {
+    Log::PrintErr("Setting VM flags failed: %s\n", error);
+    free(error);
+    return kErrorExitCode;
+  }
 
   // Initialize the Dart VM.
   // Note: We don't expect isolates to be created from dart code during
@@ -1580,7 +1585,7 @@ int main(int argc, char** argv) {
     }
   }
 
-  char* error = Dart_Initialize(&init_params);
+  error = Dart_Initialize(&init_params);
   if (error != NULL) {
     Log::PrintErr("VM initialization failed: %s\n", error);
     free(error);

runtime/bin/main.cc

@@ -324,9 +324,8 @@ static Dart_Isolate IsolateSetupHelper(Dart_Isolate isolate,
   // Make the isolate runnable so that it is ready to handle messages.
   Dart_ExitScope();
   Dart_ExitIsolate();
-  bool retval = Dart_IsolateMakeRunnable(isolate);
-  if (!retval) {
-    *error = strdup("Invalid isolate state - Unable to make it runnable");
+  *error = Dart_IsolateMakeRunnable(isolate);
+  if (*error != NULL) {
     Dart_EnterIsolate(isolate);
     Dart_ShutdownIsolate();
     return NULL;
@@ -1059,7 +1058,12 @@ void main(int argc, char** argv) {
     } else if (print_flags_seen) {
       // Will set the VM flags, print them out and then we exit as no
       // script was specified on the command line.
-      Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+      char* error = Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+      if (error != NULL) {
+        Log::PrintErr("Setting VM flags failed: %s\n", error);
+        free(error);
+        Platform::Exit(kErrorExitCode);
+      }
       Platform::Exit(0);
     } else {
       Options::PrintUsage();
@@ -1129,7 +1133,12 @@ void main(int argc, char** argv) {
     Process::SetExitHook(SnapshotOnExitHook);
   }
 
-  Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+  char* error = Dart_SetVMFlags(vm_options.count(), vm_options.arguments());
+  if (error != NULL) {
+    Log::PrintErr("Setting VM flags failed: %s\n", error);
+    free(error);
+    Platform::Exit(kErrorExitCode);
+  }
 
 // Note: must read platform only *after* VM flags are parsed because
 // they might affect how the platform is loaded.
@@ -1171,7 +1180,7 @@ void main(int argc, char** argv) {
   init_params.start_kernel_isolate = false;
 #endif
 
-  char* error = Dart_Initialize(&init_params);
+  error = Dart_Initialize(&init_params);
   if (error != NULL) {
     EventHandler::Stop();
     Log::PrintErr("VM initialization failed: %s\n", error);

runtime/bin/main_options.cc

@@ -218,7 +218,8 @@ void Options::PrintUsage() {
 "The following options are only used for VM development and may\n"
 "be changed in any future version:\n");
     const char* print_flags = "--print_flags";
-    Dart_SetVMFlags(1, &print_flags);
+    char* error = Dart_SetVMFlags(1, &print_flags);
+    ASSERT(error == NULL);
   }
 }
 // clang-format on

runtime/bin/run_vm_tests.cc

@@ -189,9 +189,8 @@ static Dart_Isolate CreateIsolateAndSetup(const char* script_uri,
 
   Dart_ExitScope();
   Dart_ExitIsolate();
-  bool retval = Dart_IsolateMakeRunnable(isolate);
-  if (!retval) {
-    *error = strdup("Invalid isolate state - Unable to make it runnable");
+  *error = Dart_IsolateMakeRunnable(isolate);
+  if (*error != NULL) {
     Dart_EnterIsolate(isolate);
     Dart_ShutdownIsolate();
     return NULL;
@@ -269,10 +268,10 @@ static int Main(int argc, const char** argv) {
   bin::TimerUtils::InitOnce();
   bin::EventHandler::Start();
 
-  bool set_vm_flags_success =
-      Flags::ProcessCommandLineFlags(dart_argc, dart_argv);
-  ASSERT(set_vm_flags_success);
-  const char* err_msg = Dart::InitOnce(
+  const char* error = Flags::ProcessCommandLineFlags(dart_argc, dart_argv);
+  ASSERT(error == NULL);
+
+  error = Dart::InitOnce(
       dart::bin::vm_snapshot_data, dart::bin::vm_snapshot_instructions,
       CreateIsolateAndSetup /* create */, NULL /* shutdown */,
       CleanupIsolate /* cleanup */, NULL /* thread_exit */,
@@ -280,15 +279,15 @@ static int Main(int argc, const char** argv) {
       dart::bin::DartUtils::WriteFile, dart::bin::DartUtils::CloseFile,
       NULL /* entropy_source */, NULL /* get_service_assets */,
       start_kernel_isolate);
+  ASSERT(error == NULL);
 
-  ASSERT(err_msg == NULL);
   // Apply the filter to all registered tests.
   TestCaseBase::RunAll();
   // Apply the filter to all registered benchmarks.
   Benchmark::RunAll(argv[0]);
 
-  err_msg = Dart::Cleanup();
-  ASSERT(err_msg == NULL);
+  error = Dart::Cleanup();
+  ASSERT(error == NULL);
 
   bin::EventHandler::Stop();
 

runtime/bin/vmservice_impl.cc

@@ -211,11 +211,10 @@ bool VmService::Setup(const char* server_ip,
   // Make runnable.
   Dart_ExitScope();
   Dart_ExitIsolate();
-  bool retval = Dart_IsolateMakeRunnable(isolate);
-  if (!retval) {
+  error_msg_ = Dart_IsolateMakeRunnable(isolate);
+  if (error_msg_ != NULL) {
     Dart_EnterIsolate(isolate);
     Dart_ShutdownIsolate();
-    error_msg_ = "Invalid isolate state - Unable to make it runnable.";
     return false;
   }
   Dart_EnterIsolate(isolate);

runtime/include/dart_api.h

@@ -64,6 +64,14 @@ typedef unsigned __int64 uint64_t;
 #endif
 #endif
 
+#if __GNUC__
+#define DART_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
+#elif _MSC_VER
+#define DART_WARN_UNUSED_RESULT _Check_return_
+#else
+#define DART_WARN_UNUSED_RESULT
+#endif
+
 #include <assert.h>
 
 /*
@@ -783,7 +791,8 @@ typedef struct {
  * \return NULL if initialization is successful. Returns an error message
  *   otherwise. The caller is responsible for freeing the error message.
  */
-DART_EXPORT char* Dart_Initialize(Dart_InitializeParams* params);
+DART_WARN_UNUSED_RESULT DART_EXPORT char* Dart_Initialize(
+    Dart_InitializeParams* params);
 
 /**
  * Cleanup state in the VM before process termination.
@@ -791,7 +800,7 @@ DART_EXPORT char* Dart_Initialize(Dart_InitializeParams* params);
  * \return NULL if cleanup is successful. Returns an error message otherwise.
  *   The caller is responsible for freeing the error message.
  */
-DART_EXPORT char* Dart_Cleanup();
+DART_WARN_UNUSED_RESULT DART_EXPORT char* Dart_Cleanup();
 
 /**
  * Sets command line flags. Should be called before Dart_Initialize.
@@ -799,9 +808,11 @@ DART_EXPORT char* Dart_Cleanup();
  * \param argc The length of the arguments array.
  * \param argv An array of arguments.
  *
- * \return True if VM flags set successfully.
+ * \return NULL if successful. Returns an error message otherwise.
+ *  The caller is responsible for freeing the error message.
  */
-DART_EXPORT bool Dart_SetVMFlags(int argc, const char** argv);
+DART_WARN_UNUSED_RESULT DART_EXPORT char* Dart_SetVMFlags(int argc,
+                                                          const char** argv);
 
 /**
  * Returns true if the named VM flag is set.
@@ -1063,14 +1074,18 @@ DART_EXPORT bool Dart_IsDart2Snapshot(const uint8_t* snapshot_buffer);
 /**
  * Make isolate runnable.
  *
- * When isolates are spawned this function is used to indicate that
+ * When isolates are spawned, this function is used to indicate that
  * the creation and initialization (including script loading) of the
  * isolate is complete and the isolate can start.
- * This function does not expect there to be a current isolate.
+ * This function expects there to be no current isolate.
  *
  * \param isolate The isolate to be made runnable.
+ *
+ * \return NULL if successful. Returns an error message otherwise. The caller
+ * is responsible for freeing the error message.
  */
-DART_EXPORT bool Dart_IsolateMakeRunnable(Dart_Isolate isolate);
+DART_WARN_UNUSED_RESULT DART_EXPORT char* Dart_IsolateMakeRunnable(
+    Dart_Isolate isolate);
 
 /*
  * ==================

runtime/vm/dart_api_impl.cc

@@ -1051,7 +1051,7 @@ DART_EXPORT char* Dart_Cleanup() {
   return NULL;
 }
 
-DART_EXPORT bool Dart_SetVMFlags(int argc, const char** argv) {
+DART_EXPORT char* Dart_SetVMFlags(int argc, const char** argv) {
   return Flags::ProcessCommandLineFlags(argc, argv);
 }
 
@@ -1605,7 +1605,7 @@ DART_EXPORT bool Dart_IsDart2Snapshot(const uint8_t* snapshot_buffer) {
   return false;
 }
 
-DART_EXPORT bool Dart_IsolateMakeRunnable(Dart_Isolate isolate) {
+DART_EXPORT char* Dart_IsolateMakeRunnable(Dart_Isolate isolate) {
   CHECK_NO_ISOLATE(Isolate::Current());
   API_TIMELINE_DURATION(Thread::Current());
   if (isolate == NULL) {
@@ -1613,11 +1613,17 @@ DART_EXPORT bool Dart_IsolateMakeRunnable(Dart_Isolate isolate) {
   }
   // TODO(16615): Validate isolate parameter.
   Isolate* iso = reinterpret_cast<Isolate*>(isolate);
+  const char* error;
   if (iso->object_store()->root_library() == Library::null()) {
     // The embedder should have called Dart_LoadScript by now.
-    return false;
+    error = "Missing root library";
+  } else {
+    error = iso->MakeRunnable();
   }
-  return iso->MakeRunnable();
+  if (error != NULL) {
+    return strdup(error);
+  }
+  return NULL;
 }
 
 // --- Messages and Ports ---

runtime/vm/dart_api_impl_test.cc

@@ -5672,11 +5672,12 @@ TEST_CASE(DartAPI_LoadScript) {
   }
 
   // Load a script successfully.
-  result = TestCase::LoadTestScript(kScriptChars, NULL);
+  Dart_Handle root_lib = TestCase::LoadTestScript(kScriptChars, NULL);
+  EXPECT_VALID(root_lib);
+  result = Dart_FinalizeLoading(false);
   EXPECT_VALID(result);
-  Dart_FinalizeLoading(false);
 
-  result = Dart_Invoke(result, NewString("main"), 0, NULL);
+  result = Dart_Invoke(root_lib, NewString("main"), 0, NULL);
   EXPECT_VALID(result);
   EXPECT(Dart_IsInteger(result));
   int64_t value = 0;
@@ -7203,8 +7204,8 @@ static Dart_Isolate RunLoopTestCallback(const char* script_name,
   EXPECT_VALID(result);
   Dart_ExitScope();
   Dart_ExitIsolate();
-  bool retval = Dart_IsolateMakeRunnable(isolate);
-  EXPECT(retval);
+  char* err_msg = Dart_IsolateMakeRunnable(isolate);
+  EXPECT(err_msg == NULL);
   return isolate;
 }
 
@@ -8214,7 +8215,8 @@ TEST_CASE(DartAPI_LazyLoadDeoptimizes) {
 
   Dart_Handle source = NewString(kLoadSecond);
   Dart_Handle url = NewString(TestCase::url());
-  Dart_LoadSource(TestCase::lib(), url, Dart_Null(), source, 0, 0);
+  result = Dart_LoadSource(TestCase::lib(), url, Dart_Null(), source, 0, 0);
+  EXPECT_VALID(result);
   result = Dart_FinalizeLoading(false);
   EXPECT_VALID(result);
 

runtime/vm/flags.cc

@@ -414,10 +414,10 @@ int Flags::CompareFlagNames(const void* left, const void* right) {
   return strcmp(left_flag->name_, right_flag->name_);
 }
 
-bool Flags::ProcessCommandLineFlags(int number_of_vm_flags,
-                                    const char** vm_flags) {
+char* Flags::ProcessCommandLineFlags(int number_of_vm_flags,
+                                     const char** vm_flags) {
   if (initialized_) {
-    return false;
+    return strdup("Flags already set");
   }
 
   qsort(flags_, num_flags_, sizeof flags_[0], CompareFlagNames);
@@ -435,20 +435,20 @@ bool Flags::ProcessCommandLineFlags(int number_of_vm_flags,
 
   if (!FLAG_ignore_unrecognized_flags) {
     int unrecognized_count = 0;
+    TextBuffer error(64);
     for (intptr_t j = 0; j < num_flags_; j++) {
       Flag* flag = flags_[j];
       if (flag->IsUnrecognized()) {
         if (unrecognized_count == 0) {
-          OS::PrintErr("Unrecognized flags: %s", flag->name_);
+          error.Printf("Unrecognized flags: %s", flag->name_);
         } else {
-          OS::PrintErr(", %s", flag->name_);
+          error.Printf(", %s", flag->name_);
         }
         unrecognized_count++;
       }
     }
     if (unrecognized_count > 0) {
-      OS::PrintErr("\n");
-      exit(255);
+      return error.Steal();
     }
   }
   if (FLAG_print_flags) {
@@ -456,7 +456,7 @@ bool Flags::ProcessCommandLineFlags(int number_of_vm_flags,
   }
 
   initialized_ = true;
-  return true;
+  return NULL;
 }
 
 bool Flags::SetFlag(const char* name, const char* value, const char** error) {

runtime/vm/flags.h

@@ -63,7 +63,7 @@ class Flags {
                                     const char* name,
                                     const char* comment);
 
-  static bool ProcessCommandLineFlags(int argc, const char** argv);
+  static char* ProcessCommandLineFlags(int argc, const char** argv);
 
   static Flag* Lookup(const char* name);
 

runtime/vm/isolate.cc

@@ -1244,13 +1244,13 @@ void Isolate::DoneFinalizing() {
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 }
 
-bool Isolate::MakeRunnable() {
+const char* Isolate::MakeRunnable() {
   ASSERT(Isolate::Current() == NULL);
 
   MutexLocker ml(mutex_);
   // Check if we are in a valid state to make the isolate runnable.
   if (is_runnable() == true) {
-    return false;  // Already runnable.
+    return "Isolate is already runnable";
   }
   // Set the isolate as runnable and if we are being spawned schedule
   // isolate on thread pool for execution.
@@ -1294,7 +1294,7 @@ bool Isolate::MakeRunnable() {
     GetRunnableHeapSizeMetric()->set_value(heap_size);
   }
 #endif  // !PRODUCT
-  return true;
+  return NULL;
 }
 
 bool Isolate::VerifyPauseCapability(const Object& capability) const {

runtime/vm/isolate.h

@@ -302,7 +302,7 @@ class Isolate : public BaseIsolate {
                      bool dont_delete_reload_context = false);
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 
-  bool MakeRunnable();
+  const char* MakeRunnable();
   void Run();
 
   MessageHandler* message_handler() const { return message_handler_; }