From 49749bb9769e91c9c15a448d377366df834e5fec Mon Sep 17 00:00:00 2001 From: Georg Neis Date: Wed, 29 Jul 2020 15:50:36 +0200 Subject: [PATCH] [turbofan] Fix a lazy deopt bug in Array.prototype.map The bug was that the allocation of the result array (before the loop) was using the outer frame state, thus returning the allocation's result (an array full of holes) as the return value of the map operation in case the allocation triggers a lazy deopt. Bug: chromium:1104514 Change-Id: I9a6db8a5860472e1b438b6b54414938d61e166c1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2324249 Reviewed-by: Jakob Gruber Commit-Queue: Georg Neis Cr-Commit-Position: refs/heads/master@{#69129} --- src/builtins/array-map.tq | 31 +++++++++++++++--------- src/codegen/tnode.h | 2 +- src/compiler/js-call-reducer.cc | 31 ++++++++++++++++++------ test/mjsunit/compiler/regress-1104514.js | 20 +++++++++++++++ 4 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 test/mjsunit/compiler/regress-1104514.js diff --git a/src/builtins/array-map.tq b/src/builtins/array-map.tq index 8ff3cbaccd..48c8f87681 100644 --- a/src/builtins/array-map.tq +++ b/src/builtins/array-map.tq @@ -3,18 +3,28 @@ // found in the LICENSE file. namespace array { + +// Continuation for lazy deopt triggered by allocation of the result array. +transitioning javascript builtin +ArrayMapPreLoopLazyDeoptContinuation( + js-implicit context: NativeContext, receiver: JSAny)( + callback: JSAny, thisArg: JSAny, length: JSAny, result: JSAny): JSAny { + const jsreceiver = Cast(receiver) otherwise unreachable; + const outputArray = Cast(result) otherwise unreachable; + const numberLength = Cast(length) otherwise unreachable; + + const callbackfn = Cast(callback) + otherwise ThrowTypeError(MessageTemplate::kCalledNonCallable, callback); + return ArrayMapLoopContinuation( + jsreceiver, callbackfn, thisArg, outputArray, jsreceiver, kZero, + numberLength); +} + transitioning javascript builtin ArrayMapLoopEagerDeoptContinuation( js-implicit context: NativeContext, receiver: JSAny)( callback: JSAny, thisArg: JSAny, array: JSAny, initialK: JSAny, length: JSAny): JSAny { - // All continuation points in the optimized filter implementation are - // after the ToObject(O) call that ensures we are dealing with a - // JSReceiver. - // - // Also, this great mass of casts is necessary because the signature - // of Torque javascript builtins requires JSAny type for all parameters - // other than {context}. const jsreceiver = Cast(receiver) otherwise unreachable; const callbackfn = Cast(callback) otherwise unreachable; const outputArray = Cast(array) otherwise unreachable; @@ -31,17 +41,14 @@ ArrayMapLoopLazyDeoptContinuation( js-implicit context: NativeContext, receiver: JSAny)( callback: JSAny, thisArg: JSAny, array: JSAny, initialK: JSAny, length: JSAny, result: JSAny): JSAny { - // All continuation points in the optimized filter implementation are - // after the ToObject(O) call that ensures we are dealing with a - // JSReceiver. const jsreceiver = Cast(receiver) otherwise unreachable; const callbackfn = Cast(callback) otherwise unreachable; const outputArray = Cast(array) otherwise unreachable; let numberK = Cast(initialK) otherwise unreachable; const numberLength = Cast(length) otherwise unreachable; - // This custom lazy deopt point is right after the callback. map() needs - // to pick up at the next step, which is setting the callback result in + // This custom lazy deopt point is right after the callback. The continuation + // needs to pick up at the next step, which is setting the callback result in // the output array. After incrementing k, we can glide into the loop // continuation builtin. diff --git a/src/codegen/tnode.h b/src/codegen/tnode.h index 83a05d0d44..1b1df5dc33 100644 --- a/src/codegen/tnode.h +++ b/src/codegen/tnode.h @@ -355,7 +355,7 @@ class TNode { return *this; } - bool is_null() { return node_ == nullptr; } + bool is_null() const { return node_ == nullptr; } operator compiler::Node*() const { return node_; } diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index be349ff6ab..0a48bcbcc6 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -263,7 +263,8 @@ class JSCallReducerAssembler : public JSGraphAssembler { TNode arg0, TNode arg1, FrameState frame_state); // Used in special cases in which we are certain CreateArray does not throw. - TNode CreateArrayNoThrow(TNode ctor, TNode size); + TNode CreateArrayNoThrow(TNode ctor, TNode size, + FrameState frame_state); TNode AllocateEmptyJSArray(ElementsKind kind, const NativeContextRef& native_context); @@ -1089,13 +1090,12 @@ TNode JSCallReducerAssembler::JSCallRuntime2( }); } -TNode JSCallReducerAssembler::CreateArrayNoThrow(TNode ctor, - TNode size) { +TNode JSCallReducerAssembler::CreateArrayNoThrow( + TNode ctor, TNode size, FrameState frame_state) { return AddNode(graph()->NewNode( javascript()->CreateArray(1, MaybeHandle()), ctor, ctor, - size, ContextInput(), FrameStateInput(), effect(), control())); + size, ContextInput(), frame_state, effect(), control())); } - TNode JSCallReducerAssembler::AllocateEmptyJSArray( ElementsKind kind, const NativeContextRef& native_context) { // TODO(jgruber): Port AllocationBuilder to JSGraphAssembler. @@ -1477,6 +1477,17 @@ struct MapFrameStateParams { TNode original_length; }; +FrameState MapPreLoopLazyFrameState(const MapFrameStateParams& params) { + DCHECK(params.a.is_null()); + Node* checkpoint_params[] = {params.receiver, params.callback, + params.this_arg, params.original_length}; + return CreateJavaScriptBuiltinContinuationFrameState( + params.jsgraph, params.shared, + Builtins::kArrayMapPreLoopLazyDeoptContinuation, params.target, + params.context, checkpoint_params, arraysize(checkpoint_params), + params.outer_frame_state, ContinuationFrameStateMode::LAZY); +} + FrameState MapLoopLazyFrameState(const MapFrameStateParams& params, TNode k) { Node* checkpoint_params[] = { @@ -1527,11 +1538,15 @@ TNode IteratingArrayBuiltinReducerAssembler::ReduceArrayPrototypeMap( // parameters. TNode array_ctor = Constant(native_context.GetInitialJSArrayMap(kind).GetConstructor()); - TNode a = CreateArrayNoThrow(array_ctor, original_length); MapFrameStateParams frame_state_params{ - jsgraph(), shared, context, target, outer_frame_state, - receiver, fncallback, this_arg, a, original_length}; + jsgraph(), shared, context, target, outer_frame_state, + receiver, fncallback, this_arg, {} /* TBD */, original_length}; + + TNode a = + CreateArrayNoThrow(array_ctor, original_length, + MapPreLoopLazyFrameState(frame_state_params)); + frame_state_params.a = a; ThrowIfNotCallable(fncallback, MapLoopLazyFrameState(frame_state_params, ZeroConstant())); diff --git a/test/mjsunit/compiler/regress-1104514.js b/test/mjsunit/compiler/regress-1104514.js new file mode 100644 index 0000000000..1c73d9c982 --- /dev/null +++ b/test/mjsunit/compiler/regress-1104514.js @@ -0,0 +1,20 @@ +// Copyright 2020 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 + +Array(32760); // > JSArray::kInitialMaxFastElementArray + +function main() { + const a = [1, 2]; + a.x = 666; + a.toString(); + const aa = Array.prototype.map.call(a, v => v); + if (aa[0] != 1 || aa[1] != 2) { %SystemBreak(); } + a.z = 667; +} + +for (var i = 0; i < 20000; ++i) { + main(); +}