Revert "[vm] Account for changes in the implicit getters due to load guards"

This reverts commit 8781007.

Reason for revert: breaks hot reload test case in Flutter,
reverting as temporary workaround to flutter/flutter#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 #51215
> Fixes flutter/flutter#103268
>
> Change-Id: I1afbeabb4a60b7a5f69ed055f40beeb6d3dcf359
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280401
> Reviewed-by: Martin Kustermann <[email protected]>
> Commit-Queue: Alexander Markov <[email protected]>
> Reviewed-by: Ryan Macnak <[email protected]>

# 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 <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Zach Anderson <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>

Author: alexmarkov

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -3862,44 +3862,44 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFieldAccessor(
       }
     }
     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);
+  } 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
     }
-
-    if (is_method || !field.is_const()) {
+  } 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)
-      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;
-      }
+      ASSERT(IsolateGroup::Current()->HasAttemptedReload());
+      body += CheckAssignable(AbstractType::Handle(Z, field.type()),
+                              Symbols::FunctionResult());
 #endif
     }
   }

runtime/vm/isolate_reload.cc

@@ -2005,11 +2005,8 @@ void ProgramReloadContext::RunInvalidationVisitors() {
 
   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 @@ 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];
@@ -2066,20 +2062,9 @@ 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_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 @@ void ProgramReloadContext::InvalidateFunctions(
 
     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.

runtime/vm/isolate_reload_test.cc

@@ -5007,63 +5007,6 @@ 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

runtime/vm/unit_test.cc

@@ -284,18 +284,12 @@ void FUNCTION_NAME(Test_CollectOldSpace)(Dart_NativeArguments native_args) {
   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 @@ Dart_Handle TestCase::LoadTestScript(const char* script,
                                      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 @@ 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}};