From 9884930b3225675e2c88ada54905800ee99f257c Mon Sep 17 00:00:00 2001 From: Toon Verwaest Date: Fri, 2 Nov 2018 10:08:57 +0100 Subject: [PATCH] [parser] Simplify Scope::DeclareVariable Restructure the code a little, and change how we detect sloppy block function redeclaration so we don't dereference a possibly nullptr function. Bug: chromium:900786 Change-Id: Ief124fe767603ca36f4dc8865c4aeb3e0635b4cf Reviewed-on: https://chromium-review.googlesource.com/c/1314331 Reviewed-by: Camillo Bruni Commit-Queue: Toon Verwaest Cr-Commit-Position: refs/heads/master@{#57206} --- src/ast/ast.h | 23 ++++- src/ast/scopes.cc | 116 +++++++++++-------------- src/ast/scopes.h | 4 +- src/ast/variables.h | 1 - src/globals.h | 1 - src/parsing/parser.cc | 4 +- test/mjsunit/regress/regress-900786.js | 5 ++ 7 files changed, 76 insertions(+), 78 deletions(-) create mode 100644 test/mjsunit/regress/regress-900786.js diff --git a/src/ast/ast.h b/src/ast/ast.h index e875a1a41d..36bdeae165 100644 --- a/src/ast/ast.h +++ b/src/ast/ast.h @@ -444,14 +444,26 @@ inline NestedVariableDeclaration* VariableDeclaration::AsNested() { class FunctionDeclaration final : public Declaration { public: FunctionLiteral* fun() const { return fun_; } + bool declares_sloppy_block_function() const { + return DeclaresSloppyBlockFunction::decode(bit_field_); + } private: friend class AstNodeFactory; - FunctionDeclaration(VariableProxy* proxy, FunctionLiteral* fun, int pos) - : Declaration(proxy, pos, kFunctionDeclaration), fun_(fun) {} + class DeclaresSloppyBlockFunction + : public BitField {}; + + FunctionDeclaration(VariableProxy* proxy, FunctionLiteral* fun, + bool declares_sloppy_block_function, int pos) + : Declaration(proxy, pos, kFunctionDeclaration), fun_(fun) { + bit_field_ = DeclaresSloppyBlockFunction::update( + bit_field_, declares_sloppy_block_function); + } FunctionLiteral* fun_; + + static const uint8_t kNextBitFieldIndex = DeclaresSloppyBlockFunction::kNext; }; @@ -2883,8 +2895,11 @@ class AstNodeFactory final { } FunctionDeclaration* NewFunctionDeclaration(VariableProxy* proxy, - FunctionLiteral* fun, int pos) { - return new (zone_) FunctionDeclaration(proxy, fun, pos); + FunctionLiteral* fun, + bool is_sloppy_block_function, + int pos) { + return new (zone_) + FunctionDeclaration(proxy, fun, is_sloppy_block_function, pos); } Block* NewBlock(int capacity, bool ignore_completion_value, diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index 3fbbc6dbd8..4fd5d012f4 100644 --- a/src/ast/scopes.cc +++ b/src/ast/scopes.cc @@ -1058,8 +1058,7 @@ Variable* DeclarationScope::DeclareParameterName( } Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode, - InitializationFlag init_flag, VariableKind kind, - MaybeAssignedFlag maybe_assigned_flag) { + InitializationFlag init_flag) { DCHECK(!already_resolved_); // This function handles VariableMode::kVar, VariableMode::kLet, and // VariableMode::kConst modes. VariableMode::kDynamic variables are @@ -1070,7 +1069,7 @@ Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode, mode == VariableMode::kVar || mode == VariableMode::kLet || mode == VariableMode::kConst); DCHECK(!GetDeclarationScope()->was_lazily_parsed()); - return Declare(zone(), name, mode, kind, init_flag, maybe_assigned_flag); + return Declare(zone(), name, mode, NORMAL_VARIABLE, init_flag); } Variable* Scope::DeclareVariable( @@ -1094,7 +1093,6 @@ Variable* Scope::DeclareVariable( VariableProxy* proxy = declaration->proxy(); DCHECK_NOT_NULL(proxy->raw_name()); const AstRawString* name = proxy->raw_name(); - bool is_function_declaration = declaration->IsFunctionDeclaration(); // Pessimistically assume that top-level variables will be assigned. // @@ -1107,70 +1105,54 @@ Variable* Scope::DeclareVariable( if (mode != VariableMode::kConst) proxy->set_is_assigned(); } - Variable* var = nullptr; - if (is_eval_scope() && is_sloppy(language_mode()) && - mode == VariableMode::kVar) { - // In a var binding in a sloppy direct eval, pollute the enclosing scope - // with this new binding by doing the following: - // The proxy is bound to a lookup variable to force a dynamic declaration - // using the DeclareEvalVar or DeclareEvalFunction runtime functions. - var = new (zone()) - Variable(this, name, mode, NORMAL_VARIABLE, init, kMaybeAssigned); - var->AllocateTo(VariableLocation::LOOKUP, -1); - } else { - // Declare the variable in the declaration scope. - var = LookupLocal(name); - if (var == nullptr) { + Variable* var = LookupLocal(name); + // Declare the variable in the declaration scope. + if (V8_LIKELY(var == nullptr)) { + if (V8_UNLIKELY(is_eval_scope() && is_sloppy(language_mode()) && + mode == VariableMode::kVar)) { + // In a var binding in a sloppy direct eval, pollute the enclosing scope + // with this new binding by doing the following: + // The proxy is bound to a lookup variable to force a dynamic declaration + // using the DeclareEvalVar or DeclareEvalFunction runtime functions. + var = new (zone()) + Variable(this, name, mode, NORMAL_VARIABLE, init, kMaybeAssigned); + var->AllocateTo(VariableLocation::LOOKUP, -1); + } else { // Declare the name. - VariableKind kind = NORMAL_VARIABLE; - if (is_function_declaration) { - kind = FUNCTION_VARIABLE; - } - var = DeclareLocal(name, mode, init, kind, kNotAssigned); - } else if (IsLexicalVariableMode(mode) || - IsLexicalVariableMode(var->mode())) { - // Allow duplicate function decls for web compat, see bug 4693. - bool duplicate_allowed = false; - if (is_sloppy(language_mode()) && is_function_declaration && - var->is_function()) { - DCHECK(IsLexicalVariableMode(mode) && - IsLexicalVariableMode(var->mode())); - // If the duplication is allowed, then the var will show up - // in the SloppyBlockFunctionMap and the new FunctionKind - // will be a permitted duplicate. - FunctionKind function_kind = - declaration->AsFunctionDeclaration()->fun()->kind(); - SloppyBlockFunctionMap* map = - GetDeclarationScope()->sloppy_block_function_map(); - duplicate_allowed = map != nullptr && - map->Lookup(const_cast(name), - name->Hash()) != nullptr && - !IsAsyncFunction(function_kind) && - !IsGeneratorFunction(function_kind); - } - if (duplicate_allowed) { - *sloppy_mode_block_scope_function_redefinition = true; - } else { - // The name was declared in this scope before; check for conflicting - // re-declarations. We have a conflict if either of the declarations - // is not a var (in script scope, we also have to ignore legacy const - // for compatibility). There is similar code in runtime.cc in the - // Declare functions. The function CheckConflictingVarDeclarations - // checks for var and let bindings from different scopes whereas this - // is a check for conflicting declarations within the same scope. This - // check also covers the special case - // - // function () { let x; { var x; } } - // - // because the var declaration is hoisted to the function scope where - // 'x' is already bound. - DCHECK(IsDeclaredVariableMode(var->mode())); - // In harmony we treat re-declarations as early errors. See - // ES5 16 for a definition of early errors. - *ok = false; - } - } else if (mode == VariableMode::kVar) { - var->set_maybe_assigned(); + var = DeclareLocal(name, mode, init); + } + } else { + var->set_maybe_assigned(); + if (V8_UNLIKELY(IsLexicalVariableMode(mode) || + IsLexicalVariableMode(var->mode()))) { + // The name was declared in this scope before; check for conflicting + // re-declarations. We have a conflict if either of the declarations is + // not a var (in script scope, we also have to ignore legacy const for + // compatibility). There is similar code in runtime.cc in the Declare + // functions. The function CheckConflictingVarDeclarations checks for + // var and let bindings from different scopes whereas this is a check + // for conflicting declarations within the same scope. This check also + // covers the special case + // + // function () { let x; { var x; } } + // + // because the var declaration is hoisted to the function scope where + // 'x' is already bound. + // + // In harmony we treat re-declarations as early errors. See ES5 16 for a + // definition of early errors. + // + // Allow duplicate function decls for web compat, see bug 4693. If the + // duplication is allowed, then the var will show up in the + // SloppyBlockFunctionMap. + SloppyBlockFunctionMap* map = + GetDeclarationScope()->sloppy_block_function_map(); + *ok = + map != nullptr && declaration->IsFunctionDeclaration() && + declaration->AsFunctionDeclaration() + ->declares_sloppy_block_function() && + map->Lookup(const_cast(name), name->Hash()) != nullptr; + *sloppy_mode_block_scope_function_redefinition = *ok; } } DCHECK_NOT_NULL(var); diff --git a/src/ast/scopes.h b/src/ast/scopes.h index 297bb11cb2..7cdabf629a 100644 --- a/src/ast/scopes.h +++ b/src/ast/scopes.h @@ -194,9 +194,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) { // Declare a local variable in this scope. If the variable has been // declared before, the previously declared variable is returned. Variable* DeclareLocal(const AstRawString* name, VariableMode mode, - InitializationFlag init_flag = kCreatedInitialized, - VariableKind kind = NORMAL_VARIABLE, - MaybeAssignedFlag maybe_assigned_flag = kNotAssigned); + InitializationFlag init_flag = kCreatedInitialized); Variable* DeclareVariable(Declaration* declaration, VariableMode mode, InitializationFlag init, diff --git a/src/ast/variables.h b/src/ast/variables.h index d33062973b..32711213e9 100644 --- a/src/ast/variables.h +++ b/src/ast/variables.h @@ -131,7 +131,6 @@ class Variable final : public ZoneObject { return kind() != SLOPPY_FUNCTION_NAME_VARIABLE || is_strict(language_mode); } - bool is_function() const { return kind() == FUNCTION_VARIABLE; } bool is_this() const { return kind() == THIS_VARIABLE; } bool is_sloppy_function_name() const { return kind() == SLOPPY_FUNCTION_NAME_VARIABLE; diff --git a/src/globals.h b/src/globals.h index a5756ff6be..3d88c04934 100644 --- a/src/globals.h +++ b/src/globals.h @@ -1030,7 +1030,6 @@ inline const char* VariableMode2String(VariableMode mode) { enum VariableKind : uint8_t { NORMAL_VARIABLE, - FUNCTION_VARIABLE, THIS_VARIABLE, SLOPPY_FUNCTION_NAME_VARIABLE }; diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc index 7c725e9887..f8b92813cc 100644 --- a/src/parsing/parser.cc +++ b/src/parsing/parser.cc @@ -1441,8 +1441,8 @@ Statement* Parser::DeclareFunction(const AstRawString* variable_name, ZonePtrList* names) { VariableProxy* proxy = factory()->NewVariableProxy(variable_name, NORMAL_VARIABLE, pos); - Declaration* declaration = - factory()->NewFunctionDeclaration(proxy, function, pos); + Declaration* declaration = factory()->NewFunctionDeclaration( + proxy, function, is_sloppy_block_function, pos); Declare(declaration, DeclarationDescriptor::NORMAL, mode, kCreatedInitialized); if (names) names->Add(variable_name, zone()); diff --git a/test/mjsunit/regress/regress-900786.js b/test/mjsunit/regress/regress-900786.js new file mode 100644 index 0000000000..c012e3fcd8 --- /dev/null +++ b/test/mjsunit/regress/regress-900786.js @@ -0,0 +1,5 @@ +// Copyright 2018 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. + +assertThrows("{function g(){}function g(){+", SyntaxError);