[compiler] Use StateValuesAccess to access frame state parameters

FrameState parameters must not be iterated directly since parameters
can be encoded into StateValues (i.e. parameter i is not necessarily
InputAt(i)). Instead, they should be accessed through the
StateValuesAccess helper class.

One example:

 82: StateValues[sparse:^^^^^^](81, 31, 32, 33, 34, 35)
  81: StateValues[sparse:^^^^^^^^](110, 24, 25, 26, 27, 28, 29, 30)
  31: NumberConstant[8]
  32: NumberConstant[9]
  33: NumberConstant[10]
  34: NumberConstant[11]
  35: NumberConstant[13]

Here, node 81 holds multiple parameters. These are properly iterated
by the StateValuesAccess class.

Bug: chromium:1166136
Change-Id: I12725f83994e1c05571bcba153ff45154b16d93f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2625879
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72126}

src/compiler/js-call-reducer.cc

@@ -25,6 +25,7 @@
 #include "src/compiler/node-matchers.h"
 #include "src/compiler/property-access-builder.h"
 #include "src/compiler/simplified-operator.h"
+#include "src/compiler/state-values-utils.h"
 #include "src/compiler/type-cache.h"
 #include "src/ic/call-optimization.h"
 #include "src/logging/counters.h"
@@ -3804,13 +3805,14 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
   // some other function (and same for the {arguments_list}).
   CreateArgumentsType const type = CreateArgumentsTypeOf(arguments_list->op());
   Node* frame_state = NodeProperties::GetFrameStateInput(arguments_list);
-  FrameStateInfo state_info = FrameStateInfoOf(frame_state->op());
   int start_index = 0;
 
   int formal_parameter_count;
   {
     Handle<SharedFunctionInfo> shared;
-    if (!state_info.shared_info().ToHandle(&shared)) return NoChange();
+    if (!FrameStateInfoOf(frame_state->op()).shared_info().ToHandle(&shared)) {
+      return NoChange();
+    }
     formal_parameter_count = SharedFunctionInfoRef(broker(), shared)
                                  .internal_formal_parameter_count();
   }
@@ -3875,11 +3877,27 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
   }
   // Add the actual parameters to the {node}, skipping the receiver.
   Node* const parameters = frame_state->InputAt(kFrameStateParametersInput);
-  for (int i = start_index + 1; i < parameters->InputCount(); ++i) {
+  StateValuesAccess parameters_access(parameters);
+  auto parameters_it = ++parameters_access.begin();  // Skip the receiver.
+  for (int i = 0; i < start_index; i++) {
+    // A non-zero start_index implies that there are rest arguments. Skip them.
+    ++parameters_it;
+  }
+  int argument_count = FrameStateInfoOf(frame_state->op()).parameter_count() -
+                       1;  // Minus receiver.
+  for (int i = start_index; i < argument_count; ++i, ++parameters_it) {
+    Node* parameter_node = parameters_it.node();
+    DCHECK_NOT_NULL(parameter_node);
     node->InsertInput(graph()->zone(),
                       JSCallOrConstructNode::ArgumentIndex(argc++),
-                      parameters->InputAt(i));
+                      parameter_node);
   }
+  // TODO(jgruber): Currently, each use-site does the awkward dance above,
+  // iterating based on the FrameStateInfo's parameter count minus one, and
+  // manually advancing the iterator past the receiver. Consider wrapping all
+  // this in an understandable iterator s.t. one only needs to iterate from the
+  // beginning until done().
+  DCHECK(parameters_it.done());
 
   if (IsCallWithArrayLikeOrSpread(node)) {
     NodeProperties::ChangeOp(

src/compiler/state-values-utils.cc

@@ -379,7 +379,10 @@ void StateValuesAccess::iterator::EnsureValid() {
   }
 }
 
-Node* StateValuesAccess::iterator::node() { return Top()->Get(nullptr); }
+Node* StateValuesAccess::iterator::node() {
+  DCHECK(!done());
+  return Top()->Get(nullptr);
+}
 
 MachineType StateValuesAccess::iterator::type() {
   Node* parent = Top()->parent();
@@ -401,6 +404,7 @@ bool StateValuesAccess::iterator::operator!=(iterator const& other) const {
 }
 
 StateValuesAccess::iterator& StateValuesAccess::iterator::operator++() {
+  DCHECK(!done());
   Advance();
   return *this;
 }

test/mjsunit/regress/regress-1166136-0.js

@@ -0,0 +1,41 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax --opt --turbo-inlining
+
+function main() {
+  function vul(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9,
+               arg10, arg11, arg12, arg13, arg14, arg15, arg16, arg17, arg18,
+               arg19, arg20, arg21, arg22, arg23, arg24, arg25, arg26,) {
+    let local_0 = Reflect.construct(Object,arguments,vul);
+    let local_1;
+    let local_2;
+    let local_3;
+    let local_4;
+    let local_5;
+    let local_6;
+    let local_7;
+    let local_8;
+    let local_9;
+    let local_10;
+    let local_11;
+    let local_12;
+    let local_13;
+    let local_14;
+    let local_15;
+    let local_16;
+    let local_17;
+    let local_18;
+    let local_19;
+    let local_20;
+  }
+
+  %PrepareFunctionForOptimization(vul);
+  vul(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+      1,1);
+}
+%PrepareFunctionForOptimization(main);
+main();
+%OptimizeFunctionOnNextCall(main);
+main();

test/mjsunit/regress/regress-1166136-1.js

@@ -0,0 +1,29 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax --opt --turbo-inlining
+
+function main() {
+  function vul(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9,
+               arg10, arg11) {
+    const res = Reflect.construct(Array,arguments,vul);
+    let local_1;
+    let local_2;
+    let local_3;
+    let local_4;
+    let local_5;
+    return res;
+  }
+
+  %PrepareFunctionForOptimization(vul);
+  return vul(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12);
+}
+
+%PrepareFunctionForOptimization(main);
+main();
+const unoptimized_result = main();
+%OptimizeFunctionOnNextCall(main);
+const optimized_result = main();
+
+assertEquals(unoptimized_result, optimized_result);

test/mjsunit/regress/regress-1166136-2.js

@@ -0,0 +1,28 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax --opt --turbo-inlining
+
+function main() {
+  function vul(x0, x1, ...args) {
+    const res = Reflect.construct(Array,args,vul);
+    let local_1;
+    let local_2;
+    let local_3;
+    let local_4;
+    let local_5;
+    return res;
+  }
+
+  %PrepareFunctionForOptimization(vul);
+  return vul(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12);
+}
+
+%PrepareFunctionForOptimization(main);
+main();
+const unoptimized_result = main();
+%OptimizeFunctionOnNextCall(main);
+const optimized_result = main();
+
+assertEquals(unoptimized_result, optimized_result);