[vm/compiler] Add missing truncate calls for parameter names.

Also replace any null flag entries left in the parameter names array to
zero Smi values, so that after truncation, no null checks are needed for
retrieved flag entries.

Now that all appropriate places truncate the parameter names array after
setting any required bits, dynamic closure call dispatchers can simply
check for any post-name flag entries to detect missing required named
parameters if no named arguments were passed.

Without the added truncate calls, then
vm/dart/minimal_kernel_bytecode_test fails when dynamic closure call
dispatchers use that shortcut as seen on patchset 1 of CL 160725.

Bug: #40813
Change-Id: I25abd23ad3ce42b03ec5ca29b4e067c162826066
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/160860
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>

Author: sstrickl

runtime/vm/compiler/ffi/call.cc

@@ -38,17 +38,17 @@ FunctionPtr TrampolineFunction(const Function& dart_signature,
 
   // The signature function won't have any names for the parameters. We need to
   // assign unique names for scope building and error messages.
+  function.CreateNameArrayIncludingFlags(Heap::kNew);
   const intptr_t num_params = dart_signature.num_fixed_parameters();
-  const Array& parameter_names = Array::Handle(zone, Array::New(num_params));
   for (intptr_t i = 0; i < num_params; ++i) {
     if (i == 0) {
       name = Symbols::ClosureParameter().raw();
     } else {
       name = Symbols::NewFormatted(thread, ":ffi_param%" Pd, i);
     }
-    parameter_names.SetAt(i, name);
+    function.SetParameterNameAt(i, name);
   }
-  function.set_parameter_names(parameter_names);
+  function.TruncateUnusedParameterFlags();
   function.SetFfiCSignature(c_signature);
 
   Type& type = Type::Handle(zone);

runtime/vm/compiler/frontend/bytecode_reader.cc

@@ -580,17 +580,13 @@ TypePtr BytecodeReaderHelper::ReadFunctionSignature(
   func.set_num_fixed_parameters(num_required_params);
   func.SetNumOptionalParameters(num_params - num_required_params,
                                 !has_optional_named_params);
-  const Array& parameter_types =
-      Array::Handle(Z, Array::New(num_params, Heap::kOld));
-  func.set_parameter_types(parameter_types);
-  const Array& parameter_names = Array::Handle(
-      Z, Array::New(Function::NameArrayLengthIncludingFlags(num_params),
-                    Heap::kOld));
-  func.set_parameter_names(parameter_names);
+  func.set_parameter_types(
+      Array::Handle(Z, Array::New(num_params, Heap::kOld)));
+  func.CreateNameArrayIncludingFlags(Heap::kOld);
 
   intptr_t i = 0;
-  parameter_types.SetAt(i, AbstractType::dynamic_type());
-  parameter_names.SetAt(i, Symbols::ClosureParameter());
+  func.SetParameterTypeAt(i, AbstractType::dynamic_type());
+  func.SetParameterNameAt(i, Symbols::ClosureParameter());
   ++i;
 
   AbstractType& type = AbstractType::Handle(Z);
@@ -602,9 +598,9 @@ TypePtr BytecodeReaderHelper::ReadFunctionSignature(
     } else {
       name = Symbols::NotNamed().raw();
     }
-    parameter_names.SetAt(i, name);
+    func.SetParameterNameAt(i, name);
     type ^= ReadObject();
-    parameter_types.SetAt(i, type);
+    func.SetParameterTypeAt(i, type);
   }
   if (has_parameter_flags) {
     intptr_t num_flags = reader_.ReadUInt();
@@ -2261,7 +2257,6 @@ void BytecodeReaderHelper::ReadFunctionDeclarations(const Class& cls) {
   Object& script_class = Object::Handle(Z);
   Function& function = Function::Handle(Z);
   Array& parameter_types = Array::Handle(Z);
-  Array& parameter_names = Array::Handle(Z);
   AbstractType& type = AbstractType::Handle(Z);
 
   name = cls.ScrubbedName();
@@ -2371,10 +2366,7 @@ void BytecodeReaderHelper::ReadFunctionDeclarations(const Class& cls) {
 
     parameter_types = Array::New(num_params, Heap::kOld);
     function.set_parameter_types(parameter_types);
-
-    parameter_names = Array::New(
-        Function::NameArrayLengthIncludingFlags(num_params), Heap::kOld);
-    function.set_parameter_names(parameter_names);
+    function.CreateNameArrayIncludingFlags(Heap::kOld);
 
     intptr_t param_index = 0;
     if (!is_static) {
@@ -2398,9 +2390,9 @@ void BytecodeReaderHelper::ReadFunctionDeclarations(const Class& cls) {
 
     for (; param_index < num_params; ++param_index) {
       name ^= ReadObject();
-      parameter_names.SetAt(param_index, name);
+      function.SetParameterNameAt(param_index, name);
       type ^= ReadObject();
-      parameter_types.SetAt(param_index, type);
+      function.SetParameterTypeAt(param_index, type);
     }
 
     type ^= ReadObject();
@@ -3109,6 +3101,7 @@ void BytecodeReaderHelper::ParseForwarderFunction(
       }
     }
   }
+  function.TruncateUnusedParameterFlags();
 
   if (has_forwarding_stub_target) {
     const intptr_t cp_index = reader_.ReadUInt();

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -2006,6 +2006,25 @@ Fragment FlowGraphBuilder::BuildClosureCallHasRequiredNamedArgumentsCheck(
   // Required named arguments only exist if null_safety is enabled.
   if (!Isolate::Current()->null_safety()) return Fragment();
 
+  if (descriptor.NamedCount() == 0) {
+    static_assert(compiler::target::kNumParameterFlags == 1,
+                  "IL builder assumes only one flag bit per parameter");
+    // No named args were provided, so check for any required named params.
+    // Here, we assume that the only parameter flag saved is the required bit
+    // for named parameters. If this changes, we'll need to check each flag
+    // entry appropriately for any set required bits.
+    Fragment has_any;
+    has_any += LoadLocal(vars->num_max_params);
+    has_any += LoadLocal(vars->parameter_names);
+    has_any += LoadNativeField(Slot::Array_length());
+    TargetEntryInstr *no_required, *has_required;
+    has_any += BranchIfEqual(&no_required, &has_required);
+
+    Fragment(has_required) + Goto(nsm);
+
+    return Fragment(has_any.entry, no_required);
+  }
+
   // Loop over the indexes of the named parameters of the function, checking
   // whether the named parameter at that index is required. If it is, then
   // check whether it matches any of the names in the ArgumentsDescriptor.
@@ -2050,39 +2069,20 @@ Fragment FlowGraphBuilder::BuildClosureCallHasRequiredNamedArgumentsCheck(
 
   Fragment(invalid_index) + Goto(done);
 
-  // Otherwise, we need to retrieve the value. If it's null, then this index
-  // and others that map to the same entry cannot be required (but later ones
-  // may be).
+  // Otherwise, we need to retrieve the value and check the appropriate bit.
   loop_body.current = valid_index;
   loop_body += LoadLocal(vars->parameter_names);
   loop_body += LoadLocal(temp);  // Index into parameter names array.
   loop_body += LoadIndexed(compiler::target::kWordSize);
-  // Store the result so we can use it in the non-null branch. We can reuse
-  // :expr_temp as we don't need the names index once we've gotten the contents.
-  loop_body += StoreLocal(TokenPosition::kNoSource, temp);
-  TargetEntryInstr *null_smi, *flags_smi;
-  loop_body += BranchIfNull(&null_smi, &flags_smi);
-
-  // If it was null, then skip forward to the first named parameter index that
-  // would map to the next parameter names index, since no other indices that
-  // map to the same entry can be set either.
-  Fragment skip_ahead(null_smi);
-  skip_ahead += LoadLocal(vars->current_param_index);
-  skip_ahead += IntConstant(compiler::target::kNumParameterFlagsPerElement);
-  skip_ahead += SmiBinaryOp(Token::kADD, /*is_truncating=*/true);
-  skip_ahead += StoreLocal(TokenPosition::kNoSource, vars->current_param_index);
-  skip_ahead += Drop();
-  skip_ahead += Goto(loop);
-
-  // If not null, see if any of the flag bits are set for the given named
-  // parameter. If so, this named parameter is required.
-  loop_body.current = flags_smi;
-  loop_body += LoadLocal(temp);  // Flag bits loaded from parameter names array.
   loop_body += LoadLocal(vars->current_param_index);
   loop_body += IntConstant(compiler::target::kNumParameterFlagsPerElement - 1);
   loop_body += SmiBinaryOp(Token::kBIT_AND);
+  if (compiler::target::kNumParameterFlags > 1) {
+    loop_body += IntConstant(compiler::target::kNumParameterFlags);
+    loop_body += SmiBinaryOp(Token::kMUL, /*is_truncating=*/false);
+  }
   loop_body += SmiBinaryOp(Token::kSHR);
-  loop_body += IntConstant(1);
+  loop_body += IntConstant(1 << compiler::target::kRequiredNamedParameterFlag);
   loop_body += SmiBinaryOp(Token::kBIT_AND);
   loop_body += IntConstant(0);
   TargetEntryInstr *not_set, *set;

runtime/vm/compiler/frontend/kernel_translation_helper.cc

@@ -735,8 +735,7 @@ void TranslationHelper::SetupFieldAccessorFunction(
   function.set_num_fixed_parameters(parameter_count);
   function.set_parameter_types(
       Array::Handle(Z, Array::New(parameter_count, Heap::kOld)));
-  function.set_parameter_names(
-      Array::Handle(Z, Array::New(parameter_count, Heap::kOld)));
+  function.CreateNameArrayIncludingFlags(Heap::kNew);
 
   intptr_t pos = 0;
   if (is_method) {
@@ -3075,32 +3074,25 @@ void TypeTranslator::BuildFunctionType(bool simple) {
     all_count = positional_count;
   }
 
-  const intptr_t all_count_with_receiver = all_count + 1;
-  const Array& parameter_types =
-      Array::Handle(Z, Array::New(all_count_with_receiver, Heap::kOld));
-  signature_function.set_parameter_types(parameter_types);
-  const Array& parameter_names = Array::Handle(
-      Z, Array::New(simple ? all_count_with_receiver
-                           : Function::NameArrayLengthIncludingFlags(
-                                 all_count_with_receiver),
-                    Heap::kOld));
-  signature_function.set_parameter_names(parameter_names);
+  // The additional first parameter is the receiver type (set to dynamic).
+  signature_function.set_num_fixed_parameters(1 + required_count);
+  signature_function.SetNumOptionalParameters(
+      all_count - required_count, positional_count > required_count);
+
+  signature_function.set_parameter_types(
+      Array::Handle(Z, Array::New(1 + all_count, Heap::kOld)));
+  signature_function.CreateNameArrayIncludingFlags(Heap::kOld);
 
   intptr_t pos = 0;
-  parameter_types.SetAt(pos, AbstractType::dynamic_type());
-  parameter_names.SetAt(pos, H.DartSymbolPlain("_receiver_"));
+  signature_function.SetParameterTypeAt(pos, AbstractType::dynamic_type());
+  signature_function.SetParameterNameAt(pos, H.DartSymbolPlain("_receiver_"));
   ++pos;
   for (intptr_t i = 0; i < positional_count; ++i, ++pos) {
     BuildTypeInternal();  // read ith positional parameter.
-    parameter_types.SetAt(pos, result_);
-    parameter_names.SetAt(pos, H.DartSymbolPlain("noname"));
+    signature_function.SetParameterTypeAt(pos, result_);
+    signature_function.SetParameterNameAt(pos, H.DartSymbolPlain("noname"));
   }
 
-  // The additional first parameter is the receiver type (set to dynamic).
-  signature_function.set_num_fixed_parameters(1 + required_count);
-  signature_function.SetNumOptionalParameters(
-      all_count - required_count, positional_count > required_count);
-
   if (!simple) {
     const intptr_t named_count =
         helper_->ReadListLength();  // read named_parameters list length.
@@ -3109,15 +3101,15 @@ void TypeTranslator::BuildFunctionType(bool simple) {
       String& name = H.DartSymbolObfuscate(helper_->ReadStringReference());
       BuildTypeInternal();  // read named_parameters[i].type.
       const uint8_t flags = helper_->ReadFlags();  // read flags
-      parameter_types.SetAt(pos, result_);
-      parameter_names.SetAt(pos, name);
+      signature_function.SetParameterTypeAt(pos, result_);
+      signature_function.SetParameterNameAt(pos, name);
       if (!apply_legacy_erasure_ &&
           (flags & static_cast<uint8_t>(NamedTypeFlags::kIsRequired)) != 0) {
         signature_function.SetIsRequiredAt(pos);
       }
     }
-    signature_function.TruncateUnusedParameterFlags();
   }
+  signature_function.TruncateUnusedParameterFlags();
 
   if (!simple) {
     helper_->SkipOptionalDartType();  // read typedef type.
@@ -3554,10 +3546,7 @@ void TypeTranslator::SetupFunctionParameters(
 
   function.set_parameter_types(
       Array::Handle(Z, Array::New(parameter_count, Heap::kOld)));
-  const Array& parameter_names = Array::Handle(
-      Z, Array::New(Function::NameArrayLengthIncludingFlags(parameter_count),
-                    Heap::kOld));
-  function.set_parameter_names(parameter_names);
+  function.CreateNameArrayIncludingFlags(Heap::kOld);
   intptr_t pos = 0;
   if (is_method) {
     ASSERT(!klass.IsNull());

runtime/vm/compiler/runtime_api.h

@@ -299,17 +299,22 @@ extern const word kOldPageSizeInWords;
 extern const word kOldPageMask;
 
 static constexpr intptr_t kObjectAlignment = ObjectAlignment::kObjectAlignment;
-// Parameter flags are stored in Smis. In particular, there is one flag (the
-// required flag), but we want ensure that the number of bits stored per Smi is
-// a power of two so we can simply uses shift to convert the parameter index to
-// calculate both the parameter flag index in the parameter names array to get
-// the packed flags and which bit in the packed flags to check.
+
+// Note: if other flags are added, then change the check for required parameters
+// when no named arguments are provided in
+// FlowGraphBuilder::BuildClosureCallHasRequiredNamedArgumentsCheck, since it
+// assumes there are no flag slots when no named parameters are required.
+enum ParameterFlags {
+  kRequiredNamedParameterFlag,
+  kNumParameterFlags,
+};
+// Parameter flags are stored in Smis. To ensure shifts and masks can be used to
+// calculate both the parameter flag index in the parameter names array and
+// which bit to check, kNumParameterFlagsPerElement should be a power of two.
 static constexpr intptr_t kNumParameterFlagsPerElementLog2 =
-    kBitsPerWordLog2 - 1;
+    kBitsPerWordLog2 - kNumParameterFlags;
 static constexpr intptr_t kNumParameterFlagsPerElement =
     1 << kNumParameterFlagsPerElementLog2;
-// Thus, in the untagged Smi value, only the lowest kNumParameterFlagsPerElement
-// bits are used for flags, with the other bits currently unused.
 static_assert(kNumParameterFlagsPerElement <= kSmiBits,
               "kNumParameterFlagsPerElement should fit in a Smi");
 

runtime/vm/kernel_loader.cc

@@ -2431,11 +2431,11 @@ FunctionPtr CreateFieldInitializerFunction(Thread* thread,
     initializer_fun.set_num_fixed_parameters(1);
     initializer_fun.set_parameter_types(
         Array::Handle(zone, Array::New(1, Heap::kOld)));
-    initializer_fun.set_parameter_names(
-        Array::Handle(zone, Array::New(1, Heap::kOld)));
+    initializer_fun.CreateNameArrayIncludingFlags(Heap::kOld);
     initializer_fun.SetParameterTypeAt(
         0, AbstractType::Handle(zone, field_owner.DeclarationType()));
     initializer_fun.SetParameterNameAt(0, Symbols::This());
+    initializer_fun.TruncateUnusedParameterFlags();
   }
   initializer_fun.set_result_type(AbstractType::Handle(zone, field.type()));
   initializer_fun.set_is_reflectable(false);