From 87810072db06c1a16fdaa7bf6bb77ef4e6405af5 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Fri, 3 Feb 2023 22:23:22 +0000 Subject: [PATCH] [vm] Account for changes in the implicit getters due to load guards Hot reload may mark certain fields as 'needs_load_guard', meaning types of their values should be checked on access, in the implicit getters. Unoptimized code for implicit getters of such fields should be forcefully recompiled during hot reload in order to update their code. This ensures that optimized code in future can deoptimize into the updated unoptimized code of implicit getters. TEST=vm/cc/IsolateReload_ImplicitGetterWithLoadGuard TEST=corelib/string_fromcharcodes_test, lib/convert/json_utf8_chunk_test and vm/dart/string_equals_test on vm-reload-linux-{debug,release}-x64 configurations. Fixes https://github.com/dart-lang/sdk/issues/51215 Fixes https://github.com/flutter/flutter/issues/103268 Change-Id: I1afbeabb4a60b7a5f69ed055f40beeb6d3dcf359 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280401 Reviewed-by: Martin Kustermann Commit-Queue: Alexander Markov Reviewed-by: Ryan Macnak --- runtime/vm/compiler/frontend/kernel_to_il.cc | 70 ++++++++++---------- runtime/vm/isolate_reload.cc | 21 +++++- runtime/vm/isolate_reload_test.cc | 57 ++++++++++++++++ runtime/vm/unit_test.cc | 24 ++++--- 4 files changed, 123 insertions(+), 49 deletions(-) diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 052bb7399e9..f685e530c73 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -3862,44 +3862,44 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFieldAccessor( } } body += NullConstant(); - } else if (is_getter && is_method) { - ASSERT(!field.needs_load_guard() - NOT_IN_PRODUCT(|| IG->HasAttemptedReload())); - body += LoadLocal(parsed_function_->ParameterVariable(0)); - body += LoadField( - field, /*calls_initializer=*/field.NeedsInitializationCheckOnLoad()); - if (field.needs_load_guard()) { -#if defined(PRODUCT) - UNREACHABLE(); -#else - body += CheckAssignable(AbstractType::Handle(Z, field.type()), - Symbols::FunctionResult()); -#endif - } - } else if (field.is_const()) { - const auto& value = Object::Handle(Z, field.StaticConstFieldValue()); - if (value.IsError()) { - Report::LongJump(Error::Cast(value)); - } - body += Constant(Instance::ZoneHandle(Z, Instance::RawCast(value.ptr()))); } else { - // Static fields - // - with trivial initializer - // - without initializer if they are not late - // are initialized eagerly and do not have implicit getters. - // Static fields with non-trivial initializer need getter to perform - // lazy initialization. Late fields without initializer need getter - // to make sure they are already initialized. - ASSERT(field.has_nontrivial_initializer() || - (field.is_late() && !field.has_initializer())); - body += LoadStaticField(field, /*calls_initializer=*/true); - if (field.needs_load_guard()) { + ASSERT(is_getter); + if (is_method) { + body += LoadLocal(parsed_function_->ParameterVariable(0)); + body += LoadField( + field, /*calls_initializer=*/field.NeedsInitializationCheckOnLoad()); + } else if (field.is_const()) { + const auto& value = Object::Handle(Z, field.StaticConstFieldValue()); + if (value.IsError()) { + Report::LongJump(Error::Cast(value)); + } + body += Constant(Instance::ZoneHandle(Z, Instance::RawCast(value.ptr()))); + } else { + // Static fields + // - with trivial initializer + // - without initializer if they are not late + // are initialized eagerly and do not have implicit getters. + // Static fields with non-trivial initializer need getter to perform + // lazy initialization. Late fields without initializer need getter + // to make sure they are already initialized. + ASSERT(field.has_nontrivial_initializer() || + (field.is_late() && !field.has_initializer())); + body += LoadStaticField(field, /*calls_initializer=*/true); + } + + if (is_method || !field.is_const()) { #if defined(PRODUCT) - UNREACHABLE(); + RELEASE_ASSERT(!field.needs_load_guard()); #else - ASSERT(IsolateGroup::Current()->HasAttemptedReload()); - body += CheckAssignable(AbstractType::Handle(Z, field.type()), - Symbols::FunctionResult()); + // Always build fragment for load guard to maintain stable deopt_id + // numbering, but link it into the graph only if field actually + // needs load guard. + Fragment load_guard = CheckAssignable( + AbstractType::Handle(Z, field.type()), Symbols::FunctionResult()); + if (field.needs_load_guard()) { + ASSERT(IG->HasAttemptedReload()); + body += load_guard; + } #endif } } diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc index f902c84584f..43e1de9f201 100644 --- a/runtime/vm/isolate_reload.cc +++ b/runtime/vm/isolate_reload.cc @@ -2005,8 +2005,11 @@ void ProgramReloadContext::RunInvalidationVisitors() { InvalidateKernelInfos(zone, kernel_infos); InvalidateSuspendStates(zone, suspend_states); - InvalidateFunctions(zone, functions); InvalidateFields(zone, fields, instances); + + // After InvalidateFields in order to invalidate + // implicit getters which need load guards. + InvalidateFunctions(zone, functions); } void ProgramReloadContext::InvalidateKernelInfos( @@ -2051,6 +2054,7 @@ void ProgramReloadContext::InvalidateFunctions( Class& owning_class = Class::Handle(zone); Library& owning_lib = Library::Handle(zone); Code& code = Code::Handle(zone); + Field& field = Field::Handle(zone); SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock()); for (intptr_t i = 0; i < functions.length(); i++) { const Function& func = *functions[i]; @@ -2062,9 +2066,20 @@ void ProgramReloadContext::InvalidateFunctions( code = func.CurrentCode(); ASSERT(!code.IsNull()); + // Force recompilation of unoptimized code of implicit getters + // in order to add load guards. This is needed for future + // deoptimizations which will expect load guard in the unoptimized code. + bool recompile_for_load_guard = false; + if (func.IsImplicitGetterFunction() || + func.IsImplicitStaticGetterFunction()) { + field = func.accessor_field(); + recompile_for_load_guard = field.needs_load_guard(); + } + owning_class = func.Owner(); owning_lib = owning_class.library(); - const bool clear_code = IsDirty(owning_lib); + const bool clear_unoptimized_code = + IsDirty(owning_lib) || recompile_for_load_guard; const bool stub_code = code.IsStubCode(); // Zero edge counters, before clearing the ICDataArray, since that's where @@ -2073,7 +2088,7 @@ void ProgramReloadContext::InvalidateFunctions( if (stub_code) { // Nothing to reset. - } else if (clear_code) { + } else if (clear_unoptimized_code) { VTIR_Print("Marking %s for recompilation, clearing code\n", func.ToCString()); // Null out the ICData array and code. diff --git a/runtime/vm/isolate_reload_test.cc b/runtime/vm/isolate_reload_test.cc index 91f6a64bdd4..e09d4d2372b 100644 --- a/runtime/vm/isolate_reload_test.cc +++ b/runtime/vm/isolate_reload_test.cc @@ -5007,6 +5007,63 @@ TEST_CASE(IsolateReload_GenericConstructorTearOff) { EXPECT_STREQ("okay", SimpleInvokeStr(lib, "main")); } +// Regression test for https://github.com/dart-lang/sdk/issues/51215. +TEST_CASE(IsolateReload_ImplicitGetterWithLoadGuard) { + const char* kLibScript = R"( + import 'file:///test:isolate_reload_helper'; + + class A { + int x; + A(this.x); + A.withUinitializedObject(int Function() callback) : x = callback(); + } + + A a = A(3); + + main() { + int sum = 0; + // Trigger OSR and optimize this function. + for (int i = 0; i < 30000; ++i) { + sum += i; + } + // Make sure A.get:x is compiled. + int y = a.x; + // Reload while having an uninitialized + // object A on the stack. This should result in + // a load guard for A.x. + A.withUinitializedObject(() { + reloadTest(); + return 4; + }); + // Trigger OSR and optimize this function once again. + for (int i = 0; i < 30000; ++i) { + sum += i; + } + // Trigger deoptimization in A.get:x. + // It should correctly deoptimize into an implicit + // getter with a load guard. + a.x = 0x8070605040302010; + int z = a.x & 0xffff; + return "y: $y, z: $z"; + } + )"; + + Dart_Handle lib1 = + TestCase::LoadTestLibrary("test_lib1.dart", kLibScript, nullptr); + EXPECT_VALID(lib1); + + const char* kMainScript = R"( + main() {} + )"; + + // Trigger hot reload during execution of 'main' from test_lib1 + // without reloading test_lib1, so its unoptimized code is retained. + EXPECT_VALID(TestCase::LoadTestScript(kMainScript, nullptr)); + EXPECT_VALID(TestCase::SetReloadTestScript(kMainScript)); + + EXPECT_STREQ("y: 3, z: 8208", SimpleInvokeStr(lib1, "main")); +} + #endif // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME) } // namespace dart diff --git a/runtime/vm/unit_test.cc b/runtime/vm/unit_test.cc index 57286f754cd..2b1e2578c40 100644 --- a/runtime/vm/unit_test.cc +++ b/runtime/vm/unit_test.cc @@ -284,13 +284,19 @@ void FUNCTION_NAME(Test_CollectOldSpace)(Dart_NativeArguments native_args) { GCTestHelper::CollectOldSpace(); } -static Dart_Handle LoadIsolateReloadTestLib() { - return TestCase::LoadTestLibrary(IsolateReloadTestLibUri(), - kIsolateReloadTestLibSource, - IsolateReloadTestNativeResolver); -} #endif // !PRODUCT +static void LoadIsolateReloadTestLibIfNeeded(const char* script) { +#ifndef PRODUCT + if (strstr(script, IsolateReloadTestLibUri()) != nullptr) { + Dart_Handle result = TestCase::LoadTestLibrary( + IsolateReloadTestLibUri(), kIsolateReloadTestLibSource, + IsolateReloadTestNativeResolver); + EXPECT_VALID(result); + } +#endif // ifndef PRODUCT +} + char* TestCase::CompileTestScriptWithDFE(const char* url, const char* source, const uint8_t** kernel_buffer, @@ -418,12 +424,7 @@ Dart_Handle TestCase::LoadTestScript(const char* script, const char* lib_url, bool finalize_classes, bool allow_compile_errors) { -#ifndef PRODUCT - if (strstr(script, IsolateReloadTestLibUri()) != NULL) { - Dart_Handle result = LoadIsolateReloadTestLib(); - EXPECT_VALID(result); - } -#endif // ifndef PRODUCT + LoadIsolateReloadTestLibIfNeeded(script); Dart_SourceFile* sourcefiles = NULL; intptr_t num_sources = BuildSourceFilesArray(&sourcefiles, script, lib_url); Dart_Handle result = @@ -440,6 +441,7 @@ static void MallocFinalizer(void* isolate_callback_data, void* peer) { Dart_Handle TestCase::LoadTestLibrary(const char* lib_uri, const char* script, Dart_NativeEntryResolver resolver) { + LoadIsolateReloadTestLibIfNeeded(script); const char* prefixed_lib_uri = OS::SCreate(Thread::Current()->zone(), "file:///%s", lib_uri); Dart_SourceFile sourcefiles[] = {{prefixed_lib_uri, script}};