[vm/shared] Fix LoadStaticField calls initializer behavior.

With b32e5e5a91 under --experimental_shared_data flag, `LoadStaticField` calls initializer stub to confirm presence of isolate when accessing non-shared static fields. This breaks the role that `calls_initializer` attribute/parameter used to play where it was specifically describing neccessity to call initializer, which is important in how `LoadStaticField` instruction is used in static field setters - it should not attempt to call initialzer.

This CL fixes the problem by renaming current `calls_initializer` `LoadStaticFieldInstr` attribute to `does_slow_checks`, and "adding" separate `calls_initializer` parameter to the instruction to signify specifically whether initializer has to be called.

TEST=ci

Change-Id: Ib81424864c98d889fb6dbb9dd15dac9a39c530f8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429062
Reviewed-by: Slava Egorov <vegorov@google.com>

runtime/vm/compiler/backend/constant_propagator_test.cc

@@ -307,8 +307,8 @@ void StrictCompareSentinel(Thread* thread,
   {
     BlockBuilder builder(H.flow_graph(), b1);
     auto v_load = builder.AddDefinition(new LoadStaticFieldInstr(
-        field_x, {},
-        /*calls_initializer=*/true, S.GetNextDeoptId()));
+        field_x, {}, SlowPathOnSentinelValue::kCallInitializer,
+        S.GetNextDeoptId()));
     auto v_sentinel = H.flow_graph()->GetConstant(Object::sentinel());
     Value* const left_value =
         non_sentinel_on_left ? new Value(v_load) : new Value(v_sentinel);

runtime/vm/compiler/backend/il.cc

@@ -4504,27 +4504,28 @@ LocationSummary* LoadStaticFieldInstr::MakeLocationSummary(Zone* zone,
                                                            bool opt) const {
   const intptr_t kNumInputs = 0;
   const bool use_shared_stub = UseSharedSlowPathStub(opt);
-  const intptr_t kNumTemps = calls_initializer() &&
+  const intptr_t kNumTemps = does_throw_access_error_or_call_initializer() &&
                                      throw_exception_on_initialization() &&
                                      use_shared_stub
                                  ? 1
                                  : 0;
   LocationSummary* locs = new (zone) LocationSummary(
       zone, kNumInputs, kNumTemps,
-      calls_initializer()
+      does_throw_access_error_or_call_initializer()
           ? (throw_exception_on_initialization()
                  ? (use_shared_stub ? LocationSummary::kCallOnSharedSlowPath
                                     : LocationSummary::kCallOnSlowPath)
                  : LocationSummary::kCall)
           : LocationSummary::kNoCall);
-  if (calls_initializer() && throw_exception_on_initialization() &&
-      use_shared_stub) {
+  if (does_throw_access_error_or_call_initializer() &&
+      throw_exception_on_initialization() && use_shared_stub) {
     locs->set_temp(
         0, Location::RegisterLocation(LateInitializationErrorABI::kFieldReg));
   }
-  locs->set_out(0, calls_initializer() ? Location::RegisterLocation(
-                                             InitStaticFieldABI::kResultReg)
-                                       : Location::RequiresRegister());
+  locs->set_out(0,
+                does_throw_access_error_or_call_initializer()
+                    ? Location::RegisterLocation(InitStaticFieldABI::kResultReg)
+                    : Location::RequiresRegister());
   return locs;
 }
 
@@ -4543,8 +4544,8 @@ void LoadStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   __ LoadMemoryValue(result, THR, static_cast<int32_t>(field_table_offset));
   __ LoadMemoryValue(result, result, static_cast<int32_t>(field_offset));
 
-  if (calls_initializer()) {
-    if (throw_exception_on_initialization()) {
+  if (does_throw_access_error_or_call_initializer()) {
+    if (calls_initializer() && throw_exception_on_initialization()) {
       ThrowErrorSlowPathCode* slow_path =
           new LateInitializationErrorSlowPath(this);
       compiler->AddSlowPathCode(slow_path);
@@ -4563,22 +4564,27 @@ void LoadStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
     __ BranchIf(NOT_EQUAL, &no_call);
 
     auto& stub = Code::ZoneHandle(compiler->zone());
-    if (field().needs_load_guard()) {
-      stub = object_store->init_static_field_stub();
-    } else {
-      // The stubs below call the initializer function directly, so make sure
-      // one is created.
-      if (original_field.has_nontrivial_initializer()) {
-        original_field.EnsureInitializerFunction();
+    if (calls_initializer()) {
+      if (field().needs_load_guard()) {
+        stub = object_store->init_static_field_stub();
+      } else {
+        // The stubs below call the initializer function directly, so make sure
+        // one is created.
+        if (original_field.has_nontrivial_initializer()) {
+          original_field.EnsureInitializerFunction();
+        }
+        stub = field().is_shared()
+                   ? (field().is_final()
+                          ? object_store
+                                ->init_shared_late_final_static_field_stub()
+                          : object_store->init_shared_late_static_field_stub())
+                   : (field().is_final()
+                          ? object_store->init_late_final_static_field_stub()
+                          : object_store->init_late_static_field_stub());
       }
-      stub =
-          field().is_shared()
-              ? (field().is_final()
-                     ? object_store->init_shared_late_final_static_field_stub()
-                     : object_store->init_shared_late_static_field_stub())
-              : (field().is_final()
-                     ? object_store->init_late_final_static_field_stub()
-                     : object_store->init_late_static_field_stub());
+    } else {
+      ASSERT(FLAG_experimental_shared_data && !field().is_shared());
+      stub = object_store->check_isolate_field_access_stub();
     }
 
     __ LoadObject(InitStaticFieldABI::kFieldReg, original_field);

runtime/vm/compiler/backend/il.h

@@ -6602,27 +6602,47 @@ class GuardFieldTypeInstr : public GuardFieldInstr {
   DISALLOW_COPY_AND_ASSIGN(GuardFieldTypeInstr);
 };
 
+enum class SlowPathOnSentinelValue {
+  kDoNothing,
+  kThrowAccessError,  // This is part of shared field implementation.
+  kCallInitializer,   // This will also do the shared field access check.
+};
+
 template <intptr_t N>
 class TemplateLoadField : public TemplateDefinition<N, Throws> {
   using Base = TemplateDefinition<N, Throws>;
 
  public:
-  TemplateLoadField(const InstructionSource& source,
-                    bool calls_initializer = false,
-                    intptr_t deopt_id = DeoptId::kNone,
-                    const Field* field = nullptr)
+  TemplateLoadField(
+      const InstructionSource& source,
+      SlowPathOnSentinelValue slow_path = SlowPathOnSentinelValue::kDoNothing,
+      intptr_t deopt_id = DeoptId::kNone,
+      const Field* field = nullptr)
       : Base(source, deopt_id),
         token_pos_(source.token_pos),
         throw_exception_on_initialization_(
             field != nullptr && !field->has_initializer() && field->is_late()),
-        calls_initializer_(calls_initializer) {
-    ASSERT(!calls_initializer || field != nullptr);
-    ASSERT(!calls_initializer || (deopt_id != DeoptId::kNone));
+        slow_path_(slow_path) {
+    ASSERT(slow_path == SlowPathOnSentinelValue::kDoNothing ||
+           field != nullptr);
+    ASSERT(slow_path == SlowPathOnSentinelValue::kDoNothing ||
+           (deopt_id != DeoptId::kNone));
   }
 
   virtual TokenPosition token_pos() const { return token_pos_; }
-  bool calls_initializer() const { return calls_initializer_; }
-  void set_calls_initializer(bool value) { calls_initializer_ = value; }
+
+  bool does_throw_access_error_or_call_initializer() const {
+    return slow_path_ > SlowPathOnSentinelValue::kDoNothing;
+  }
+  bool throws_access_error() const {
+    return slow_path_ == SlowPathOnSentinelValue::kThrowAccessError;
+  }
+  bool calls_initializer() const {
+    return slow_path_ == SlowPathOnSentinelValue::kCallInitializer;
+  }
+  void clear_calls_initializer() {
+    slow_path_ = SlowPathOnSentinelValue::kDoNothing;
+  }
 
   bool throw_exception_on_initialization() const {
     return throw_exception_on_initialization_;
@@ -6636,14 +6656,16 @@ class TemplateLoadField : public TemplateDefinition<N, Throws> {
   virtual intptr_t DeoptimizationTarget() const { return Base::GetDeoptId(); }
   virtual bool ComputeCanDeoptimize() const { return false; }
   virtual bool ComputeCanDeoptimizeAfterCall() const {
-    return calls_initializer() && !CompilerState::Current().is_aot();
+    return does_throw_access_error_or_call_initializer() &&
+           !CompilerState::Current().is_aot();
   }
   virtual intptr_t NumberOfInputsConsumedBeforeCall() const {
     return Base::InputCount();
   }
 
   virtual bool HasUnknownSideEffects() const {
-    return calls_initializer() && !throw_exception_on_initialization();
+    return does_throw_access_error_or_call_initializer() &&
+           !throw_exception_on_initialization();
   }
 
   virtual bool CanCallDart() const {
@@ -6653,13 +6675,17 @@ class TemplateLoadField : public TemplateDefinition<N, Throws> {
     // automatically (see Thread::RestoreWriteBarrierInvariant).
     return false;
   }
-  virtual bool CanTriggerGC() const { return calls_initializer(); }
-  virtual bool MayThrow() const { return calls_initializer(); }
+  virtual bool CanTriggerGC() const {
+    return does_throw_access_error_or_call_initializer();
+  }
+  virtual bool MayThrow() const {
+    return does_throw_access_error_or_call_initializer();
+  }
 
 #define FIELD_LIST(F)                                                          \
   F(const TokenPosition, token_pos_)                                           \
   F(const bool, throw_exception_on_initialization_)                            \
-  F(bool, calls_initializer_)
+  F(SlowPathOnSentinelValue, slow_path_)
 
   DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(TemplateLoadField, Base, FIELD_LIST)
 #undef FIELD_LIST
@@ -6670,11 +6696,12 @@ class TemplateLoadField : public TemplateDefinition<N, Throws> {
 
 class LoadStaticFieldInstr : public TemplateLoadField<0> {
  public:
-  LoadStaticFieldInstr(const Field& field,
-                       const InstructionSource& source,
-                       bool calls_initializer = false,
-                       intptr_t deopt_id = DeoptId::kNone)
-      : TemplateLoadField<0>(source, calls_initializer, deopt_id, &field),
+  LoadStaticFieldInstr(
+      const Field& field,
+      const InstructionSource& source,
+      SlowPathOnSentinelValue slow_path = SlowPathOnSentinelValue::kDoNothing,
+      intptr_t deopt_id = DeoptId::kNone)
+      : TemplateLoadField<0>(source, slow_path, deopt_id, &field),
         field_(field) {}
 
   DECLARE_INSTRUCTION(LoadStaticField)
@@ -8101,7 +8128,9 @@ class LoadFieldInstr : public TemplateLoadField<1> {
                  bool calls_initializer = false,
                  intptr_t deopt_id = DeoptId::kNone)
       : TemplateLoadField(source,
-                          calls_initializer,
+                          calls_initializer
+                              ? SlowPathOnSentinelValue::kCallInitializer
+                              : SlowPathOnSentinelValue::kDoNothing,
                           deopt_id,
                           slot.IsDartField() ? &slot.field() : nullptr),
         slot_(slot),

runtime/vm/compiler/backend/il_printer.cc

@@ -976,9 +976,15 @@ void IfThenElseInstr::PrintOperandsTo(BaseTextBuffer* f) const {
 
 void LoadStaticFieldInstr::PrintOperandsTo(BaseTextBuffer* f) const {
   f->Printf("%s", String::Handle(field().name()).ToCString());
+  if (throws_access_error()) {
+    f->AddString(", ThrowsAccessError");
+  }
   if (calls_initializer()) {
     f->AddString(", CallsInitializer");
   }
+  if (throw_exception_on_initialization()) {
+    f->AddString(", ThrowExceptionOnInitialization");
+  }
 }
 
 void StoreStaticFieldInstr::PrintOperandsTo(BaseTextBuffer* f) const {
@@ -1033,6 +1039,9 @@ void MaterializeObjectInstr::PrintOperandsTo(BaseTextBuffer* f) const {
 void LoadFieldInstr::PrintOperandsTo(BaseTextBuffer* f) const {
   instance()->PrintTo(f);
   f->Printf(" . %s%s", slot().Name(), IsImmutableLoad() ? " {final}" : "");
+  if (throws_access_error()) {
+    f->AddString(", ThrowsAccessError");
+  }
   if (calls_initializer()) {
     f->AddString(", CallsInitializer");
   }

runtime/vm/compiler/backend/redundancy_elimination.cc

@@ -1904,9 +1904,9 @@ class LoadOptimizer : public ValueObject {
 
   void ClearCallsInitializer(Instruction* instr) {
     if (auto* load_field = instr->AsLoadField()) {
-      load_field->set_calls_initializer(false);
+      load_field->clear_calls_initializer();
     } else if (auto* load_static = instr->AsLoadStaticField()) {
-      load_static->set_calls_initializer(false);
+      load_static->clear_calls_initializer();
     } else {
       UNREACHABLE();
     }

runtime/vm/compiler/backend/type_propagator_test.cc

@@ -609,8 +609,7 @@ ISOLATE_UNIT_TEST_CASE(TypePropagator_RedefineCanBeSentinelWithCannotBe) {
   {
     BlockBuilder builder(H.flow_graph(), b1);
     v3 = builder.AddDefinition(new LoadStaticFieldInstr(
-        field_x, {},
-        /*calls_initializer=*/false, S.GetNextDeoptId()));
+        field_x, {}, SlowPathOnSentinelValue::kDoNothing, S.GetNextDeoptId()));
     auto v5 = builder.AddDefinition(new ConstantInstr(Object::sentinel()));
     builder.AddBranch(new StrictCompareInstr(
                           {}, Token::kEQ_STRICT, new Value(v3), new Value(v5),

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

@@ -634,14 +634,17 @@ Fragment BaseFlowGraphBuilder::StoreFieldGuarded(
 
 Fragment BaseFlowGraphBuilder::LoadStaticField(const Field& field,
                                                bool calls_initializer) {
-  // "Inititalizer" code is in charge of checking non-shared field access from
-  // stateless isolates(which exist when experimental_shared_data is enabled).
-  const bool do_call_initializer =
-      calls_initializer ||
+  const bool check_access =
       (dart::FLAG_experimental_shared_data && !field.is_shared());
+  const auto slow_path =
+      calls_initializer
+          ? SlowPathOnSentinelValue::kCallInitializer
+          : (check_access ? SlowPathOnSentinelValue::kThrowAccessError
+                          : SlowPathOnSentinelValue::kDoNothing);
   LoadStaticFieldInstr* load = new (Z) LoadStaticFieldInstr(
-      field, InstructionSource(), do_call_initializer,
-      do_call_initializer ? GetNextDeoptId() : DeoptId::kNone);
+      field, InstructionSource(), slow_path,
+      slow_path != SlowPathOnSentinelValue::kDoNothing ? GetNextDeoptId()
+                                                       : DeoptId::kNone);
   Push(load);
   return Fragment(load);
 }

runtime/vm/compiler/stub_code_compiler.cc

@@ -50,6 +50,38 @@ void StubCodeCompiler::GenerateInitStaticFieldStub() {
   __ Ret();
 }
 
+void StubCodeCompiler::GenerateCheckIsolateFieldAccessStub() {
+  const Register kFieldReg = InitStaticFieldABI::kFieldReg;
+  const Register kScratchReg = InitLateStaticFieldInternalRegs::kScratchReg;
+
+  __ EnterStubFrame();
+
+  Label throw_since_no_isolate_is_present;
+  if (!FLAG_experimental_shared_data) {
+    // Should not be invoked
+    __ Breakpoint();
+  }
+  // This stub is also called from mutator thread running without an
+  // isolate and attempts to load value from isolate static field.
+  __ LoadIsolate(kScratchReg);
+  __ BranchIfZero(kScratchReg, &throw_since_no_isolate_is_present);
+  __ LeaveStubFrame();
+  __ Ret();
+
+#if defined(TARGET_ARCH_ARM) || defined(TARGET_ARCH_ARM64)
+  // We are jumping over LeaveStubFrame so restore LR state to match one
+  // at the jump point.
+  __ set_lr_state(compiler::LRState::OnEntry().EnterFrame());
+#endif  // defined(TARGET_ARCH_ARM) || defined(TARGET_ARCH_ARM64)
+  // Throw FieldAccessError
+  __ Bind(&throw_since_no_isolate_is_present);
+  __ PushObject(NullObject());  // Make room for (unused) result.
+  __ PushRegister(kFieldReg);
+  __ CallRuntime(kStaticFieldAccessedWithoutIsolateErrorRuntimeEntry,
+                 /*argument_count=*/1);
+  __ Breakpoint();
+}
+
 void StubCodeCompiler::GenerateInitLateStaticFieldStub(bool is_final,
                                                        bool is_shared) {
   const Register kResultReg = InitStaticFieldABI::kResultReg;

runtime/vm/object_store.h

@@ -254,6 +254,7 @@ class ObjectPointerVisitor;
   RW(Code, allocate_record3_stub)                                              \
   RW(Code, allocate_record3_named_stub)                                        \
   RW(Code, allocate_unhandled_exception_stub)                                  \
+  RW(Code, check_isolate_field_access_stub)                                    \
   RW(Code, clone_context_stub)                                                 \
   RW(Code, write_barrier_wrappers_stub)                                        \
   RW(Code, array_write_barrier_stub)                                           \
@@ -361,8 +362,9 @@ class ObjectPointerVisitor;
   DO(allocate_record3_stub, AllocateRecord3)                                   \
   DO(allocate_record3_named_stub, AllocateRecord3Named)                        \
   DO(allocate_unhandled_exception_stub, AllocateUnhandledException)            \
-  DO(clone_context_stub, CloneContext)                                         \
   DO(call_closure_no_such_method_stub, CallClosureNoSuchMethod)                \
+  DO(clone_context_stub, CloneContext)                                         \
+  DO(check_isolate_field_access_stub, CheckIsolateFieldAccess)                 \
   DO(default_tts_stub, DefaultTypeTest)                                        \
   DO(default_nullable_tts_stub, DefaultNullableTypeTest)                       \
   DO(top_type_tts_stub, TopTypeTypeTest)                                       \

runtime/vm/regexp/regexp_assembler_ir.cc

@@ -518,8 +518,11 @@ void IRRegExpMacroAssembler::StoreLocal(LocalVariable* local, Value* value) {
 LoadStaticFieldInstr* IRRegExpMacroAssembler::LoadStaticField(
     const Field& field,
     bool calls_initializer) const {
-  return new (Z) LoadStaticFieldInstr(field, InstructionSource(),
-                                      calls_initializer, GetNextDeoptId());
+  return new (Z) LoadStaticFieldInstr(
+      field, InstructionSource(),
+      calls_initializer ? SlowPathOnSentinelValue::kCallInitializer
+                        : SlowPathOnSentinelValue::kDoNothing,
+      GetNextDeoptId());
 }
 
 void IRRegExpMacroAssembler::set_current_instruction(Instruction* instruction) {

runtime/vm/service_isolate.cc

@@ -392,6 +392,9 @@ class RunServiceTask : public ThreadPool::Task {
     // e.g. it could have no port to communicate with it. Declare
     // initialization failure and shut it down.
     if (main_error != nullptr) {
+      if (FLAG_trace_service) {
+        OS::PrintErr("vm-service: encountered an error: %s\n", main_error);
+      }
       ShutdownIsolate(reinterpret_cast<Dart_Isolate>(isolate));
       ServiceIsolate::InitializingFailed(main_error);
       return;

runtime/vm/stub_code_list.h

@@ -191,6 +191,7 @@ namespace dart {
   V(AsyncExceptionHandler)                                                     \
   V(CloneSuspendState)                                                         \
   V(FfiAsyncCallbackSend)                                                      \
+  V(CheckIsolateFieldAccess)                                                   \
   V(UnknownDartCode)
 
 }  // namespace dart

tests/ffi/run_isolate_group_run_test.dart

@@ -36,6 +36,15 @@ updateFooNoInitializer() {
   foo_no_initializer = 78;
 }
 
+class Baz {
+  static late final foo;
+}
+
+@pragma('vm:never-inline')
+bar() {
+  Baz.foo = 42;
+}
+
 main() {
   Expect.equals(42, IsolateGroup.runSync(() => 42));
 
@@ -95,7 +104,7 @@ main() {
     (e) => e is Error && e.toString().contains("AccessError"),
     'Expect error accessing',
   );
-  Expect.equals(foo, 56);
+  Expect.equals(56, foo);
 
   updateFooNoInitializer();
   Expect.throws(
@@ -117,7 +126,12 @@ main() {
     (e) => e is Error && e.toString().contains("AccessError"),
     'Expect error accessing',
   );
-  Expect.equals(foo_no_initializer, 78);
+  Expect.equals(78, foo_no_initializer);
+
+  {
+    bar();
+    Expect.equals(42, Baz.foo);
+  }
 
   print("All tests completed :)");
 }