From 606a1e76be516f91a5676aba29326ea5bfbd65c3 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Fri, 2 Jul 2021 12:47:31 +0000 Subject: [PATCH] [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 Reviewed-by: Slava Egorov --- runtime/lib/object.cc | 5 ++ .../vm/dart/isolates/regress_46539_test.dart | 77 +++++++++++++++++ .../dart_2/isolates/regress_46539_test.dart | 77 +++++++++++++++++ runtime/vm/bootstrap_natives.h | 1 + runtime/vm/compiler/stub_code_compiler_arm.cc | 10 +-- .../vm/compiler/stub_code_compiler_arm64.cc | 12 +-- .../vm/compiler/stub_code_compiler_ia32.cc | 10 +-- runtime/vm/compiler/stub_code_compiler_x64.cc | 10 +-- runtime/vm/runtime_entry.cc | 83 ++++++++----------- sdk/lib/_internal/vm/lib/internal_patch.dart | 3 + 10 files changed, 219 insertions(+), 69 deletions(-) create mode 100644 runtime/tests/vm/dart/isolates/regress_46539_test.dart create mode 100644 runtime/tests/vm/dart_2/isolates/regress_46539_test.dart diff --git a/runtime/lib/object.cc b/runtime/lib/object.cc index 129caeb3b9e..14dacfb7c21 100644 --- a/runtime/lib/object.cc +++ b/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, diff --git a/runtime/tests/vm/dart/isolates/regress_46539_test.dart b/runtime/tests/vm/dart/isolates/regress_46539_test.dart new file mode 100644 index 00000000000..b1189d1ab11 --- /dev/null +++ b/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); +} diff --git a/runtime/tests/vm/dart_2/isolates/regress_46539_test.dart b/runtime/tests/vm/dart_2/isolates/regress_46539_test.dart new file mode 100644 index 00000000000..b1189d1ab11 --- /dev/null +++ b/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); +} diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index 7eb345296c8..e9710df6b0c 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/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) \ diff --git a/runtime/vm/compiler/stub_code_compiler_arm.cc b/runtime/vm/compiler/stub_code_compiler_arm.cc index 50f9b49b52b..8b831c34bc4 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm.cc +++ b/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. diff --git a/runtime/vm/compiler/stub_code_compiler_arm64.cc b/runtime/vm/compiler/stub_code_compiler_arm64.cc index 1e36ee5a3fd..4d92ced76cb 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm64.cc +++ b/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. diff --git a/runtime/vm/compiler/stub_code_compiler_ia32.cc b/runtime/vm/compiler/stub_code_compiler_ia32.cc index 16ef27a1aba..03fd79d8635 100644 --- a/runtime/vm/compiler/stub_code_compiler_ia32.cc +++ b/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(); diff --git a/runtime/vm/compiler/stub_code_compiler_x64.cc b/runtime/vm/compiler/stub_code_compiler_x64.cc index e776269ebe2..71c094ce6ff 100644 --- a/runtime/vm/compiler/stub_code_compiler_x64.cc +++ b/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(); diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc index 856d43f244e..c7305ca34fd 100644 --- a/runtime/vm/runtime_entry.cc +++ b/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 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 diff --git a/sdk/lib/_internal/vm/lib/internal_patch.dart b/sdk/lib/_internal/vm/lib/internal_patch.dart index 6cc238823dc..67501fb78f5 100644 --- a/sdk/lib/_internal/vm/lib/internal_patch.dart +++ b/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