From a48c03bb2afcd52b7e120f6226e949c1e6f77579 Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Fri, 1 Jul 2011 14:05:46 +0000 Subject: [PATCH] Fix an issue with optimization of functions inside catch. When optimizing a function defined inside a catch, we did not count the catch context as part of the context chain. R=vegorov@chromium.org BUG=1521 TEST=regress-1521 Review URL: http://codereview.chromium.org/7285032 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8518 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/scopes.cc | 97 ++++++++++++++++++++++++------------ src/scopes.h | 10 +++- test/mjsunit/regress/regress-1521.js | 47 +++++++++++++++++ 3 files changed, 120 insertions(+), 34 deletions(-) create mode 100644 test/mjsunit/regress/regress-1521.js diff --git a/src/scopes.cc b/src/scopes.cc index c20c852..29712de 100644 --- a/src/scopes.cc +++ b/src/scopes.cc @@ -119,9 +119,9 @@ Scope::Scope(Type type) temps_(0), params_(0), unresolved_(0), - decls_(0) { + decls_(0), + already_resolved_(false) { SetDefaults(type, NULL, Handle::null()); - ASSERT(!resolved()); } @@ -131,14 +131,14 @@ Scope::Scope(Scope* outer_scope, Type type) temps_(4), params_(4), unresolved_(16), - decls_(4) { + decls_(4), + already_resolved_(false) { SetDefaults(type, outer_scope, Handle::null()); // At some point we might want to provide outer scopes to // eval scopes (by walking the stack and reading the scope info). // In that case, the ASSERT below needs to be adjusted. ASSERT((type == GLOBAL_SCOPE || type == EVAL_SCOPE) == (outer_scope == NULL)); ASSERT(!HasIllegalRedeclaration()); - ASSERT(!resolved()); } @@ -148,15 +148,34 @@ Scope::Scope(Scope* inner_scope, Handle scope_info) temps_(4), params_(4), unresolved_(16), - decls_(4) { + decls_(4), + already_resolved_(true) { ASSERT(!scope_info.is_null()); SetDefaults(FUNCTION_SCOPE, NULL, scope_info); - ASSERT(resolved()); if (scope_info->HasHeapAllocatedLocals()) { num_heap_slots_ = scope_info_->NumberOfContextSlots(); } + AddInnerScope(inner_scope); +} + +Scope::Scope(Scope* inner_scope, Handle catch_variable_name) + : inner_scopes_(1), + variables_(), + temps_(0), + params_(0), + unresolved_(0), + decls_(0), + already_resolved_(true) { + SetDefaults(CATCH_SCOPE, NULL, Handle::null()); AddInnerScope(inner_scope); + ++num_var_or_const_; + Variable* variable = variables_.Declare(this, + catch_variable_name, + Variable::VAR, + true, // Valid left-hand side. + Variable::NORMAL); + AllocateHeapSlot(variable); } @@ -190,30 +209,43 @@ void Scope::SetDefaults(Type type, Scope* Scope::DeserializeScopeChain(CompilationInfo* info, Scope* global_scope) { + // Reconstruct the outer scope chain from a closure's context chain. ASSERT(!info->closure().is_null()); - // If we have a serialized scope info, reuse it. + Context* context = info->closure()->context(); + Scope* current_scope = NULL; Scope* innermost_scope = NULL; - Scope* scope = NULL; - - SerializedScopeInfo* scope_info = info->closure()->shared()->scope_info(); - if (scope_info != SerializedScopeInfo::Empty()) { - JSFunction* current = *info->closure(); - do { - current = current->context()->closure(); - Handle scope_info(current->shared()->scope_info()); - if (*scope_info != SerializedScopeInfo::Empty()) { - scope = new Scope(scope, scope_info); - if (innermost_scope == NULL) innermost_scope = scope; + bool contains_with = false; + while (!context->IsGlobalContext()) { + if (context->IsWithContext()) { + // All the inner scopes are inside a with. + contains_with = true; + for (Scope* s = innermost_scope; s != NULL; s = s->outer_scope()) { + s->scope_inside_with_ = true; + } + } else { + if (context->IsFunctionContext()) { + SerializedScopeInfo* scope_info = + context->closure()->shared()->scope_info(); + current_scope = + new Scope(current_scope, Handle(scope_info)); } else { - ASSERT(current->context()->IsGlobalContext()); + ASSERT(context->IsCatchContext()); + String* name = String::cast(context->extension()); + current_scope = new Scope(current_scope, Handle(name)); } - } while (!current->context()->IsGlobalContext()); - } + if (contains_with) current_scope->RecordWithStatement(); + if (innermost_scope == NULL) innermost_scope = current_scope; + } - global_scope->AddInnerScope(scope); - if (innermost_scope == NULL) innermost_scope = global_scope; + // Forget about a with when we move to a context for a different function. + if (context->previous()->closure() != context->closure()) { + contains_with = false; + } + context = context->previous(); + } - return innermost_scope; + global_scope->AddInnerScope(current_scope); + return (innermost_scope == NULL) ? global_scope : innermost_scope; } @@ -238,7 +270,7 @@ bool Scope::Analyze(CompilationInfo* info) { void Scope::Initialize(bool inside_with) { - ASSERT(!resolved()); + ASSERT(!already_resolved()); // Add this scope as a new inner scope of the outer scope. if (outer_scope_ != NULL) { @@ -279,11 +311,10 @@ void Scope::Initialize(bool inside_with) { Variable* Scope::LocalLookup(Handle name) { Variable* result = variables_.Lookup(name); - if (result != NULL || !resolved()) { + if (result != NULL || scope_info_.is_null()) { return result; } - // If the scope is resolved, we can find a variable in serialized scope - // info. + // If we have a serialized scope info, we might find the variable there. // // We should never lookup 'arguments' in this scope as it is implicitly // present in every scope. @@ -331,7 +362,7 @@ Variable* Scope::DeclareFunctionVar(Handle name) { void Scope::DeclareParameter(Handle name) { - ASSERT(!resolved()); + ASSERT(!already_resolved()); ASSERT(is_function_scope()); Variable* var = variables_.Declare(this, name, Variable::VAR, true, Variable::NORMAL); @@ -340,7 +371,7 @@ void Scope::DeclareParameter(Handle name) { Variable* Scope::DeclareLocal(Handle name, Variable::Mode mode) { - ASSERT(!resolved()); + ASSERT(!already_resolved()); // This function handles VAR and CONST modes. DYNAMIC variables are // introduces during variable allocation, INTERNAL variables are allocated // explicitly, and TEMPORARY variables are allocated via NewTemporary(). @@ -363,7 +394,7 @@ VariableProxy* Scope::NewUnresolved(Handle name, // Note that we must not share the unresolved variables with // the same name because they may be removed selectively via // RemoveUnresolved(). - ASSERT(!resolved()); + ASSERT(!already_resolved()); VariableProxy* proxy = new VariableProxy(name, false, inside_with, position); unresolved_.Add(proxy); return proxy; @@ -383,7 +414,7 @@ void Scope::RemoveUnresolved(VariableProxy* var) { Variable* Scope::NewTemporary(Handle name) { - ASSERT(!resolved()); + ASSERT(!already_resolved()); Variable* var = new Variable(this, name, Variable::TEMPORARY, true, Variable::NORMAL); temps_.Add(var); @@ -1022,7 +1053,7 @@ void Scope::AllocateVariablesRecursively() { // If scope is already resolved, we still need to allocate // variables in inner scopes which might not had been resolved yet. - if (resolved()) return; + if (already_resolved()) return; // The number of slots required for variables. num_stack_slots_ = 0; num_heap_slots_ = Context::MIN_CONTEXT_SLOTS; diff --git a/src/scopes.h b/src/scopes.h index ffa65c1..a7623a3 100644 --- a/src/scopes.h +++ b/src/scopes.h @@ -364,6 +364,10 @@ class Scope: public ZoneObject { bool outer_scope_is_eval_scope_; bool force_eager_compilation_; + // True if it doesn't need scope resolution (e.g., if the scope was + // constructed based on a serialized scope info or a catch context). + bool already_resolved_; + // Computed as variables are declared. int num_var_or_const_; @@ -373,7 +377,7 @@ class Scope: public ZoneObject { // Serialized scopes support. Handle scope_info_; - bool resolved() { return !scope_info_.is_null(); } + bool already_resolved() { return already_resolved_; } // Create a non-local variable with a given name. // These variables are looked up dynamically at runtime. @@ -409,8 +413,12 @@ class Scope: public ZoneObject { void AllocateVariablesRecursively(); private: + // Construct a function scope based on the scope info. Scope(Scope* inner_scope, Handle scope_info); + // Construct a catch scope with a binding for the name. + Scope(Scope* inner_scope, Handle catch_variable_name); + void AddInnerScope(Scope* inner_scope) { if (inner_scope != NULL) { inner_scopes_.Add(inner_scope); diff --git a/test/mjsunit/regress/regress-1521.js b/test/mjsunit/regress/regress-1521.js new file mode 100644 index 0000000..415db67 --- /dev/null +++ b/test/mjsunit/regress/regress-1521.js @@ -0,0 +1,47 @@ +// Copyright 2011 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Optimized variable access inside through a catch context should work. +function test(x) { + try { + throw new Error(); + } catch (e) { + var y = {f: 1}; + var f = function () { + var z = y; + var g = function () { + if (y.f === z.f) return x; + }; + %OptimizeFunctionOnNextCall(g); + return g; + } + assertEquals(3, f()()); + } +} + +test(3); + -- 2.7.4