From 6b04cac19d697666bfe9fb76dcbd6b6cf4093cde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=96=B9=E5=93=B2=E6=BA=90?= Date: Wed, 6 Jul 2022 11:04:10 +0000 Subject: [PATCH] [vm] Bug fix in dedup optimization when build deferred components. Closes https://github.com/dart-lang/sdk/pull/49393 TEST=vm/dart{,_2}/regress_49372 Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-product-x64-try GitOrigin-RevId: c6306f96ae2bafa25c45e5c7a818c71eb72d8d12 Bug: https://github.com/dart-lang/sdk/issues/49372 Change-Id: I7ec4b170819321cb74037e475006890259446744 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/250720 Commit-Queue: Slava Egorov Reviewed-by: Slava Egorov --- .../tests/vm/dart/regress_49372_deferred.dart | 12 +++++ runtime/tests/vm/dart/regress_49372_test.dart | 21 ++++++++ .../vm/dart_2/regress_49372_deferred.dart | 14 +++++ .../tests/vm/dart_2/regress_49372_test.dart | 23 ++++++++ .../compiler/backend/flow_graph_compiler.cc | 40 +++----------- runtime/vm/object.cc | 54 +++++++++++++++++++ runtime/vm/object.h | 3 ++ runtime/vm/program_visitor.cc | 6 ++- runtime/vm/reusable_handles.h | 2 + runtime/vm/thread.h | 1 + 10 files changed, 141 insertions(+), 35 deletions(-) create mode 100644 runtime/tests/vm/dart/regress_49372_deferred.dart create mode 100644 runtime/tests/vm/dart/regress_49372_test.dart create mode 100644 runtime/tests/vm/dart_2/regress_49372_deferred.dart create mode 100644 runtime/tests/vm/dart_2/regress_49372_test.dart diff --git a/runtime/tests/vm/dart/regress_49372_deferred.dart b/runtime/tests/vm/dart/regress_49372_deferred.dart new file mode 100644 index 00000000000..f466fe2ebf4 --- /dev/null +++ b/runtime/tests/vm/dart/regress_49372_deferred.dart @@ -0,0 +1,12 @@ +// Copyright (c) 2022, 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. + +// Regression test for https://github.com/dart-lang/sdk/issues/49372. +// Verifies that dedup optimization doesn't merge Code from different unit. + +@pragma('vm:never-inline') +int foo() => 10; + +@pragma('vm:never-inline') +int bar() => foo() + 2; \ No newline at end of file diff --git a/runtime/tests/vm/dart/regress_49372_test.dart b/runtime/tests/vm/dart/regress_49372_test.dart new file mode 100644 index 00000000000..7453b2ba27d --- /dev/null +++ b/runtime/tests/vm/dart/regress_49372_test.dart @@ -0,0 +1,21 @@ +// Copyright (c) 2022, 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. + +// Regression test for https://github.com/dart-lang/sdk/issues/49372. +// Verifies that dedup optimization doesn't merge Code from different unit. + +import 'package:expect/expect.dart'; +import 'regress_49372_deferred.dart' deferred as lib; + +@pragma('vm:never-inline') +int foo() => 10; + +@pragma('vm:never-inline') +int bar() => foo() + 7; + +void main() async { + await lib.loadLibrary(); + Expect.equals(12, lib.bar()); + Expect.equals(17, bar()); +} diff --git a/runtime/tests/vm/dart_2/regress_49372_deferred.dart b/runtime/tests/vm/dart_2/regress_49372_deferred.dart new file mode 100644 index 00000000000..5c5ce0fb27d --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_49372_deferred.dart @@ -0,0 +1,14 @@ +// Copyright (c) 2022, 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. + +// Regression test for https://github.com/dart-lang/sdk/issues/49372. +// Verifies that dedup optimization doesn't merge Code from different unit. + +// @dart = 2.9 + +@pragma('vm:never-inline') +int foo() => 10; + +@pragma('vm:never-inline') +int bar() => foo() + 2; \ No newline at end of file diff --git a/runtime/tests/vm/dart_2/regress_49372_test.dart b/runtime/tests/vm/dart_2/regress_49372_test.dart new file mode 100644 index 00000000000..7c7990c7054 --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_49372_test.dart @@ -0,0 +1,23 @@ +// Copyright (c) 2022, 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. + +// Regression test for https://github.com/dart-lang/sdk/issues/49372. +// Verifies that dedup optimization doesn't merge Code from different unit. + +// @dart = 2.9 + +import 'package:expect/expect.dart'; +import 'regress_49372_deferred.dart' deferred as lib; + +@pragma('vm:never-inline') +int foo() => 10; + +@pragma('vm:never-inline') +int bar() => foo() + 7; + +void main() async { + await lib.loadLibrary(); + Expect.equals(12, lib.bar()); + Expect.equals(17, bar()); +} diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.cc b/runtime/vm/compiler/backend/flow_graph_compiler.cc index 32c36acc1f0..6355ea91feb 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler.cc @@ -3606,49 +3606,21 @@ void FlowGraphCompiler::EmitMoveFromNative( } } -// The assignment to loading units here must match that in -// AssignLoadingUnitsCodeVisitor, which runs after compilation is done. -static intptr_t LoadingUnitOf(Zone* zone, const Function& function) { - const Class& cls = Class::Handle(zone, function.Owner()); - const Library& lib = Library::Handle(zone, cls.library()); - const LoadingUnit& unit = LoadingUnit::Handle(zone, lib.loading_unit()); - ASSERT(!unit.IsNull()); - return unit.id(); -} - -static intptr_t LoadingUnitOf(Zone* zone, const Code& code) { - // No WeakSerializationReference owners here because those are only - // introduced during AOT serialization. - if (code.IsStubCode() || code.IsTypeTestStubCode()) { - return LoadingUnit::kRootId; - } else if (code.IsAllocationStubCode()) { - const Class& cls = Class::Cast(Object::Handle(zone, code.owner())); - const Library& lib = Library::Handle(zone, cls.library()); - const LoadingUnit& unit = LoadingUnit::Handle(zone, lib.loading_unit()); - ASSERT(!unit.IsNull()); - return unit.id(); - } else if (code.IsFunctionCode()) { - return LoadingUnitOf(zone, - Function::Cast(Object::Handle(zone, code.owner()))); - } else { - UNREACHABLE(); - return LoadingUnit::kIllegalId; - } -} - bool FlowGraphCompiler::CanPcRelativeCall(const Function& target) const { - return FLAG_precompiled_mode && - (LoadingUnitOf(zone_, function()) == LoadingUnitOf(zone_, target)); + return FLAG_precompiled_mode && (LoadingUnit::LoadingUnitOf(function()) == + LoadingUnit::LoadingUnitOf(target)); } bool FlowGraphCompiler::CanPcRelativeCall(const Code& target) const { return FLAG_precompiled_mode && !target.InVMIsolateHeap() && - (LoadingUnitOf(zone_, function()) == LoadingUnitOf(zone_, target)); + (LoadingUnit::LoadingUnitOf(function()) == + LoadingUnit::LoadingUnitOf(target)); } bool FlowGraphCompiler::CanPcRelativeCall(const AbstractType& target) const { return FLAG_precompiled_mode && !target.InVMIsolateHeap() && - (LoadingUnitOf(zone_, function()) == LoadingUnit::kRootId); + (LoadingUnit::LoadingUnitOf(function()) == + LoadingUnit::LoadingUnit::kRootId); } #undef __ diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 3cc799fa689..bdd1af2bf72 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -18602,6 +18602,60 @@ ObjectPtr LoadingUnit::CompleteLoad(const String& error_message, return DartEntry::InvokeFunction(func, args); } +// The assignment to loading units here must match that in +// AssignLoadingUnitsCodeVisitor, which runs after compilation is done. +intptr_t LoadingUnit::LoadingUnitOf(const Function& function) { + Thread* thread = Thread::Current(); + REUSABLE_CLASS_HANDLESCOPE(thread); + REUSABLE_LIBRARY_HANDLESCOPE(thread); + REUSABLE_LOADING_UNIT_HANDLESCOPE(thread); + + Class& cls = thread->ClassHandle(); + Library& lib = thread->LibraryHandle(); + LoadingUnit& unit = thread->LoadingUnitHandle(); + + cls = function.Owner(); + lib = cls.library(); + unit = lib.loading_unit(); + ASSERT(!unit.IsNull()); + return unit.id(); +} + +intptr_t LoadingUnit::LoadingUnitOf(const Code& code) { + if (code.IsStubCode() || code.IsTypeTestStubCode()) { + return LoadingUnit::kRootId; + } else { + Thread* thread = Thread::Current(); + REUSABLE_FUNCTION_HANDLESCOPE(thread); + REUSABLE_CLASS_HANDLESCOPE(thread); + REUSABLE_LIBRARY_HANDLESCOPE(thread); + REUSABLE_LOADING_UNIT_HANDLESCOPE(thread); + + Class& cls = thread->ClassHandle(); + Library& lib = thread->LibraryHandle(); + LoadingUnit& unit = thread->LoadingUnitHandle(); + Function& func = thread->FunctionHandle(); + + if (code.IsAllocationStubCode()) { + cls ^= code.owner(); + lib = cls.library(); + unit = lib.loading_unit(); + ASSERT(!unit.IsNull()); + return unit.id(); + } else if (code.IsFunctionCode()) { + func ^= code.function(); + cls = func.Owner(); + lib = cls.library(); + unit = lib.loading_unit(); + ASSERT(!unit.IsNull()); + return unit.id(); + } else { + UNREACHABLE(); + return LoadingUnit::kIllegalId; + } + } +} + const char* Error::ToErrorCString() const { if (IsNull()) { return "Error: null"; diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 0ed84a62b6e..ab528c39754 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -7432,6 +7432,9 @@ class LoadingUnit : public Object { return RoundedAllocationSize(sizeof(UntaggedLoadingUnit)); } + static intptr_t LoadingUnitOf(const Function& function); + static intptr_t LoadingUnitOf(const Code& code); + LoadingUnitPtr parent() const; void set_parent(const LoadingUnit& value) const; diff --git a/runtime/vm/program_visitor.cc b/runtime/vm/program_visitor.cc index b87fa96f761..7c195a95a1f 100644 --- a/runtime/vm/program_visitor.cc +++ b/runtime/vm/program_visitor.cc @@ -1214,7 +1214,11 @@ class CodeKeyValueTrait { if (pair->UncheckedEntryPointOffset() != key->UncheckedEntryPointOffset()) { return false; } - return Instructions::Equals(pair->instructions(), key->instructions()); + if (!Instructions::Equals(pair->instructions(), key->instructions())) { + return false; + } + return LoadingUnit::LoadingUnitOf(*pair) == + LoadingUnit::LoadingUnitOf(*key); } }; #endif diff --git a/runtime/vm/reusable_handles.h b/runtime/vm/reusable_handles.h index 5fcb2f7bf51..c13955276c6 100644 --- a/runtime/vm/reusable_handles.h +++ b/runtime/vm/reusable_handles.h @@ -96,6 +96,8 @@ REUSABLE_HANDLE_LIST(REUSABLE_SCOPE) ReusableInstanceHandleScope reused_instance_handle(thread); #define REUSABLE_LIBRARY_HANDLESCOPE(thread) \ ReusableLibraryHandleScope reused_library_handle(thread); +#define REUSABLE_LOADING_UNIT_HANDLESCOPE(thread) \ + ReusableLoadingUnitHandleScope reused_loading_unit_handle(thread); #define REUSABLE_OBJECT_HANDLESCOPE(thread) \ ReusableObjectHandleScope reused_object_handle(thread); #define REUSABLE_PC_DESCRIPTORS_HANDLESCOPE(thread) \ diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index 5f9660d1f50..e0bc2a0dd21 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -84,6 +84,7 @@ class Thread; V(GrowableObjectArray) \ V(Instance) \ V(Library) \ + V(LoadingUnit) \ V(Object) \ V(PcDescriptors) \ V(Smi) \