Revert "[vm] Account for changes in the implicit getters due to load guards"
This reverts commit 87810072db06c1a16fdaa7bf6bb77ef4e6405af5.
Reason for revert: breaks hot reload test case in Flutter,
reverting as temporary workaround to https://github.com/flutter/flutter/issues/120091.
Original change's description:
> [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 <kustermann@google.com>
> Commit-Queue: Alexander Markov <alexmarkov@google.com>
> Reviewed-by: Ryan Macnak <rmacnak@google.com>
# Not skipping CQ checks because original CL landed > 1 day ago.
Change-Id: Ia7dba382547cceb38524707f9ecef892016108de
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281220
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc
index f685e53..052bb73 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -3862,44 +3862,44 @@
}
}
body += NullConstant();
- } else {
- 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()) {
+ } 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)
- RELEASE_ASSERT(!field.needs_load_guard());
+ UNREACHABLE();
#else
- // 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;
- }
+ 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()) {
+#if defined(PRODUCT)
+ UNREACHABLE();
+#else
+ ASSERT(IsolateGroup::Current()->HasAttemptedReload());
+ body += CheckAssignable(AbstractType::Handle(Z, field.type()),
+ Symbols::FunctionResult());
#endif
}
}
diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc
index 43e1de9..f902c84 100644
--- a/runtime/vm/isolate_reload.cc
+++ b/runtime/vm/isolate_reload.cc
@@ -2005,11 +2005,8 @@
InvalidateKernelInfos(zone, kernel_infos);
InvalidateSuspendStates(zone, suspend_states);
- InvalidateFields(zone, fields, instances);
-
- // After InvalidateFields in order to invalidate
- // implicit getters which need load guards.
InvalidateFunctions(zone, functions);
+ InvalidateFields(zone, fields, instances);
}
void ProgramReloadContext::InvalidateKernelInfos(
@@ -2054,7 +2051,6 @@
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];
@@ -2066,20 +2062,9 @@
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_unoptimized_code =
- IsDirty(owning_lib) || recompile_for_load_guard;
+ const bool clear_code = IsDirty(owning_lib);
const bool stub_code = code.IsStubCode();
// Zero edge counters, before clearing the ICDataArray, since that's where
@@ -2088,7 +2073,7 @@
if (stub_code) {
// Nothing to reset.
- } else if (clear_unoptimized_code) {
+ } else if (clear_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 e09d4d2..91f6a64 100644
--- a/runtime/vm/isolate_reload_test.cc
+++ b/runtime/vm/isolate_reload_test.cc
@@ -5007,63 +5007,6 @@
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 2b1e257..57286f7 100644
--- a/runtime/vm/unit_test.cc
+++ b/runtime/vm/unit_test.cc
@@ -284,18 +284,12 @@
GCTestHelper::CollectOldSpace();
}
-#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
+static Dart_Handle LoadIsolateReloadTestLib() {
+ return TestCase::LoadTestLibrary(IsolateReloadTestLibUri(),
+ kIsolateReloadTestLibSource,
+ IsolateReloadTestNativeResolver);
}
+#endif // !PRODUCT
char* TestCase::CompileTestScriptWithDFE(const char* url,
const char* source,
@@ -424,7 +418,12 @@
const char* lib_url,
bool finalize_classes,
bool allow_compile_errors) {
- LoadIsolateReloadTestLibIfNeeded(script);
+#ifndef PRODUCT
+ if (strstr(script, IsolateReloadTestLibUri()) != NULL) {
+ Dart_Handle result = LoadIsolateReloadTestLib();
+ EXPECT_VALID(result);
+ }
+#endif // ifndef PRODUCT
Dart_SourceFile* sourcefiles = NULL;
intptr_t num_sources = BuildSourceFilesArray(&sourcefiles, script, lib_url);
Dart_Handle result =
@@ -441,7 +440,6 @@
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}};
