From 9fa7ce514e6a2d55d37e75323b411f27ab7ac6b0 Mon Sep 17 00:00:00 2001 From: Paolo Severini Date: Wed, 16 Jun 2021 10:10:35 -0700 Subject: [PATCH] [turbofan] Fix iterator-generator issue with --turbo-optimize-apply Fuzzing found a problem with --turbo-optimize-apply when the Array.prototype iterator is replaced with a generator function. We can the issue by installing a protector on the array iterator. This CL also defines the --turbo-optimize-apply as 'future' to get more test coverage. Bug: v8:9974 Change-Id: Id5bc68fde98ea5d1f6a951c4381ca6283b892632 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2966058 Commit-Queue: Paolo Severini Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#75197} --- src/compiler/js-call-reducer.cc | 7 ++ src/flags/flag-definitions.h | 1 + .../call-with-arraylike-or-spread-5.js | 55 +++++++++++++++ .../call-with-arraylike-or-spread-6.js | 55 +++++++++++++++ .../call-with-arraylike-or-spread-7.js | 67 +++++++++++++++++++ 5 files changed, 185 insertions(+) create mode 100644 test/mjsunit/compiler/call-with-arraylike-or-spread-5.js create mode 100644 test/mjsunit/compiler/call-with-arraylike-or-spread-6.js create mode 100644 test/mjsunit/compiler/call-with-arraylike-or-spread-7.js diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index 5d2d797ea5..c6660f9dde 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -4152,6 +4152,13 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread( return NoChange(); } + // For call/construct with spread, we need to also install a code + // dependency on the array iterator lookup protector cell to ensure + // that no one messed with the %ArrayIteratorPrototype%.next method. + if (IsCallOrConstructWithSpread(node)) { + if (!dependencies()->DependOnArrayIteratorProtector()) return NoChange(); + } + int new_argument_count; if (arguments_list->opcode() == IrOpcode::kJSCreateLiteralArray) { // Find array length and elements' kind from the feedback's allocation diff --git a/src/flags/flag-definitions.h b/src/flags/flag-definitions.h index 01db9b4a2a..b984c86891 100644 --- a/src/flags/flag-definitions.h +++ b/src/flags/flag-definitions.h @@ -865,6 +865,7 @@ DEFINE_BOOL(turbo_compress_translation_arrays, false, DEFINE_BOOL(turbo_inline_js_wasm_calls, false, "inline JS->Wasm calls") DEFINE_BOOL(turbo_optimize_apply, false, "optimize Function.prototype.apply") +DEFINE_WEAK_IMPLICATION(future, turbo_optimize_apply) DEFINE_BOOL(turbo_collect_feedback_in_generic_lowering, true, "enable experimental feedback collection in generic lowering.") diff --git a/test/mjsunit/compiler/call-with-arraylike-or-spread-5.js b/test/mjsunit/compiler/call-with-arraylike-or-spread-5.js new file mode 100644 index 0000000000..e74295c361 --- /dev/null +++ b/test/mjsunit/compiler/call-with-arraylike-or-spread-5.js @@ -0,0 +1,55 @@ +// 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 --turbo-optimize-apply --opt + +// These tests do not work well if this script is run more than once (e.g. +// --stress-opt); after a few runs the whole function is immediately compiled +// and assertions would fail. We prevent re-runs. +// Flags: --nostress-opt --no-always-opt + +// Some of the tests rely on optimizing/deoptimizing at predictable moments, so +// this is not suitable for deoptimization fuzzing. +// Flags: --deopt-every-n-times=0 + +// Test for optimization of CallWithSpread when the array iterator is replaced +// with a generator function. +// +// Note: this test must be in a separate file because the test invalidates a +// protector, which then remains invalidated. +(function () { + "use strict"; + + // This invalidates the DependOnArrayIteratorProtector. + Object.defineProperty(Array.prototype, Symbol.iterator, { + value: function* () { + yield 42; + }, + }); + + var log_got_interpreted = true; + + function log(a) { + assertEquals(1, arguments.length); + log_got_interpreted = %IsBeingInterpreted(); + return a; + } + function foo() { + return log(...[1]); + } + + %PrepareFunctionForOptimization(log); + %PrepareFunctionForOptimization(foo); + assertEquals(42, foo()); + assertTrue(log_got_interpreted); + + // Compile foo. + %OptimizeFunctionOnNextCall(log); + %OptimizeFunctionOnNextCall(foo); + assertEquals(42, foo()); + // The call with spread should not have been inlined, because of the + // generator/iterator. + assertFalse(log_got_interpreted); + assertOptimized(foo); +})(); diff --git a/test/mjsunit/compiler/call-with-arraylike-or-spread-6.js b/test/mjsunit/compiler/call-with-arraylike-or-spread-6.js new file mode 100644 index 0000000000..61884b4b28 --- /dev/null +++ b/test/mjsunit/compiler/call-with-arraylike-or-spread-6.js @@ -0,0 +1,55 @@ +// 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 --turbo-optimize-apply --opt + +// These tests do not work well if this script is run more than once (e.g. +// --stress-opt); after a few runs the whole function is immediately compiled +// and assertions would fail. We prevent re-runs. +// Flags: --nostress-opt --no-always-opt + +// Some of the tests rely on optimizing/deoptimizing at predictable moments, so +// this is not suitable for deoptimization fuzzing. +// Flags: --deopt-every-n-times=0 + +// Test for optimization of CallWithSpread when the array iterator is replaced +// with a generator function and the array in empty. +// +// Note: this test must be in a separate file because the test invalidates a +// protector, which then remains invalidated. +(function () { + "use strict"; + + // This invalidates the DependOnArrayIteratorProtector. + Object.defineProperty(Array.prototype, Symbol.iterator, { + value: function* () { + yield 42; + }, + }); + + var log_got_interpreted = true; + + function log(a) { + assertEquals(1, arguments.length); + log_got_interpreted = %IsBeingInterpreted(); + return a; + } + function foo() { + return log(...[]); + } + + %PrepareFunctionForOptimization(log); + %PrepareFunctionForOptimization(foo); + assertEquals(42, foo()); + assertTrue(log_got_interpreted); + + // Compile foo. + %OptimizeFunctionOnNextCall(log); + %OptimizeFunctionOnNextCall(foo); + assertEquals(42, foo()); + // The call with spread should not have been inlined, because of the + // generator/iterator. + assertFalse(log_got_interpreted); + assertOptimized(foo); +})(); diff --git a/test/mjsunit/compiler/call-with-arraylike-or-spread-7.js b/test/mjsunit/compiler/call-with-arraylike-or-spread-7.js new file mode 100644 index 0000000000..147ba5bddb --- /dev/null +++ b/test/mjsunit/compiler/call-with-arraylike-or-spread-7.js @@ -0,0 +1,67 @@ +// 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 --turbo-optimize-apply --opt + +// These tests do not work well if this script is run more than once (e.g. +// --stress-opt); after a few runs the whole function is immediately compiled +// and assertions would fail. We prevent re-runs. +// Flags: --nostress-opt --no-always-opt + +// Some of the tests rely on optimizing/deoptimizing at predictable moments, so +// this is not suitable for deoptimization fuzzing. +// Flags: --deopt-every-n-times=0 + +// Test for optimization of CallWithSpread when the array iterator is replaced +// with a generator function after a function is compiled. +// +// Note: this test must be in a separate file because the test invalidates a +// protector, which then remains invalidated. +(function () { + "use strict"; + var log_got_interpreted = true; + + function log(a) { + assertEquals(1, arguments.length); + log_got_interpreted = %IsBeingInterpreted(); + return a; + } + function foo() { + return log(...[1]); + } + + %PrepareFunctionForOptimization(log); + %PrepareFunctionForOptimization(foo); + assertEquals(1, foo()); + assertTrue(log_got_interpreted); + + // Compile foo. + %OptimizeFunctionOnNextCall(log); + %OptimizeFunctionOnNextCall(foo); + assertEquals(1, foo()); + // The call with spread should have been inlined. + assertFalse(log_got_interpreted); + assertOptimized(foo); + + // This invalidates the DependOnArrayIteratorProtector and causes deopt. + Object.defineProperty(Array.prototype, Symbol.iterator, { + value: function* () { + yield 42; + }, + }); + + // Now we expect the value yielded by the generator. + assertEquals(42, foo()); + assertFalse(log_got_interpreted); + assertUnoptimized(foo); + + // Recompile 'foo'. + %PrepareFunctionForOptimization(foo); + %OptimizeFunctionOnNextCall(foo); + assertEquals(42, foo()); + // The call with spread will not be inlined because we have redefined the + // array iterator. + assertFalse(log_got_interpreted); + assertOptimized(foo); +})();