[vm,dyn_modules] Fix creation of dynamic invocation forwarders for bytecode functions

TEST=ci

Change-Id: Id3e01591565c20fe8fe5f18438fdc66b61ac5e1a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426521
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Slava Egorov <[email protected]>

Author: alexmarkov

runtime/vm/compiler/aot/precompiler.cc

@@ -1821,7 +1821,7 @@ static void AddNamesToFunctionsTable(Zone* zone,
   AddNameToFunctionsTable(zone, table, fname, function);
 
   *dyn_function = function.ptr();
-  if (kernel::NeedsDynamicInvocationForwarder(function)) {
+  if (function.NeedsDynamicInvocationForwarder()) {
     *mangled_name = function.name();
     *mangled_name =
         Function::CreateDynamicInvocationForwarderName(*mangled_name);

runtime/vm/interpreter.cc

@@ -3541,8 +3541,7 @@ ObjectPtr Interpreter::Run(Thread* thread,
     ASSERT(Function::KindOf(function) ==
            UntaggedFunction::kDynamicInvocationForwarder);
 
-    ArrayPtr checks = Array::RawCast(function->untag()->data());
-    FunctionPtr target = Function::RawCast(checks->untag()->element(0));
+    FunctionPtr target = Function::RawCast(function->untag()->data());
     ASSERT(Function::KindOf(target) !=
            UntaggedFunction::kDynamicInvocationForwarder);
 

runtime/vm/kernel.cc

@@ -654,89 +654,6 @@ void ReadParameterCovariance(const Function& function,
   }
 }
 
-bool NeedsDynamicInvocationForwarder(const Function& function) {
-  Zone* zone = Thread::Current()->zone();
-
-  // Right now closures do not need a dyn:* forwarder.
-  // See https://github.com/dart-lang/sdk/issues/40813
-  if (function.IsClosureFunction()) return false;
-
-  // Method extractors have no parameters to check and return value is a closure
-  // and therefore not an unboxed primitive type.
-  if (function.IsMethodExtractor()) {
-    return false;
-  }
-
-  // Record field getters have no parameters to check and 'dynamic' return type.
-  if (function.IsRecordFieldGetter()) {
-    return false;
-  }
-
-  // Invoke field dispatchers are dynamically generated, will invoke a getter to
-  // obtain the field value and then invoke ".call()" on the result.
-  // Those dynamically generated dispathers don't have proper kernel metadata
-  // associated with them - we can therefore not query if there are dynamic
-  // calls to them or not and are therefore conservative.
-  if (function.IsInvokeFieldDispatcher()) {
-    return true;
-  }
-
-  // The dyn:* forwarders perform unboxing of parameters before calling the
-  // actual target (which accepts unboxed parameters) and boxes return values
-  // of the return value.
-  if (function.HasUnboxedParameters() || function.HasUnboxedReturnValue()) {
-    return true;
-  }
-
-  if (function.MaxNumberOfParametersInRegisters(zone) > 0) {
-    return true;
-  }
-
-  // There are no parameters to type check for getters and if the return value
-  // is boxed, then the dyn:* forwarder is not needed.
-  if (function.IsImplicitGetterFunction()) {
-    return false;
-  }
-
-  // Covariant parameters (both explicitly covariant and generic-covariant-impl)
-  // are checked in the body of a function and therefore don't need checks in a
-  // dynamic invocation forwarder. So dynamic invocation forwarder is only
-  // needed if there are non-covariant parameters of non-top type.
-  if (function.IsImplicitSetterFunction()) {
-    const auto& field = Field::Handle(zone, function.accessor_field());
-    return !(field.is_covariant() || field.is_generic_covariant_impl());
-  }
-
-  const auto& type_params =
-      TypeParameters::Handle(zone, function.type_parameters());
-  if (!type_params.IsNull()) {
-    auto& bound = AbstractType::Handle(zone);
-    for (intptr_t i = 0, n = type_params.Length(); i < n; ++i) {
-      bound = type_params.BoundAt(i);
-      if (!bound.IsTopTypeForSubtyping() &&
-          !type_params.IsGenericCovariantImplAt(i)) {
-        return true;
-      }
-    }
-  }
-
-  const intptr_t num_params = function.NumParameters();
-  BitVector is_covariant(zone, num_params);
-  BitVector is_generic_covariant_impl(zone, num_params);
-  ReadParameterCovariance(function, &is_covariant, &is_generic_covariant_impl);
-
-  auto& type = AbstractType::Handle(zone);
-  for (intptr_t i = function.NumImplicitParameters(); i < num_params; ++i) {
-    type = function.ParameterTypeAt(i);
-    if (!type.IsTopTypeForSubtyping() &&
-        !is_generic_covariant_impl.Contains(i) && !is_covariant.Contains(i)) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
 static ProcedureAttributesMetadata ProcedureAttributesOf(
     Zone* zone,
     const KernelProgramInfo& kernel_program_info,

runtime/vm/kernel.h

@@ -206,12 +206,6 @@ void ReadParameterCovariance(const Function& function,
                              BitVector* is_covariant,
                              BitVector* is_generic_covariant_impl);
 
-// Returns true if the given function needs dynamic invocation forwarder:
-// that is if any of the arguments require checking on the dynamic
-// call-site: if function has no parameters or has only covariant parameters
-// as such function already checks all of its parameters.
-bool NeedsDynamicInvocationForwarder(const Function& function);
-
 ProcedureAttributesMetadata ProcedureAttributesOf(const Function& function,
                                                   Zone* zone);
 

runtime/vm/object.cc

@@ -4387,6 +4387,11 @@ FunctionPtr Function::CreateDynamicInvocationForwarder(
   // blocks inlining and can't take Function-s only Code objects.
   forwarder.set_is_visible(false);
 
+#if defined(DART_DYNAMIC_MODULES)
+  if (HasBytecode()) {
+    forwarder.ClearBytecode();
+  }
+#endif
   forwarder.ClearICDataArray();
   forwarder.ClearCode();
   forwarder.set_usage_counter(0);
@@ -4428,13 +4433,7 @@ FunctionPtr Function::GetDynamicInvocationForwarder(
       /*create_if_absent=*/false);
   if (!result.IsNull()) return result.ptr();
 
-  const bool needs_dyn_forwarder =
-#if defined(DART_DYNAMIC_MODULES) && defined(DART_PRECOMPILED_RUNTIME)
-      // TODO(alexmarkov)
-      false;
-#else
-      kernel::NeedsDynamicInvocationForwarder(*this);
-#endif
+  const bool needs_dyn_forwarder = NeedsDynamicInvocationForwarder();
   if (!needs_dyn_forwarder) {
     return ptr();
   }
@@ -4455,6 +4454,90 @@ FunctionPtr Function::GetDynamicInvocationForwarder(
   return result.ptr();
 }
 
+bool Function::NeedsDynamicInvocationForwarder() const {
+  Zone* zone = Thread::Current()->zone();
+
+  // Right now closures do not need a dyn:* forwarder.
+  // See https://github.com/dart-lang/sdk/issues/40813
+  if (IsClosureFunction()) return false;
+
+  // Method extractors have no parameters to check and return value is a closure
+  // and therefore not an unboxed primitive type.
+  if (IsMethodExtractor()) {
+    return false;
+  }
+
+  // Record field getters have no parameters to check and 'dynamic' return type.
+  if (IsRecordFieldGetter()) {
+    return false;
+  }
+
+  // Invoke field dispatchers are dynamically generated, will invoke a getter to
+  // obtain the field value and then invoke ".call()" on the result.
+  // Those dynamically generated dispathers don't have proper kernel metadata
+  // associated with them - we can therefore not query if there are dynamic
+  // calls to them or not and are therefore conservative.
+  if (IsInvokeFieldDispatcher()) {
+    return true;
+  }
+
+#if !defined(DART_PRECOMPILED_RUNTIME)
+  // The dyn:* forwarders perform unboxing of parameters before calling the
+  // actual target (which accepts unboxed parameters) and boxes return values
+  // of the return value.
+  if (HasUnboxedParameters() || HasUnboxedReturnValue()) {
+    return true;
+  }
+
+  if (MaxNumberOfParametersInRegisters(zone) > 0) {
+    return true;
+  }
+#endif
+
+  // There are no parameters to type check for getters and if the return value
+  // is boxed, then the dyn:* forwarder is not needed.
+  if (IsImplicitGetterFunction()) {
+    return false;
+  }
+
+  // Covariant parameters (both explicitly covariant and generic-covariant-impl)
+  // are checked in the body of a function and therefore don't need checks in a
+  // dynamic invocation forwarder. So dynamic invocation forwarder is only
+  // needed if there are non-covariant parameters of non-top type.
+  if (IsImplicitSetterFunction()) {
+    const auto& field = Field::Handle(zone, accessor_field());
+    return !(field.is_covariant() || field.is_generic_covariant_impl());
+  }
+
+  const auto& type_params = TypeParameters::Handle(zone, type_parameters());
+  if (!type_params.IsNull()) {
+    auto& bound = AbstractType::Handle(zone);
+    for (intptr_t i = 0, n = type_params.Length(); i < n; ++i) {
+      bound = type_params.BoundAt(i);
+      if (!bound.IsTopTypeForSubtyping() &&
+          !type_params.IsGenericCovariantImplAt(i)) {
+        return true;
+      }
+    }
+  }
+
+  const intptr_t num_params = NumParameters();
+  BitVector is_covariant(zone, num_params);
+  BitVector is_generic_covariant_impl(zone, num_params);
+  ReadParameterCovariance(&is_covariant, &is_generic_covariant_impl);
+
+  auto& type = AbstractType::Handle(zone);
+  for (intptr_t i = NumImplicitParameters(); i < num_params; ++i) {
+    type = ParameterTypeAt(i);
+    if (!type.IsTopTypeForSubtyping() &&
+        !is_generic_covariant_impl.Contains(i) && !is_covariant.Contains(i)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 void Function::ReadParameterCovariance(
     BitVector* is_covariant,
     BitVector* is_generic_covariant_impl) const {
@@ -8393,6 +8476,12 @@ void Function::AttachBytecode(const Bytecode& value) const {
   SetInstructions(StubCode::InterpretCall());
 }
 
+void Function::ClearBytecode() const {
+  ASSERT(HasBytecode());
+  untag()->set_ic_data_array_or_bytecode(Object::null());
+  ClearCode();
+}
+
 #endif  // defined(DART_DYNAMIC_MODULES)
 
 bool Function::HasCode(FunctionPtr function) {
@@ -11916,8 +12005,7 @@ bool Function::NeedsMonomorphicCheckedEntry(Zone* zone) const {
 
   // Only if there are dynamic callers and if we didn't create a dyn:* forwarder
   // for it do we need the monomorphic checked entry.
-  return HasDynamicCallers(zone) &&
-         !kernel::NeedsDynamicInvocationForwarder(*this);
+  return HasDynamicCallers(zone) && !NeedsDynamicInvocationForwarder();
 #else
   UNREACHABLE();
   return true;

runtime/vm/object.h

@@ -3271,6 +3271,7 @@ class Function : public Object {
 
 #if defined(DART_DYNAMIC_MODULES)
   void AttachBytecode(const Bytecode& bytecode) const;
+  void ClearBytecode() const;
   inline BytecodePtr GetBytecode() const;
   static inline BytecodePtr GetBytecode(FunctionPtr function);
   inline bool HasBytecode() const;
@@ -4099,6 +4100,12 @@ class Function : public Object {
 
   FunctionPtr GetDynamicInvocationForwarder(const String& mangled_name) const;
 
+  // Returns true if this function needs dynamic invocation forwarder:
+  // that is if any of the arguments require checking on the dynamic
+  // call-site: if function has no parameters or has only covariant parameters
+  // as such function already checks all of its parameters.
+  bool NeedsDynamicInvocationForwarder() const;
+
   // Fills in [is_covariant] and [is_generic_covariant_impl] vectors
   // according to covariance attributes of function parameters.
   //