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

This is a reland of commit 87810072db

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: I22eb0f7addce387e7773e45c170c3f9050003083
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/282807
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

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
     }
   }

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.

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

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}};