Revert "[dartdev] Use VmInteropHandler for invoking sub commands"

This reverts commit 08252fc.

Reason for revert: #59784

Original change's description:
> [dartdev] Use VmInteropHandler for invoking sub commands
>
> Use VmInteropHandler for invoking sub commands instead of running them
> in an isolate. Running sub commands in an isolate causes an increased footprint.
> Changing this to use VmInteropHandler avoids the additional memory footprint.
>
> Commands that need to use an AOT runtime for execution now exec the AOT
> runtime and run the command.
>
> TEST=ci
>
> Change-Id: If7aed1cab2fec9d9940bd562ad5aa9c4e9a6ac7f
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398604
> Reviewed-by: Ben Konyi <[email protected]>
> Reviewed-by: Brian Quinlan <[email protected]>
> Commit-Queue: Siva Annamalai <[email protected]>

Change-Id: I82a997d49a7d52e1fdaa7d75f509603ebe5e51dd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/401901
Reviewed-by: Siva Annamalai <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>

Author: bkonyi

pkg/dartdev/lib/src/commands/compile.dart

@@ -17,7 +17,6 @@ import '../experiments.dart';
 import '../native_assets.dart';
 import '../sdk.dart';
 import '../utils.dart';
-import '../vm_interop_handler.dart';
 
 const int genericErrorExitCode = 255;
 const int compileErrorExitCode = 254;
@@ -94,18 +93,17 @@ class CompileJSCommand extends CompileSubcommandCommand {
     final args = argResults!;
     var snapshot = sdk.dart2jsAotSnapshot;
     var runtime = sdk.dartAotRuntime;
-    var isAOT = true;
     if (!Sdk.checkArtifactExists(snapshot, logError: false)) {
       // AOT snapshots cannot be generated on IA32, so we need this fallback
       // branch until support for IA32 is dropped (https://dartbug.com/49969).
       snapshot = sdk.dart2jsSnapshot;
+      runtime = sdk.dart;
       if (!Sdk.checkArtifactExists(snapshot)) {
         return genericErrorExitCode;
       }
-      runtime = sdk.dart;
-      isAOT = false;
     }
     final dart2jsCommand = [
+      runtime,
       snapshot,
       '--libraries-spec=${sdk.librariesJson}',
       '--cfe-invocation-modes=compile',
@@ -114,13 +112,8 @@ class CompileJSCommand extends CompileSubcommandCommand {
       if (args.rest.isNotEmpty) ...args.rest.sublist(0),
     ];
     try {
-      VmInteropHandler.run(
-        runtime,
-        dart2jsCommand,
-        packageConfigOverride: null,
-        isAOT : isAOT,
-      );
-      return 0;
+      final exitCode = await runProcessInheritStdio(dart2jsCommand);
+      return exitCode;
     } catch (e, st) {
       log.stderr('Error: JS compilation failed');
       log.stderr(e.toString());

pkg/dartdev/lib/src/vm_interop_handler.dart

@@ -18,9 +18,6 @@ abstract class VmInteropHandler {
   ///
   /// If [markMainIsolateAsSystemIsolate] is given and set to true, the spawned
   /// isolate will run with `--mark-main-isolate-as-system-isolate` enabled.
-  ///
-  /// If [isAOT] is given and set to true, the script is executed by
-  /// execing the dartaotruntime.
   static void run(
     String script,
     List<String> args, {
@@ -30,12 +27,11 @@ abstract class VmInteropHandler {
     //
     // See https://github.com/dart-lang/sdk/issues/53576
     bool markMainIsolateAsSystemIsolate = false,
-    bool isAOT = false,
   }) {
     final port = _port;
     if (port == null) return;
     final message = <dynamic>[
-      isAOT ? _kResultRunAOT : _kResultRunJIT,
+      _kResultRun,
       script,
       packageConfigOverride,
       markMainIsolateAsSystemIsolate,
@@ -55,9 +51,8 @@ abstract class VmInteropHandler {
   }
 
   // Note: keep in sync with runtime/bin/dartdev_isolate.h
-  static const int _kResultRunJIT = 1;
-  static const int _kResultRunAOT = 2;
-  static const int _kResultExit = 3;
+  static const int _kResultRun = 1;
+  static const int _kResultExit = 2;
 
   static SendPort? _port;
 }

runtime/bin/dartdev_isolate.cc

@@ -38,7 +38,6 @@ DartDevIsolate::DartDevRunner DartDevIsolate::runner_ =
     DartDevIsolate::DartDevRunner();
 bool DartDevIsolate::should_run_dart_dev_ = false;
 bool DartDevIsolate::print_usage_error_ = false;
-CommandLineOptions* DartDevIsolate::vm_options_ = nullptr;
 Monitor* DartDevIsolate::DartDevRunner::monitor_ = new Monitor();
 DartDevIsolate::DartDev_Result DartDevIsolate::DartDevRunner::result_ =
     DartDevIsolate::DartDev_Result_Unknown;
@@ -139,7 +138,7 @@ void DartDevIsolate::DartDevRunner::Run(
   Thread::Start("DartDev Runner", RunCallback, reinterpret_cast<uword>(this));
   monitor_->WaitMicros(Monitor::kNoTimeout);
 
-  if (result_ == DartDevIsolate::DartDev_Result_RunJIT) {
+  if (result_ == DartDevIsolate::DartDev_Result_Run) {
     // Clear the DartDev dart_options and replace them with the processed
     // options provided by DartDev.
     dart_options_->Reset();
@@ -158,8 +157,8 @@ void DartDevIsolate::DartDevRunner::DartDevResultCallback(
   ASSERT(message->type == Dart_CObject_kArray);
   int32_t type = GetArrayItem(message, 0)->value.as_int32;
   switch (type) {
-    case DartDevIsolate::DartDev_Result_RunJIT: {
-      result_ = DartDevIsolate::DartDev_Result_RunJIT;
+    case DartDevIsolate::DartDev_Result_Run: {
+      result_ = DartDevIsolate::DartDev_Result_Run;
       ASSERT(GetArrayItem(message, 1)->type == Dart_CObject_kString);
       auto item2 = GetArrayItem(message, 2);
 
@@ -205,86 +204,6 @@ void DartDevIsolate::DartDevRunner::DartDevResultCallback(
       }
       break;
     }
-    case DartDevIsolate::DartDev_Result_RunAOT: {
-      result_ = DartDevIsolate::DartDev_Result_RunAOT;
-      ASSERT(GetArrayItem(message, 1)->type == Dart_CObject_kString);
-      auto item2 = GetArrayItem(message, 2);
-
-      ASSERT(item2->type == Dart_CObject_kString ||
-             item2->type == Dart_CObject_kNull);
-
-      auto item3 = GetArrayItem(message, 3);
-
-      ASSERT(item3->type == Dart_CObject_kBool);
-      const bool mark_main_isolate_as_system_isolate = item3->value.as_bool;
-      if (mark_main_isolate_as_system_isolate) {
-        Options::set_mark_main_isolate_as_system_isolate(true);
-      }
-
-      if (*script_ != nullptr) {
-        free(*script_);
-      }
-      if (*package_config_override_ != nullptr) {
-        free(*package_config_override_);
-        *package_config_override_ = nullptr;
-      }
-      *script_ = Utils::StrDup(GetArrayItem(message, 1)->value.as_string);
-
-      if (item2->type == Dart_CObject_kString) {
-        *package_config_override_ = Utils::StrDup(item2->value.as_string);
-      }
-
-      intptr_t num_vm_options = 0;
-      const char** vm_options = nullptr;
-      ASSERT(GetArrayItem(message, 4)->type == Dart_CObject_kArray);
-      Dart_CObject* args = GetArrayItem(message, 4);
-      intptr_t argc = args->value.as_array.length;
-      Dart_CObject** dart_args = args->value.as_array.values;
-
-      if (vm_options_ != nullptr) {
-        num_vm_options = vm_options_->count();
-        vm_options = vm_options_->arguments();
-      }
-      auto deleter = [](char** args) {
-        for (intptr_t i = 0; i < argc_; ++i) {
-          free(args[i]);
-        }
-        delete[] args;
-      };
-      // Total count of arguments to be passed to the script being execed.
-      argc_ = argc + num_vm_options + 1;
-
-      // Array of arguments to be passed to the script being execed.
-      argv_ = std::unique_ptr<char*[], void (*)(char**)>(new char*[argc_ + 1],
-                                                         deleter);
-
-      intptr_t idx = 0;
-      // Copy in name of the script to run (dartaotruntime).
-      argv_[0] = Utils::StrDup(GetArrayItem(message, 1)->value.as_string);
-      idx += 1;
-      // Copy in any vm options that need to be passed to the execed process.
-      for (intptr_t i = 0; i < num_vm_options; ++i) {
-        argv_[i + idx] = Utils::StrDup(vm_options[i]);
-      }
-      idx += num_vm_options;
-      // Copy in the dart options that need to be passed to the command.
-      for (intptr_t i = 0; i < argc; ++i) {
-        argv_[i + idx] = Utils::StrDup(dart_args[i]->value.as_string);
-      }
-      // Null terminate the argv array.
-      argv_[argc + idx] = nullptr;
-
-      // Exec the script to be run and pass the arguments.
-      char err_msg[256];
-      err_msg[0] = '\0';
-      int ret = Process::Exec(nullptr, *script_,
-                              const_cast<const char**>(argv_.get()), argc_,
-                              nullptr, err_msg, sizeof(err_msg));
-      if (ret != 0) {
-        ProcessError(err_msg, kErrorExitCode);
-      }
-      break;
-    }
     case DartDevIsolate::DartDev_Result_Exit: {
       ASSERT(GetArrayItem(message, 1)->type == Dart_CObject_kInt32);
       int32_t dartdev_exit_code = GetArrayItem(message, 1)->value.as_int32;
@@ -395,9 +314,7 @@ DartDevIsolate::DartDev_Result DartDevIsolate::RunDartDev(
     Dart_IsolateGroupCreateCallback create_isolate,
     char** packages_file,
     char** script,
-    CommandLineOptions* vm_options,
     CommandLineOptions* dart_options) {
-  vm_options_ = vm_options;
   runner_.Run(create_isolate, packages_file, script, dart_options);
   return runner_.result();
 }

runtime/bin/dartdev_isolate.h

@@ -27,9 +27,8 @@ class DartDevIsolate {
   // Note: keep in sync with pkg/dartdev/lib/vm_interop_handler.dart
   typedef enum {
     DartDev_Result_Unknown = -1,
-    DartDev_Result_RunJIT = 1,
-    DartDev_Result_RunAOT = 2,
-    DartDev_Result_Exit = 3,
+    DartDev_Result_Run = 1,
+    DartDev_Result_Exit = 2,
   } DartDev_Result;
 
   // Returns true if there does not exist a file at |script_uri| or the URI is
@@ -59,7 +58,6 @@ class DartDevIsolate {
       Dart_IsolateGroupCreateCallback create_isolate,
       char** packages_file,
       char** script,
-      CommandLineOptions* vm_options,
       CommandLineOptions* dart_options);
 
  protected:
@@ -85,11 +83,11 @@ class DartDevIsolate {
     static char** package_config_override_;
     static std::unique_ptr<char*[], void (*)(char**)> argv_;
     static intptr_t argc_;
-    static Monitor* monitor_;
 
     Dart_IsolateGroupCreateCallback create_isolate_;
     CommandLineOptions* dart_options_;
     const char* packages_file_;
+    static Monitor* monitor_;
 
     DISALLOW_ALLOCATION();
   };
@@ -100,7 +98,6 @@ class DartDevIsolate {
   static DartDevRunner runner_;
   static bool should_run_dart_dev_;
   static bool print_usage_error_;
-  static CommandLineOptions* vm_options_;
 
   DISALLOW_ALLOCATION();
   DISALLOW_IMPLICIT_CONSTRUCTORS(DartDevIsolate);

runtime/bin/main_impl.cc

@@ -1421,11 +1421,11 @@ void main(int argc, char** argv) {
       Options::gen_snapshot_kind() == SnapshotKind::kNone) {
     DartDevIsolate::DartDev_Result dartdev_result = DartDevIsolate::RunDartDev(
         CreateIsolateGroupAndSetup, &package_config_override, &script_name,
-        &vm_options, &dart_options);
+        &dart_options);
     ASSERT(dartdev_result != DartDevIsolate::DartDev_Result_Unknown);
     ran_dart_dev = true;
     should_run_user_program =
-        (dartdev_result == DartDevIsolate::DartDev_Result_RunJIT);
+        (dartdev_result == DartDevIsolate::DartDev_Result_Run);
     if (should_run_user_program) {
       try_load_snapshots_lambda();
     }

runtime/bin/process.cc

@@ -114,9 +114,9 @@ void FUNCTION_NAME(Process_Start)(Dart_NativeArguments args) {
   const char* path = DartUtils::GetStringValue(path_handle);
   Dart_Handle arguments = Dart_GetNativeArgument(args, 3);
   intptr_t args_length = 0;
-  const char** string_args = const_cast<const char**>(
+  char** string_args =
       ExtractCStringList(arguments, status_handle,
-                         "Arguments must be builtin strings", &args_length));
+                         "Arguments must be builtin strings", &args_length);
   if (string_args == nullptr) {
     Dart_SetBooleanReturnValue(args, false);
     return;

runtime/bin/process.h

@@ -94,7 +94,7 @@ class Process {
   // process exit streams.
   static int Start(Namespace* namespc,
                    const char* path,
-                   const char* arguments[],
+                   char* arguments[],
                    intptr_t arguments_length,
                    const char* working_directory,
                    char* environment[],
@@ -114,23 +114,6 @@ class Process {
                    intptr_t exit_handler,
                    ProcessResult* result);
 
-  // Exec process.
-  // On systems that support 'exec' it will use it to replace
-  // the current process image with the image corresponding to 'path'
-  // On systems that do not support it (Windows) it will start in a
-  // child process in the same group as the parent so that when the parent
-  // is killed the child also dies.
-  // Returns 0 if the process could be execed successfully
-  // Returns -1 if the exec could not be done successfully and 'errmsg'
-  // points to the error message
-  static int Exec(Namespace* namespc,
-                  const char* path,
-                  const char* arguments[],
-                  intptr_t arguments_length,
-                  const char* working_directory,
-                  char* errmsg,
-                  intptr_t errmsg_len);
-
   // Kill a process with a given pid.
   static bool Kill(intptr_t id, int signal);
 

runtime/bin/process_fuchsia.cc

@@ -838,19 +838,6 @@ int Process::Start(Namespace* namespc,
   return starter.Start();
 }
 
-// The command line dart utility does not run on Fuchsia, this functionality
-// is not supported on that platform.
-int Process::Exec(Namespace* namespc,
-                  const char* path,
-                  const char** arguments,
-                  const char* working_directory,
-                  char* errmsg,
-                  intptr_t errmsg_len) {
-  snprintf(errmsg, errmsg_len,
-           "Process::Exec is not supported on this platform");
-  return -1;
-}
-
 intptr_t Process::SetSignalHandler(intptr_t signal) {
   errno = ENOSYS;
   return -1;