From 4ab36120d7af0234b75f062cb6c4a36b04a4e5e6 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Fri, 4 Dec 2020 23:54:37 +0000 Subject: [PATCH] [vm] Fix finalization of classes when instances are allocated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Classes should be allocate-finalized before instance can be allocated in order to properly update class hierarchy information and invalidate code which was speculatively optimized. TEST=runtime/tests/vm/dart/regress_44342_test.dart Fixes https://github.com/dart-lang/sdk/issues/44342 Change-Id: I52a415f4cf25010eb23be339266a91ae6b7d7261 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175106 Commit-Queue: Alexander Markov Reviewed-by: Régis Crelier Reviewed-by: Ryan Macnak --- runtime/lib/ffi.cc | 2 +- runtime/tests/vm/dart/regress_44342_test.dart | 40 +++++++++++++++++++ .../tests/vm/dart_2/regress_44342_test.dart | 40 +++++++++++++++++++ runtime/tests/vm/vm.status | 1 + runtime/vm/compiler/aot/precompiler.cc | 2 +- .../vm/compiler/frontend/constant_reader.cc | 3 +- runtime/vm/dart_api_impl.cc | 4 +- runtime/vm/object.cc | 8 +++- 8 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 runtime/tests/vm/dart/regress_44342_test.dart create mode 100644 runtime/tests/vm/dart_2/regress_44342_test.dart diff --git a/runtime/lib/ffi.cc b/runtime/lib/ffi.cc index bb85a966989..2144be8e2c8 100644 --- a/runtime/lib/ffi.cc +++ b/runtime/lib/ffi.cc @@ -224,7 +224,7 @@ DEFINE_NATIVE_ENTRY(Ffi_asExternalTypedData, 0, 2) { const auto& typed_data_class = Class::Handle(zone, isolate->class_table()->At(cid)); const auto& error = - Error::Handle(zone, typed_data_class.EnsureIsFinalized(thread)); + Error::Handle(zone, typed_data_class.EnsureIsAllocateFinalized(thread)); if (!error.IsNull()) { Exceptions::PropagateError(error); } diff --git a/runtime/tests/vm/dart/regress_44342_test.dart b/runtime/tests/vm/dart/regress_44342_test.dart new file mode 100644 index 00000000000..98514fc3ab6 --- /dev/null +++ b/runtime/tests/vm/dart/regress_44342_test.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2020, 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=--deterministic --optimization_counter_threshold=100 + +// Verifies that class of a constant participates in CHA decisions. +// Regression test for https://github.com/dart-lang/sdk/issues/44342. + +import 'package:expect/expect.dart'; + +class A { + const A(); + + int foo() => 3; + + @pragma("vm:never-inline") + int bar() => foo(); +} + +class B extends A {} + +class C extends A { + const C(); + + int foo() => 4; +} + +A x = B(); +A y = const C(); + +main() { + for (int i = 0; i < 200; ++i) { + // Optimize A.bar() with inlined A.foo(). + int result = x.bar(); + Expect.equals(3, result); + } + int result = y.bar(); + Expect.equals(4, result); +} diff --git a/runtime/tests/vm/dart_2/regress_44342_test.dart b/runtime/tests/vm/dart_2/regress_44342_test.dart new file mode 100644 index 00000000000..98514fc3ab6 --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_44342_test.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2020, 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=--deterministic --optimization_counter_threshold=100 + +// Verifies that class of a constant participates in CHA decisions. +// Regression test for https://github.com/dart-lang/sdk/issues/44342. + +import 'package:expect/expect.dart'; + +class A { + const A(); + + int foo() => 3; + + @pragma("vm:never-inline") + int bar() => foo(); +} + +class B extends A {} + +class C extends A { + const C(); + + int foo() => 4; +} + +A x = B(); +A y = const C(); + +main() { + for (int i = 0; i < 200; ++i) { + // Optimize A.bar() with inlined A.foo(). + int result = x.bar(); + Expect.equals(3, result); + } + int result = y.bar(); + Expect.equals(4, result); +} diff --git a/runtime/tests/vm/vm.status b/runtime/tests/vm/vm.status index cdc11866a24..802cc8b5845 100644 --- a/runtime/tests/vm/vm.status +++ b/runtime/tests/vm/vm.status @@ -115,6 +115,7 @@ dart_2/data_uri_import_test/badencodeddate: CompileTimeError [ $mode == debug ] cc/CorelibIsolateStartup: SkipByDesign # This is a benchmark that is not informative in debug mode. +cc/SixtyThousandDartClasses: SkipSlow # Finalization of 64K classes is too slow in debug mode. cc/VerifyExplicit_Crash: Crash # Negative tests of VerifiedMemory should crash iff in DEBUG mode. TODO(koda): Improve support for negative tests. cc/VerifyImplicit_Crash: Crash # Negative tests of VerifiedMemory should crash iff in DEBUG mode. TODO(koda): Improve support for negative tests. dart/appjit_cha_deopt_test: Pass, Slow # Quite slow in debug mode, uses --optimization-counter-threshold=100 diff --git a/runtime/vm/compiler/aot/precompiler.cc b/runtime/vm/compiler/aot/precompiler.cc index ea2fc4a5a0a..61cd91d5b7f 100644 --- a/runtime/vm/compiler/aot/precompiler.cc +++ b/runtime/vm/compiler/aot/precompiler.cc @@ -1122,7 +1122,7 @@ void Precompiler::AddInstantiatedClass(const Class& cls) { class_count_++; cls.set_is_allocated(true); - error_ = cls.EnsureIsFinalized(T); + error_ = cls.EnsureIsAllocateFinalized(T); if (!error_.IsNull()) { Jump(error_); } diff --git a/runtime/vm/compiler/frontend/constant_reader.cc b/runtime/vm/compiler/frontend/constant_reader.cc index d6bff45b2d1..6eb0a03f919 100644 --- a/runtime/vm/compiler/frontend/constant_reader.cc +++ b/runtime/vm/compiler/frontend/constant_reader.cc @@ -233,7 +233,8 @@ InstancePtr ConstantReader::ReadConstantInternal(intptr_t constant_offset) { "%s is not loaded yet.", klass.ToCString()); } - const auto& obj = Object::Handle(Z, klass.EnsureIsFinalized(H.thread())); + const auto& obj = + Object::Handle(Z, klass.EnsureIsAllocateFinalized(H.thread())); ASSERT(obj.IsNull()); ASSERT(klass.is_enum_class() || klass.is_const()); instance = Instance::New(klass, Heap::kOld); diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 2b0c369f720..49c32bc9f75 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -4627,7 +4627,7 @@ DART_EXPORT Dart_Handle Dart_Allocate(Dart_Handle type) { return Api::NewError("Precompilation dropped '%s'", cls.ToCString()); } #endif - CHECK_ERROR_HANDLE(cls.EnsureIsFinalized(T)); + CHECK_ERROR_HANDLE(cls.EnsureIsAllocateFinalized(T)); return Api::NewHandle(T, AllocateObject(T, cls)); } @@ -4653,7 +4653,7 @@ Dart_AllocateWithNativeFields(Dart_Handle type, return Api::NewError("Precompilation dropped '%s'", cls.ToCString()); } #endif - CHECK_ERROR_HANDLE(cls.EnsureIsFinalized(T)); + CHECK_ERROR_HANDLE(cls.EnsureIsAllocateFinalized(T)); if (num_native_fields != cls.num_native_fields()) { return Api::NewError( "%s: invalid number of native fields %" Pd " passed in, expected %d", diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 7870499954c..f11ffea86d5 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -4310,6 +4310,10 @@ ErrorPtr Class::EnsureIsAllocateFinalized(Thread* thread) const { UNREACHABLE(); } } + // May be allocate-finalized recursively during EnsureIsFinalized. + if (is_allocate_finalized()) { + return Error::null(); + } error ^= ClassFinalizer::AllocateFinalizeClass(*this); return error.raw(); } @@ -18756,7 +18760,7 @@ bool Instance::IsCallable(Function* function) const { InstancePtr Instance::New(const Class& cls, Heap::Space space) { Thread* thread = Thread::Current(); - if (cls.EnsureIsFinalized(thread) != Error::null()) { + if (cls.EnsureIsAllocateFinalized(thread) != Error::null()) { return Instance::null(); } intptr_t instance_size = cls.host_instance_size(); @@ -23912,7 +23916,7 @@ PointerPtr Pointer::New(const AbstractType& type_arg, const Class& cls = Class::Handle(Isolate::Current()->class_table()->At(kFfiPointerCid)); - cls.EnsureIsFinalized(Thread::Current()); + cls.EnsureIsAllocateFinalized(Thread::Current()); Pointer& result = Pointer::Handle(zone); result ^= Object::Allocate(kFfiPointerCid, Pointer::InstanceSize(), space);