[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: Ic96845b19951170effea3dd3619f798e2c72968a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/402781
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Siva Annamalai <[email protected]>

Author: a-siva

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

@@ -17,6 +17,7 @@ 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;
@@ -93,17 +94,18 @@ class CompileJSCommand extends CompileSubcommandCommand {
     final args = argResults!;
     var snapshot = sdk.dart2jsAotSnapshot;
     var runtime = sdk.dartAotRuntime;
+    var useExecProcess = 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;
+      useExecProcess = false;
     }
     final dart2jsCommand = [
-      runtime,
       snapshot,
       '--libraries-spec=${sdk.librariesJson}',
       '--cfe-invocation-modes=compile',
@@ -112,8 +114,13 @@ class CompileJSCommand extends CompileSubcommandCommand {
       if (args.rest.isNotEmpty) ...args.rest.sublist(0),
     ];
     try {
-      final exitCode = await runProcessInheritStdio(dart2jsCommand);
-      return exitCode;
+      VmInteropHandler.run(
+        runtime,
+        dart2jsCommand,
+        packageConfigOverride: null,
+        useExecProcess: useExecProcess,
+      );
+      return 0;
     } catch (e, st) {
       log.stderr('Error: JS compilation failed');
       log.stderr(e.toString());
@@ -135,8 +142,12 @@ class CompileDDCCommand extends CompileSubcommandCommand {
   // This command is an internal developer command used by tools and is
   // hidden in the help message.
   CompileDDCCommand({bool verbose = false})
-      : super(cmdName, 'Compile Dart to JavaScript using ddc.', verbose,
-              hidden:true,);
+      : super(
+          cmdName,
+          'Compile Dart to JavaScript using ddc.',
+          verbose,
+          hidden: true,
+        );
 
   @override
   String get invocation => '${super.invocation} <dart entry point>';

pkg/dartdev/lib/src/vm_interop_handler.dart

@@ -2,6 +2,7 @@
 // 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 'dart:io';
 import 'dart:isolate';
 
 /// Contains methods used to communicate DartDev results back to the VM.
@@ -18,6 +19,10 @@ 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 [useExecProcess] is given and set to true, the script is executed by
+  /// execing it (On Linux/Mac the exec call is used, on Windows a new child
+  /// process is started).
   static void run(
     String script,
     List<String> args, {
@@ -27,16 +32,33 @@ abstract class VmInteropHandler {
     //
     // See https://github.com/dart-lang/sdk/issues/53576
     bool markMainIsolateAsSystemIsolate = false,
+    bool useExecProcess = false,
   }) {
+    List<String> argsList;
+    if (useExecProcess && Platform.isWindows) {
+      // On Windows if a new process is used to execute the script we
+      // need to escape the script path and all the arguments as we
+      // construct a single command line string to be passed to the
+      // Windows process create call.
+      if (script.contains(' ') && !script.contains('"')) {
+        // Escape paths that may contain spaces
+        script = '"$script"';
+      }
+      argsList = [
+        for (int i = 0; i < args.length; i++) _windowsArgumentEscape(args[i]),
+      ];
+    } else {
+      // Copy the list so it doesn't get GC'd underneath us.
+      argsList = args.toList();
+    }
     final port = _port;
     if (port == null) return;
     final message = <dynamic>[
-      _kResultRun,
+      useExecProcess? _kResultRunExec : _kResultRun,
       script,
       packageConfigOverride,
       markMainIsolateAsSystemIsolate,
-      // Copy the list so it doesn't get GC'd underneath us.
-      args.toList()
+      argsList
     ];
     port.send(message);
   }
@@ -50,9 +72,65 @@ abstract class VmInteropHandler {
     port.send(message);
   }
 
+  /// This code is identical to the one in process_patch.dart, please ensure
+  /// changes made here are also done in process_patch.dart.
+  /// TODO : figure out if this functionality can be abstracted out to a
+  ///        common place.
+  static String _windowsArgumentEscape(String argument) {
+    if (argument.isEmpty) {
+      return '""';
+    }
+    var result = argument;
+    if (argument.contains('\t') ||
+        argument.contains(' ') ||
+        argument.contains('"')) {
+      // Produce something that the C runtime on Windows will parse
+      // back as this string.
+
+      // Replace any number of '\' followed by '"' with
+      // twice as many '\' followed by '\"'.
+      var backslash = '\\'.codeUnitAt(0);
+      var sb = StringBuffer();
+      var nextPos = 0;
+      var quotePos = argument.indexOf('"', nextPos);
+      while (quotePos != -1) {
+        var numBackslash = 0;
+        var pos = quotePos - 1;
+        while (pos >= 0 && argument.codeUnitAt(pos) == backslash) {
+          numBackslash++;
+          pos--;
+        }
+        sb.write(argument.substring(nextPos, quotePos - numBackslash));
+        for (var i = 0; i < numBackslash; i++) {
+          sb.write(r'\\');
+        }
+        sb.write(r'\"');
+        nextPos = quotePos + 1;
+        quotePos = argument.indexOf('"', nextPos);
+      }
+      sb.write(argument.substring(nextPos, argument.length));
+      result = sb.toString();
+
+      // Add '"' at the beginning and end and replace all '\' at
+      // the end with two '\'.
+      sb = StringBuffer('"');
+      sb.write(result);
+      nextPos = argument.length - 1;
+      while (argument.codeUnitAt(nextPos) == backslash) {
+        sb.write('\\');
+        nextPos--;
+      }
+      sb.write('"');
+      result = sb.toString();
+    }
+
+    return result;
+  }
+
   // Note: keep in sync with runtime/bin/dartdev_isolate.h
   static const int _kResultRun = 1;
-  static const int _kResultExit = 2;
+  static const int _kResultRunExec = 2;
+  static const int _kResultExit = 3;
 
   static SendPort? _port;
 }

runtime/bin/dartdev_isolate.cc

@@ -38,6 +38,7 @@ 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;
@@ -204,6 +205,86 @@ void DartDevIsolate::DartDevRunner::DartDevResultCallback(
       }
       break;
     }
+    case DartDevIsolate::DartDev_Result_RunExec: {
+      result_ = DartDevIsolate::DartDev_Result_RunExec;
+      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;
@@ -314,7 +395,9 @@ 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

@@ -28,7 +28,8 @@ class DartDevIsolate {
   typedef enum {
     DartDev_Result_Unknown = -1,
     DartDev_Result_Run = 1,
-    DartDev_Result_Exit = 2,
+    DartDev_Result_RunExec = 2,
+    DartDev_Result_Exit = 3,
   } DartDev_Result;
 
   // Returns true if there does not exist a file at |script_uri| or the URI is
@@ -58,6 +59,7 @@ class DartDevIsolate {
       Dart_IsolateGroupCreateCallback create_isolate,
       char** packages_file,
       char** script,
+      CommandLineOptions* vm_options,
       CommandLineOptions* dart_options);
 
  protected:
@@ -83,11 +85,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();
   };
@@ -98,6 +100,7 @@ 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

@@ -1433,7 +1433,7 @@ 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,
-        &dart_options);
+        &vm_options, &dart_options);
     ASSERT(dartdev_result != DartDevIsolate::DartDev_Result_Unknown);
     ran_dart_dev = true;
     should_run_user_program =

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;
-  char** string_args =
+  const char** string_args = const_cast<const char**>(
       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,
-                   char* arguments[],
+                   const char* arguments[],
                    intptr_t arguments_length,
                    const char* working_directory,
                    char* environment[],
@@ -114,6 +114,23 @@ 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);