[vm/concurrency] Ensure fixes to switchable call (due to disabled code) is correctly performed

If two mutators concurrently execute a switchable call that targets a
code object which has been disabled, they both go to runtime trying to
fix that switchable call site's object pool.

This Cl ensures we use the same logic as for other miss handlers to
ensure we properly guard against concurrent accesses.

The attached regression test will trigger a segfault - though only
with low probability.

Fixes https://github.com/dart-lang/sdk/issues/46539

TEST=vm/dart{,_2}/isolates/regress_46539_test

Change-Id: I91d84d25d74fb742ea992016a581b118345dd404
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205649
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>

runtime/lib/object.cc

@@ -308,6 +308,11 @@ DEFINE_NATIVE_ENTRY(Internal_collectAllGarbage, 0, 0) {
   return Object::null();
 }
 
+DEFINE_NATIVE_ENTRY(Internal_deoptimizeFunctionsOnStack, 0, 0) {
+  DeoptimizeFunctionsOnStack();
+  return Object::null();
+}
+
 static bool ExtractInterfaceTypeArgs(Zone* zone,
                                      const Class& instance_cls,
                                      const TypeArguments& instance_type_args,

runtime/tests/vm/dart/isolates/regress_46539_test.dart

@@ -0,0 +1,77 @@
+// 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.
+
+// VMOptions=--optimization-filter=foo --enable-isolate-groups --experimental-enable-isolate-groups-jit --no-use-osr --optimization-counter-threshold=1 --deterministic
+
+// Important: This is a regression test for a concurrency issue, if this test
+// is flaky it is essentially failing!
+
+import 'dart:async';
+import 'dart:io';
+import 'dart:isolate';
+import 'dart:_internal' show VMInternalsForTesting;
+
+import 'package:expect/expect.dart';
+
+const int isolateCount = 3;
+const int deoptIsolateId = 0;
+const int polyIsolateId = 1;
+
+main() async {
+  final onExit = ReceivePort();
+  final onError = ReceivePort()
+    ..listen((error) {
+      print('Error: $error');
+      exitCode = 250;
+    });
+  for (int i = 0; i < isolateCount; ++i) {
+    await Isolate.spawn(isolate, i,
+        onExit: onExit.sendPort, onError: onError.sendPort);
+  }
+  final onExits = StreamIterator(onExit);
+  for (int i = 0; i < isolateCount; ++i) {
+    Expect.isTrue(await onExits.moveNext());
+  }
+  onExits.cancel();
+  onError.close();
+}
+
+final globalA = A();
+final globalB = B();
+
+isolate(int isolateId) {
+  final A a = isolateId == polyIsolateId ? globalB : globalA;
+  if (isolateId == polyIsolateId) {
+    // We start deopting after 1 second.
+    sleep(500000);
+  }
+
+  // This runs in unoptimized mode and will therefore do switchable calls.
+  final sw = Stopwatch()..start();
+  while (sw.elapsedMicroseconds < 2000000) {
+    a.foo(isolateId);
+    a.foo(isolateId);
+    a.foo(isolateId);
+    a.foo(isolateId);
+  }
+}
+
+class A {
+  @pragma('vm:never-inline')
+  foo(int isolateId) {
+    if (isolateId == deoptIsolateId) {
+      VMInternalsForTesting.deoptimizeFunctionsOnStack();
+    }
+  }
+}
+
+class B implements A {
+  @pragma('vm:never-inline')
+  foo(int isolateId) {}
+}
+
+void sleep(int us) {
+  final sw = Stopwatch()..start();
+  while (sw.elapsedMicroseconds < us);
+}

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

@@ -0,0 +1,77 @@
+// 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.
+
+// VMOptions=--optimization-filter=foo --enable-isolate-groups --experimental-enable-isolate-groups-jit --no-use-osr --optimization-counter-threshold=1 --deterministic
+
+// Important: This is a regression test for a concurrency issue, if this test
+// is flaky it is essentially failing!
+
+import 'dart:async';
+import 'dart:io';
+import 'dart:isolate';
+import 'dart:_internal' show VMInternalsForTesting;
+
+import 'package:expect/expect.dart';
+
+const int isolateCount = 3;
+const int deoptIsolateId = 0;
+const int polyIsolateId = 1;
+
+main() async {
+  final onExit = ReceivePort();
+  final onError = ReceivePort()
+    ..listen((error) {
+      print('Error: $error');
+      exitCode = 250;
+    });
+  for (int i = 0; i < isolateCount; ++i) {
+    await Isolate.spawn(isolate, i,
+        onExit: onExit.sendPort, onError: onError.sendPort);
+  }
+  final onExits = StreamIterator(onExit);
+  for (int i = 0; i < isolateCount; ++i) {
+    Expect.isTrue(await onExits.moveNext());
+  }
+  onExits.cancel();
+  onError.close();
+}
+
+final globalA = A();
+final globalB = B();
+
+isolate(int isolateId) {
+  final A a = isolateId == polyIsolateId ? globalB : globalA;
+  if (isolateId == polyIsolateId) {
+    // We start deopting after 1 second.
+    sleep(500000);
+  }
+
+  // This runs in unoptimized mode and will therefore do switchable calls.
+  final sw = Stopwatch()..start();
+  while (sw.elapsedMicroseconds < 2000000) {
+    a.foo(isolateId);
+    a.foo(isolateId);
+    a.foo(isolateId);
+    a.foo(isolateId);
+  }
+}
+
+class A {
+  @pragma('vm:never-inline')
+  foo(int isolateId) {
+    if (isolateId == deoptIsolateId) {
+      VMInternalsForTesting.deoptimizeFunctionsOnStack();
+    }
+  }
+}
+
+class B implements A {
+  @pragma('vm:never-inline')
+  foo(int isolateId) {}
+}
+
+void sleep(int us) {
+  final sw = Stopwatch()..start();
+  while (sw.elapsedMicroseconds < us);
+}

runtime/vm/bootstrap_natives.h

@@ -344,6 +344,7 @@ namespace dart {
   V(Internal_allocateTwoByteString, 1)                                         \
   V(Internal_writeIntoOneByteString, 3)                                        \
   V(Internal_writeIntoTwoByteString, 3)                                        \
+  V(Internal_deoptimizeFunctionsOnStack, 0)                                    \
   V(InvocationMirror_unpackTypeArguments, 2)                                   \
   V(NoSuchMethodError_existingMethodSignature, 3)                              \
   V(WeakProperty_getKey, 1)                                                    \

runtime/vm/compiler/stub_code_compiler_arm.cc

@@ -689,13 +689,13 @@ void StubCodeCompiler::GenerateFixCallersTargetStub(Assembler* assembler) {
   // calling into the runtime.
   __ EnterStubFrame();
   __ LoadImmediate(R1, 0);
-  __ Push(R9);  // Preserve cache (guarded CID as Smi).
+  __ Push(R1);  // Result slot.
   __ Push(R0);  // Preserve receiver.
-  __ Push(R1);
-  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 0);
-  __ Pop(CODE_REG);
+  __ Push(R9);  // Preserve cache.
+  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 2);
+  __ Pop(R9);  // Restore cache.
   __ Pop(R0);  // Restore receiver.
-  __ Pop(R9);  // Restore cache (guarded CID as Smi).
+  __ Pop(CODE_REG);  // Get target Code object.
   // Remove the stub frame.
   __ LeaveStubFrame();
   // Jump to the dart function.

runtime/vm/compiler/stub_code_compiler_arm64.cc

@@ -804,13 +804,13 @@ void StubCodeCompiler::GenerateFixCallersTargetStub(Assembler* assembler) {
   // Create a stub frame as we are pushing some objects on the stack before
   // calling into the runtime.
   __ EnterStubFrame();
-  __ Push(R5);  // Preserve cache (guarded CID as Smi).
+  __ Push(ZR);  // Result slot.
   __ Push(R0);  // Preserve receiver.
-  __ Push(ZR);
-  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 0);
-  __ Pop(CODE_REG);
-  __ Pop(R0);  // Restore receiver.
-  __ Pop(R5);  // Restore cache (guarded CID as Smi).
+  __ Push(R5);  // Preserve cache (guarded CID as Smi).
+  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 2);
+  __ Pop(R5);        // Restore cache (guarded CID as Smi).
+  __ Pop(R0);        // Restore receiver.
+  __ Pop(CODE_REG);  // Get target Code object.
   // Remove the stub frame.
   __ LeaveStubFrame();
   // Jump to the dart function.

runtime/vm/compiler/stub_code_compiler_ia32.cc

@@ -515,13 +515,13 @@ void StubCodeCompiler::GenerateFixCallersTargetStub(Assembler* assembler) {
   __ Bind(&monomorphic);
   // This was a switchable call.
   __ EnterStubFrame();
-  __ pushl(ECX);           // Preserve cache (guarded CID as Smi).
-  __ pushl(EBX);           // Preserve receiver.
   __ pushl(Immediate(0));  // Result slot.
-  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 0);
-  __ popl(CODE_REG);  // Get Code object.
-  __ popl(EBX);       // Restore receiver.
+  __ pushl(EBX);           // Preserve receiver.
+  __ pushl(ECX);           // Preserve cache (guarded CID as Smi).
+  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 2);
   __ popl(ECX);       // Restore cache (guarded CID as Smi).
+  __ popl(EBX);       // Restore receiver.
+  __ popl(CODE_REG);  // Get target Code object.
   __ movl(EAX, FieldAddress(CODE_REG, target::Code::entry_point_offset(
                                           CodeEntryKind::kMonomorphic)));
   __ LeaveFrame();

runtime/vm/compiler/stub_code_compiler_x64.cc

@@ -721,13 +721,13 @@ void StubCodeCompiler::GenerateFixCallersTargetStub(Assembler* assembler) {
   __ movq(CODE_REG,
           Address(THR, target::Thread::fix_callers_target_code_offset()));
   __ EnterStubFrame();
-  __ pushq(RBX);           // Preserve cache (guarded CID as Smi).
-  __ pushq(RDX);           // Preserve receiver.
   __ pushq(Immediate(0));  // Result slot.
-  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 0);
-  __ popq(CODE_REG);  // Get Code object.
+  __ pushq(RDX);           // Preserve receiver.
+  __ pushq(RBX);           // Preserve cache.
+  __ CallRuntime(kFixCallersTargetMonomorphicRuntimeEntry, 2);
+  __ popq(RBX);       // Restore cache.
   __ popq(RDX);       // Restore receiver.
-  __ popq(RBX);       // Restore cache (guarded CID as Smi).
+  __ popq(CODE_REG);  // Get target Code object.
   __ movq(RAX, FieldAddress(CODE_REG, target::Code::entry_point_offset(
                                           CodeEntryKind::kMonomorphic)));
   __ LeaveStubFrame();

runtime/vm/runtime_entry.cc

@@ -1613,6 +1613,7 @@ static UnlinkedCallPtr LoadUnlinkedCall(Zone* zone,
 enum class MissHandler {
   kInlineCacheMiss,
   kSwitchableCallMiss,
+  kFixCallersTargetMonomorphic,
 };
 
 // Handles updating of type feedback and possible patching of instance calls.
@@ -2094,7 +2095,8 @@ void PatchableCallHandler::UpdateICDataWithTarget(
   // If we instead only insert a new ICData entry and will return to the IC stub
   // which will call the target, the stub will take care of the increment.
   const bool call_target_directly =
-      miss_handler_ == MissHandler::kInlineCacheMiss;
+      miss_handler_ == MissHandler::kInlineCacheMiss ||
+      miss_handler_ == MissHandler::kFixCallersTargetMonomorphic;
   const intptr_t invocation_count = call_target_directly ? 1 : 0;
 
   if (caller_arguments_.length() == 1) {
@@ -2132,12 +2134,21 @@ void PatchableCallHandler::ReturnJIT(const Code& stub,
                                      const Function& target) {
   // In JIT we can have two different miss handlers to which we return slightly
   // differently.
-  if (miss_handler_ == MissHandler::kSwitchableCallMiss) {
-    arguments_.SetArgAt(0, stub);  // Second return value.
-    arguments_.SetReturn(data);
-  } else {
-    ASSERT(miss_handler_ == MissHandler::kInlineCacheMiss);
-    arguments_.SetReturn(target);
+  switch (miss_handler_) {
+    case MissHandler::kSwitchableCallMiss: {
+      arguments_.SetArgAt(0, stub);  // Second return value.
+      arguments_.SetReturn(data);
+      break;
+    }
+    case MissHandler::kFixCallersTargetMonomorphic: {
+      const auto& new_code = Code::Handle(zone_, target.EnsureHasCode());
+      arguments_.SetReturn(new_code);
+      break;
+    }
+    case MissHandler::kInlineCacheMiss: {
+      arguments_.SetReturn(target);
+      break;
+    }
   }
 }
 
@@ -2964,49 +2975,25 @@ DEFINE_RUNTIME_ENTRY(FixCallersTarget, 0) {
 
 // The caller must be a monomorphic call from unoptimized code.
 // Patch call to point to new target.
-DEFINE_RUNTIME_ENTRY(FixCallersTargetMonomorphic, 0) {
+DEFINE_RUNTIME_ENTRY(FixCallersTargetMonomorphic, 2) {
 #if !defined(DART_PRECOMPILED_RUNTIME)
-  StackFrameIterator iterator(ValidationPolicy::kDontValidateFrames, thread,
-                              StackFrameIterator::kNoCrossThreadIteration);
-  StackFrame* frame = iterator.NextFrame();
-  ASSERT(frame != NULL);
-  while (frame->IsStubFrame() || frame->IsExitFrame()) {
-    frame = iterator.NextFrame();
-    ASSERT(frame != NULL);
-  }
-  if (frame->IsEntryFrame()) {
-    // Since function's current code is always unpatched, the entry frame always
-    // calls to unpatched code.
-    UNREACHABLE();
-  }
-  ASSERT(frame->IsDartFrame());
-  const Code& caller_code = Code::Handle(zone, frame->LookupDartCode());
-  RELEASE_ASSERT(!caller_code.is_optimized());
+  const Instance& receiver = Instance::CheckedHandle(zone, arguments.ArgAt(0));
+  const Array& switchable_call_data =
+      Array::CheckedHandle(zone, arguments.ArgAt(1));
 
-  Object& cache = Object::Handle(zone);
-  const Code& old_target_code = Code::Handle(
-      zone, CodePatcher::GetInstanceCallAt(frame->pc(), caller_code, &cache));
-  const Function& target_function =
-      Function::Handle(zone, old_target_code.function());
-  const Code& current_target_code =
-      Code::Handle(zone, target_function.EnsureHasCode());
-  CodePatcher::PatchInstanceCallAt(frame->pc(), caller_code, cache,
-                                   current_target_code);
-  if (FLAG_trace_patching) {
-    OS::PrintErr(
-        "FixCallersTargetMonomorphic: caller %#" Px
-        " "
-        "target '%s' -> %#" Px " (%s)\n",
-        frame->pc(), target_function.ToFullyQualifiedCString(),
-        current_target_code.EntryPoint(),
-        current_target_code.is_optimized() ? "optimized" : "unoptimized");
-  }
-  // With isolate groups enabled, it is possible that the target code
-  // has been deactivated just now(as a result of re-optimizatin for example),
-  // which will result in another run through FixCallersTarget.
-  ASSERT(!current_target_code.IsDisabled() ||
-         IsolateGroup::AreIsolateGroupsEnabled());
-  arguments.SetReturn(current_target_code);
+  DartFrameIterator iterator(thread,
+                             StackFrameIterator::kNoCrossThreadIteration);
+  StackFrame* caller_frame = iterator.NextFrame();
+  const auto& caller_code = Code::Handle(zone, caller_frame->LookupDartCode());
+  const auto& caller_function =
+      Function::Handle(zone, caller_frame->LookupDartFunction());
+
+  GrowableArray<const Instance*> caller_arguments(1);
+  caller_arguments.Add(&receiver);
+  PatchableCallHandler handler(
+      thread, caller_arguments, MissHandler::kFixCallersTargetMonomorphic,
+      arguments, caller_frame, caller_code, caller_function);
+  handler.ResolveSwitchAndReturn(switchable_call_data);
 #else
   UNREACHABLE();
 #endif

sdk/lib/_internal/vm/lib/internal_patch.dart

@@ -193,6 +193,9 @@ void spawnFunction(
 abstract class VMInternalsForTesting {
   // This function can be used by tests to enforce garbage collection.
   static void collectAllGarbage() native "Internal_collectAllGarbage";
+
+  static void deoptimizeFunctionsOnStack()
+      native "Internal_deoptimizeFunctionsOnStack";
 }
 
 @patch