[vm] Ensure unoptimized compilations do not have an active isolate

Our compiler shouldn't depend on current isolate, since it can
run on any isolate within an IG.

Doing this change, reveals two existing dependencies on current
isolate from compiler
- resolving native symbols in unoptimized compilations
- issuing of debug events for breakpoints

For the former we'll re-enter the currently active isolate that
triggered unoptimized compilation.
=> We may want to change that embedder API to not be based on
   handles and instead give embedder a simple `const char*`.

For the ladder we'll enter the isolate corresponding to the
breakpoint debug event to be issued. We are at place where
all mutators are stopped, so that does seem okish.
=> Future could remove this by making Object Id Ring per-IG

Issue https://github.com/dart-lang/sdk/issues/48523

TEST=service_2/break_on_function_many_child_isolates_test/dds

Change-Id: Id246db5972ae505e82f637ce04bb2302bed76257
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278901
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>

runtime/tests/vm/dart/isolates/reload_utils.dart

@@ -187,6 +187,46 @@ class Reloader {
     return reloadResult;
   }
 
+  Future<Stream> getDebugStream() async {
+    return await _remoteVm.getEventStream('Debug');
+  }
+
+  Future<String> getIsolateId(String name) async {
+    final vm = await _remoteVm.rpc.sendRequest('getVM', {});
+    final isolates = vm['isolates'];
+    for (final isolate in isolates) {
+      if (isolate['name'].contains(name)) {
+        return isolate['id'];
+      }
+    }
+    throw 'Did not find isolate with name "$name"';
+  }
+
+  Future addBreakpoint(int line, {String? isolateId}) async {
+    isolateId ??= await _remoteVm.mainId;
+    final rootScriptId = await getRootScriptId();
+    var result = await _remoteVm.rpc.sendRequest('addBreakpoint', {
+      'isolateId': isolateId,
+      'scriptId': rootScriptId,
+      'line': line,
+    });
+    if (result['enabled'] != true) throw 'failed to set breakpoint';
+  }
+
+  Future<String> getRootScriptId() async {
+    final isolateId = await _remoteVm.mainId;
+    final result = await _remoteVm.rpc.sendRequest('getIsolate', {
+      'isolateId': isolateId,
+    });
+    final rootLibraryId = result['rootLib']['id'];
+
+    final result2 = await _remoteVm.rpc.sendRequest('getObject', {
+      'isolateId': isolateId,
+      'objectId': rootLibraryId,
+    });
+    return result2['scripts'][0]['id'];
+  }
+
   Future<int> close() async {
     await _remoteVm.disconnect();
     final exitCode = await _process.exitCode;

runtime/tests/vm/dart/regress_48523_test.dart

@@ -0,0 +1,115 @@
+// Copyright (c) 2021, 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 'dart:async';
+import 'dart:convert';
+
+import 'package:expect/expect.dart';
+
+import 'isolates/reload_utils.dart';
+
+main() async {
+  if (!currentVmSupportsReload) return;
+
+  await withTempDir((String tempDir) async {
+    final dills = await generateDills(tempDir, dartTestFile());
+    final reloader = await launchOn(dills[0]);
+
+    await reloader.waitUntilStdoutContains('[testee] helper isolate is ready');
+
+    final helperIsolateId = await reloader.getIsolateId('helper-isolate');
+
+    // Set breakpoint.
+    final debugEvents = StreamIterator(await reloader.getDebugStream());
+    await reloader.addBreakpoint(7, isolateId: helperIsolateId);
+
+    // Reload 1
+    final reloadResult1 = await reloader.reload(dills[1]);
+    Expect.equals('ReloadReport', reloadResult1['type']);
+    Expect.equals(true, reloadResult1['success']);
+
+    // Now we should get a debug resolved event.
+    if (!await debugEvents.moveNext()) throw 'failed';
+    final event = debugEvents.current;
+    print(JsonEncoder.withIndent('  ').convert(event));
+    if (event['kind'] != 'BreakpointResolved') throw 'failed';
+    print('Got breakpoint resolved event ($event)');
+
+    // Continue testee, which will run (and therefore compile) old closure
+    // without a script.
+    await reloader.waitUntilStdoutContains('[testee] running old closure');
+    await reloader.waitUntilStdoutContains('[testee] done');
+
+    // Reload 1
+    print('reloading');
+    final reloadResult2 = await reloader.reload(dills[2]);
+    Expect.equals('ReloadReport', reloadResult2['type']);
+    Expect.equals(true, reloadResult2['success']);
+    print('reload 2 done');
+
+    await reloader.waitUntilStdoutContains('[testee] shutting down');
+
+    final int exitCode = await reloader.close();
+    Expect.equals(0, exitCode);
+  });
+}
+
+String dartTestFile() => '''
+import 'dart:async';
+import 'dart:isolate';
+dynamic getAnonymousClosure() {
+  return () {                               // @include-in-reload-0
+    print('[testee] running old closure');  // @include-in-reload-0
+    if (int.parse('1') == 0) {              // @include-in-reload-0
+      throw 'should not execute';           // @include-in-reload-0
+    }                                       // @include-in-reload-0
+  };                                        // @include-in-reload-0
+  return null;                              // @include-in-reload-1
+}
+
+var escapedOldClosure;
+
+Future main() async {
+  escapedOldClosure = getAnonymousClosure();
+
+  Isolate.spawn((_) {
+    print('[testee] helper isolate is ready');
+    ReceivePort();
+  }, null, debugName: 'helper-isolate');
+
+  // Debugger should now set breakpoint on
+  // myOldClosure.
+
+  // Wait until we got reloaded.
+  while (await waitUntilReloadDone());
+
+  // Now run the old closure (which has breakpoint in it).
+  escapedOldClosure();
+
+  print('[testee] done');
+
+  // Wait until we got reloaded.
+  while (await waitUntilReloadDone2());
+
+  print('[testee] shutting down');
+}
+
+final timeout = const Duration(milliseconds: 200);
+
+@pragma('vm:never-inline')
+Future<bool> waitUntilReloadDone() async {
+  await Future.delayed(timeout);   // @include-in-reload-0
+  return true;                     // @include-in-reload-0
+  return false;                    // @include-in-reload-1
+  throw 'unexpected';              // @include-in-reload-2
+}
+
+@pragma('vm:never-inline')
+Future<bool> waitUntilReloadDone2() async {
+  throw 'unexpected';              // @include-in-reload-0
+  await Future.delayed(timeout);   // @include-in-reload-1
+  return true;                     // @include-in-reload-1
+  return false;                    // @include-in-reload-2
+}
+''';

runtime/tests/vm/dart_2/isolates/reload_utils.dart

@@ -190,6 +190,46 @@ class Reloader {
     return reloadResult;
   }
 
+  Future<Stream> getDebugStream() async {
+    return await _remoteVm.getEventStream('Debug');
+  }
+
+  Future<String> getIsolateId(String name) async {
+    final vm = await _remoteVm.rpc.sendRequest('getVM', {});
+    final isolates = vm['isolates'];
+    for (final isolate in isolates) {
+      if (isolate['name'].contains(name)) {
+        return isolate['id'];
+      }
+    }
+    throw 'Did not find isolate with name "$name"';
+  }
+
+  Future addBreakpoint(int line, {String isolateId}) async {
+    isolateId ??= await _remoteVm.mainId;
+    final rootScriptId = await getRootScriptId();
+    var result = await _remoteVm.rpc.sendRequest('addBreakpoint', {
+      'isolateId': isolateId,
+      'scriptId': rootScriptId,
+      'line': line,
+    });
+    if (result['enabled'] != true) throw 'failed to set breakpoint';
+  }
+
+  Future<String> getRootScriptId() async {
+    final isolateId = await _remoteVm.mainId;
+    final result = await _remoteVm.rpc.sendRequest('getIsolate', {
+      'isolateId': isolateId,
+    });
+    final rootLibraryId = result['rootLib']['id'];
+
+    final result2 = await _remoteVm.rpc.sendRequest('getObject', {
+      'isolateId': isolateId,
+      'objectId': rootLibraryId,
+    });
+    return result2['scripts'][0]['id'];
+  }
+
   Future<int> close() async {
     await _remoteVm.disconnect();
     final exitCode = await _process.exitCode;

runtime/tests/vm/dart_2/regress_48523_test.dart

@@ -0,0 +1,117 @@
+// Copyright (c) 2021, 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.
+
+// @dart = 2.9
+
+import 'dart:async';
+import 'dart:convert';
+
+import 'package:expect/expect.dart';
+
+import 'isolates/reload_utils.dart';
+
+main() async {
+  if (!currentVmSupportsReload) return;
+
+  await withTempDir((String tempDir) async {
+    final dills = await generateDills(tempDir, dartTestFile());
+    final reloader = await launchOn(dills[0]);
+
+    await reloader.waitUntilStdoutContains('[testee] helper isolate is ready');
+
+    final helperIsolateId = await reloader.getIsolateId('helper-isolate');
+
+    // Set breakpoint.
+    final debugEvents = StreamIterator(await reloader.getDebugStream());
+    await reloader.addBreakpoint(7, isolateId: helperIsolateId);
+
+    // Reload 1
+    final reloadResult1 = await reloader.reload(dills[1]);
+    Expect.equals('ReloadReport', reloadResult1['type']);
+    Expect.equals(true, reloadResult1['success']);
+
+    // Now we should get a debug resolved event.
+    if (!await debugEvents.moveNext()) throw 'failed';
+    final event = debugEvents.current;
+    print(JsonEncoder.withIndent('  ').convert(event));
+    if (event['kind'] != 'BreakpointResolved') throw 'failed';
+    print('Got breakpoint resolved event ($event)');
+
+    // Continue testee, which will run (and therefore compile) old closure
+    // without a script.
+    await reloader.waitUntilStdoutContains('[testee] running old closure');
+    await reloader.waitUntilStdoutContains('[testee] done');
+
+    // Reload 1
+    print('reloading');
+    final reloadResult2 = await reloader.reload(dills[2]);
+    Expect.equals('ReloadReport', reloadResult2['type']);
+    Expect.equals(true, reloadResult2['success']);
+    print('reload 2 done');
+
+    await reloader.waitUntilStdoutContains('[testee] shutting down');
+
+    final int exitCode = await reloader.close();
+    Expect.equals(0, exitCode);
+  });
+}
+
+String dartTestFile() => '''
+import 'dart:async';
+import 'dart:isolate';
+dynamic getAnonymousClosure() {
+  return () {                               // @include-in-reload-0
+    print('[testee] running old closure');  // @include-in-reload-0
+    if (int.parse('1') == 0) {              // @include-in-reload-0
+      throw 'should not execute';           // @include-in-reload-0
+    }                                       // @include-in-reload-0
+  };                                        // @include-in-reload-0
+  return null;                              // @include-in-reload-1
+}
+
+var escapedOldClosure;
+
+Future main() async {
+  escapedOldClosure = getAnonymousClosure();
+
+  Isolate.spawn((_) {
+    print('[testee] helper isolate is ready');
+    ReceivePort();
+  }, null, debugName: 'helper-isolate');
+
+  // Debugger should now set breakpoint on
+  // myOldClosure.
+
+  // Wait until we got reloaded.
+  while (await waitUntilReloadDone());
+
+  // Now run the old closure (which has breakpoint in it).
+  escapedOldClosure();
+
+  print('[testee] done');
+
+  // Wait until we got reloaded.
+  while (await waitUntilReloadDone2());
+
+  print('[testee] shutting down');
+}
+
+final timeout = const Duration(milliseconds: 200);
+
+@pragma('vm:never-inline')
+Future<bool> waitUntilReloadDone() async {
+  await Future.delayed(timeout);   // @include-in-reload-0
+  return true;                     // @include-in-reload-0
+  return false;                    // @include-in-reload-1
+  throw 'unexpected';              // @include-in-reload-2
+}
+
+@pragma('vm:never-inline')
+Future<bool> waitUntilReloadDone2() async {
+  throw 'unexpected';              // @include-in-reload-0
+  await Future.delayed(timeout);   // @include-in-reload-1
+  return true;                     // @include-in-reload-1
+  return false;                    // @include-in-reload-2
+}
+''';

runtime/vm/compiler/backend/il.cc

@@ -6635,7 +6635,22 @@ void NativeCallInstr::SetupNative() {
     return;
   }
 
-  Zone* zone = Thread::Current()->zone();
+  Thread* thread = Thread::Current();
+  Zone* zone = thread->zone();
+
+  // Currently we perform unoptimized compilations only on mutator threads. If
+  // the compiler has to resolve a native to a function pointer it calls out to
+  // the embedder to do so.
+  //
+  // Unfortunately that embedder API was designed by giving it a handle to a
+  // string. So the embedder will have to call back into the VM to convert it to
+  // a C string - which requires an active isolate.
+  //
+  // => To allow this `dart-->jit-compiler-->embedder-->dart api` we set the
+  //    active isolate again.
+  //
+  ActiveIsolateScope active_isolate(thread);
+
   const Class& cls = Class::Handle(zone, function().Owner());
   const Library& library = Library::Handle(zone, cls.library());
 

runtime/vm/compiler/jit/compiler.cc

@@ -677,12 +677,14 @@ static ObjectPtr CompileFunctionHelper(CompilationPipeline* pipeline,
                                        const Function& function,
                                        volatile bool optimized,
                                        intptr_t osr_id) {
+  Thread* const thread = Thread::Current();
+  NoActiveIsolateScope no_active_isolate(thread);
+
   ASSERT(!FLAG_precompiled_mode);
   ASSERT(!optimized || function.WasCompiled() || function.ForceOptimize());
   if (function.ForceOptimize()) optimized = true;
   LongJumpScope jump;
   if (setjmp(*jump.Set()) == 0) {
-    Thread* const thread = Thread::Current();
     StackZone stack_zone(thread);
     Zone* const zone = stack_zone.GetZone();
     const bool trace_compiler =
@@ -879,7 +881,6 @@ ObjectPtr Compiler::CompileOptimizedFunction(Thread* thread,
                                              const Function& function,
                                              intptr_t osr_id) {
   VMTagScope tag_scope(thread, VMTag::kCompileOptimizedTagId);
-  NoActiveIsolateScope no_active_isolate(thread);
 
 #if defined(SUPPORT_TIMELINE)
   const char* event_name;

runtime/vm/debugger.cc

@@ -337,7 +337,7 @@ bool Debugger::NeedsDebugEvents() {
     // E.g., NoActiveIsolateScope.
     return false;
   }
-  ASSERT(isolate_ == Isolate::Current());
+  RELEASE_ASSERT(isolate_ == Isolate::Current());
   ASSERT(!Isolate::IsSystemIsolate(isolate_));
   return FLAG_warn_on_pause_with_no_debugger || Service::debug_stream.enabled();
 }
@@ -3192,6 +3192,13 @@ void GroupDebugger::NotifyCompilation(const Function& function) {
   for (intptr_t i = 0; i < breakpoint_locations_.length(); i++) {
     BreakpointLocation* location = breakpoint_locations_.At(i);
     if (EnsureLocationIsInFunction(zone, resolved_function, location)) {
+      // All mutators are stopped (see RELEASE_ASSERT above). We temporarily
+      // enter the isolate for which the breakpoint was registered.
+      // The code path below may issue service events which will use the active
+      // isolate's object-id ring for naming VM objects.
+      ActiveIsolateScope active_isolate(thread,
+                                        location->debugger()->isolate());
+
       // Ensure the location is resolved for the original function.
       location->EnsureIsResolved(function, location->token_pos());
       if (FLAG_verbose_debug) {

runtime/vm/debugger.h

@@ -700,6 +700,8 @@ class Debugger {
   explicit Debugger(Isolate* isolate);
   ~Debugger();
 
+  Isolate* isolate() const { return isolate_; }
+
   void NotifyIsolateCreated();
   void Shutdown();
 

runtime/vm/isolate.h

@@ -1847,12 +1847,40 @@ class NoActiveIsolateScope : public StackResource {
   NoActiveIsolateScope() : NoActiveIsolateScope(Thread::Current()) {}
   explicit NoActiveIsolateScope(Thread* thread)
       : StackResource(thread), thread_(thread) {
+    outer_ = thread_->no_active_isolate_scope_;
     saved_isolate_ = thread_->isolate_;
+
+    thread_->no_active_isolate_scope_ = this;
     thread_->isolate_ = nullptr;
   }
   ~NoActiveIsolateScope() {
     ASSERT(thread_->isolate_ == nullptr);
     thread_->isolate_ = saved_isolate_;
+    thread_->no_active_isolate_scope_ = outer_;
+  }
+
+ private:
+  friend class ActiveIsolateScope;
+
+  Thread* thread_;
+  Isolate* saved_isolate_;
+  NoActiveIsolateScope* outer_;
+};
+
+class ActiveIsolateScope : public StackResource {
+ public:
+  explicit ActiveIsolateScope(Thread* thread)
+      : ActiveIsolateScope(thread,
+                           thread->no_active_isolate_scope_->saved_isolate_) {}
+
+  ActiveIsolateScope(Thread* thread, Isolate* isolate)
+      : StackResource(thread), thread_(thread) {
+    RELEASE_ASSERT(thread->isolate() == nullptr);
+    thread_->isolate_ = isolate;
+  }
+  ~ActiveIsolateScope() {
+    ASSERT(thread_->isolate_ != nullptr);
+    thread_->isolate_ = nullptr;
   }
 
  private:

runtime/vm/thread.h

@@ -54,6 +54,7 @@ class Library;
 class Object;
 class OSThread;
 class JSONObject;
+class NoActiveIsolateScope;
 class PcDescriptors;
 class RuntimeEntry;
 class Smi;
@@ -1273,6 +1274,7 @@ class Thread : public ThreadState {
   CompilerState* compiler_state_ = nullptr;
   HierarchyInfo* hierarchy_info_;
   TypeUsageInfo* type_usage_info_;
+  NoActiveIsolateScope* no_active_isolate_scope_ = nullptr;
 
   CompilerTimings* compiler_timings_ = nullptr;
 
@@ -1371,7 +1373,7 @@ class Thread : public ThreadState {
 #undef REUSABLE_FRIEND_DECLARATION
 
   friend class ApiZone;
-  friend class DisabledNoActiveIsolateScope;
+  friend class ActiveIsolateScope;
   friend class InterruptChecker;
   friend class Isolate;
   friend class IsolateGroup;