From 68beef53c36d8640b453da4169548cbbd61c91a9 Mon Sep 17 00:00:00 2001 From: mstarzinger Date: Wed, 3 Jun 2015 04:32:25 -0700 Subject: [PATCH] Fix arrow functions requiring context without slots. This fixes a corner-case where arrow functions that require a context allocate none, because there are no additional slots allocated. Note that this didn't happen with true function scopes because they always had at least the receiver slot. The outcome was a context chain that no longer was in sync with the scope chain, hence context slot loads were bogus. This is observable using the DYNAMIC_LOCAL optimization in all compilers. R=rossberg@chromium.org,wingo@igalia.com TEST=mjsunit/harmony/regress/regress-4160 BUG=v8:4160 LOG=N Review URL: https://codereview.chromium.org/1146063006 Cr-Commit-Position: refs/heads/master@{#28788} --- src/arm/full-codegen-arm.cc | 8 +++--- src/arm64/full-codegen-arm64.cc | 8 +++--- src/code-stubs.h | 2 +- src/compiler/ast-graph-builder.cc | 3 +- src/ia32/full-codegen-ia32.cc | 8 +++--- src/mips/full-codegen-mips.cc | 8 +++--- src/mips64/full-codegen-mips64.cc | 8 +++--- src/ppc/full-codegen-ppc.cc | 8 +++--- src/x64/full-codegen-x64.cc | 8 +++--- src/x87/full-codegen-x87.cc | 8 +++--- test/mjsunit/harmony/regress/regress-4160.js | 29 ++++++++++++++++++++ 11 files changed, 63 insertions(+), 35 deletions(-) create mode 100644 test/mjsunit/harmony/regress/regress-4160.js diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index ad7423904..a4c6be8ae 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -191,17 +191,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { // Argument to NewContext is the function, which is still in r1. Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; if (info->scope()->is_script_scope()) { __ push(r1); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/arm64/full-codegen-arm64.cc b/src/arm64/full-codegen-arm64.cc index c41c05e03..507ce3dab 100644 --- a/src/arm64/full-codegen-arm64.cc +++ b/src/arm64/full-codegen-arm64.cc @@ -192,17 +192,17 @@ void FullCodeGenerator::Generate() { bool function_in_register_x1 = true; - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { // Argument to NewContext is the function, which is still in x1. Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; if (info->scope()->is_script_scope()) { __ Mov(x10, Operand(info->scope()->GetScopeInfo(info->isolate()))); __ Push(x1, x10); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/code-stubs.h b/src/code-stubs.h index eca7c76c2..77a2f8bba 100644 --- a/src/code-stubs.h +++ b/src/code-stubs.h @@ -732,7 +732,7 @@ class FastNewContextStub final : public HydrogenCodeStub { static const int kMaximumSlots = 64; FastNewContextStub(Isolate* isolate, int slots) : HydrogenCodeStub(isolate) { - DCHECK(slots > 0 && slots <= kMaximumSlots); + DCHECK(slots >= 0 && slots <= kMaximumSlots); set_sub_minor_key(SlotsBits::encode(slots)); } diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index 11ac9edf6..d1103d7c9 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -525,8 +525,7 @@ bool AstGraphBuilder::CreateGraph(bool constant_context, bool stack_check) { } // Build function context only if there are context allocated variables. - int heap_slots = info()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info()->num_heap_slots() > 0) { // Push a new inner context scope for the function. Node* inner_context = BuildLocalFunctionContext(function_context_.get(), patched_receiver); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index ffc83df5c..87aa3a225 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -184,17 +184,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; // Argument to NewContext is the function, which is still in edi. if (info->scope()->is_script_scope()) { __ push(edi); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/mips/full-codegen-mips.cc b/src/mips/full-codegen-mips.cc index 47975c1d8..df0cdfa0d 100644 --- a/src/mips/full-codegen-mips.cc +++ b/src/mips/full-codegen-mips.cc @@ -200,17 +200,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { Comment cmnt(masm_, "[ Allocate context"); // Argument to NewContext is the function, which is still in a1. bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; if (info->scope()->is_script_scope()) { __ push(a1); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/mips64/full-codegen-mips64.cc b/src/mips64/full-codegen-mips64.cc index ac56219ce..b7659c2d4 100644 --- a/src/mips64/full-codegen-mips64.cc +++ b/src/mips64/full-codegen-mips64.cc @@ -197,17 +197,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { Comment cmnt(masm_, "[ Allocate context"); // Argument to NewContext is the function, which is still in a1. bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; if (info->scope()->is_script_scope()) { __ push(a1); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/ppc/full-codegen-ppc.cc b/src/ppc/full-codegen-ppc.cc index 6ae6f106f..78132a06c 100644 --- a/src/ppc/full-codegen-ppc.cc +++ b/src/ppc/full-codegen-ppc.cc @@ -196,17 +196,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { // Argument to NewContext is the function, which is still in r4. Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; if (info->scope()->is_script_scope()) { __ push(r4); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 059626531..91d40ced0 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -183,17 +183,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; // Argument to NewContext is the function, which is still in rdi. if (info->scope()->is_script_scope()) { __ Push(rdi); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/src/x87/full-codegen-x87.cc b/src/x87/full-codegen-x87.cc index cb4173f80..5b3f8e0ee 100644 --- a/src/x87/full-codegen-x87.cc +++ b/src/x87/full-codegen-x87.cc @@ -184,17 +184,17 @@ void FullCodeGenerator::Generate() { bool function_in_register = true; // Possibly allocate a local context. - int heap_slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; - if (heap_slots > 0) { + if (info->scope()->num_heap_slots() > 0) { Comment cmnt(masm_, "[ Allocate context"); bool need_write_barrier = true; + int slots = info->scope()->num_heap_slots() - Context::MIN_CONTEXT_SLOTS; // Argument to NewContext is the function, which is still in edi. if (info->scope()->is_script_scope()) { __ push(edi); __ Push(info->scope()->GetScopeInfo(info->isolate())); __ CallRuntime(Runtime::kNewScriptContext, 2); - } else if (heap_slots <= FastNewContextStub::kMaximumSlots) { - FastNewContextStub stub(isolate(), heap_slots); + } else if (slots <= FastNewContextStub::kMaximumSlots) { + FastNewContextStub stub(isolate(), slots); __ CallStub(&stub); // Result of FastNewContextStub is always in new space. need_write_barrier = false; diff --git a/test/mjsunit/harmony/regress/regress-4160.js b/test/mjsunit/harmony/regress/regress-4160.js new file mode 100644 index 000000000..b02daeed4 --- /dev/null +++ b/test/mjsunit/harmony/regress/regress-4160.js @@ -0,0 +1,29 @@ +// Copyright 2015 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: --harmony-arrow-functions --allow-natives-syntax + +(function(x) { + (function(x) { + var boom = (() => eval(x)); + assertEquals(23, boom()); + assertEquals(23, boom()); + %OptimizeFunctionOnNextCall(boom); + assertEquals(23, boom()); + assertEquals("23", x); + })("23"); + assertEquals("42", x); +})("42"); + +(function(x) { + (function(x) { + var boom = (() => (eval("var x = 66"), x)); + assertEquals(66, boom()); + assertEquals(66, boom()); + %OptimizeFunctionOnNextCall(boom); + assertEquals(66, boom()); + assertEquals("23", x); + })("23"); + assertEquals("42", x); +})("42"); -- 2.34.1