[parser] Move locals limit check from PatternRewriter to VariableDeclarationParsingScope

Change-Id: Id9955037b2de03d151e038f57f922429d85f06b3
Reviewed-on: https://chromium-review.googlesource.com/c/1425197
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58965}

src/parsing/expression-scope.h

@@ -273,10 +273,14 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
 
   VariableProxy* Declare(VariableProxy* proxy) {
     VariableKind kind = NORMAL_VARIABLE;
-    bool added;
+    bool was_added;
     this->parser()->DeclareVariable(
         proxy, kind, mode_, Variable::DefaultInitializationFlag(mode_),
-        this->parser()->scope(), &added, proxy->position());
+        this->parser()->scope(), &was_added, proxy->position());
+    if (was_added &&
+        this->parser()->scope()->num_var() > kMaxNumFunctionLocals) {
+      this->parser()->ReportMessage(MessageTemplate::kTooManyVariables);
+    }
     if (names_) names_->Add(proxy->raw_name(), this->parser()->zone());
     if (!this->IsLexicalDeclaration()) {
       if (this->parser()->loop_nesting_depth() > 0) {
@@ -312,6 +316,11 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
   }
 
  private:
+  // Limit the allowed number of local variables in a function. The hard limit
+  // in Ignition is 2^31-1 due to the size of register operands. We limit it to
+  // a more reasonable lower up-limit.
+  static const int kMaxNumFunctionLocals = (1 << 23) - 1;
+
   VariableMode mode_;
   ZonePtrList<const AstRawString>* names_;
 
@@ -331,11 +340,11 @@ class ParameterDeclarationParsingScope : public ExpressionScope<Types> {
   void Declare(VariableProxy* proxy) {
     VariableKind kind = PARAMETER_VARIABLE;
     VariableMode mode = VariableMode::kVar;
-    bool added;
+    bool was_added;
     this->parser()->DeclareVariable(
         proxy, kind, mode, Variable::DefaultInitializationFlag(mode),
-        this->parser()->scope(), &added, proxy->position());
-    if (!has_duplicate() && !added) {
+        this->parser()->scope(), &was_added, proxy->position());
+    if (!has_duplicate() && !was_added) {
       duplicate_loc_ = proxy->location();
     }
   }
@@ -655,11 +664,11 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
         has_simple_parameter_list_ ? VariableMode::kVar : VariableMode::kLet;
     for (int i = 0; i < this->variable_list()->length(); i++) {
       VariableProxy* proxy = this->variable_list()->at(i);
-      bool added;
+      bool was_added;
       this->parser()->DeclareVariable(proxy, kind, mode,
                                       Variable::DefaultInitializationFlag(mode),
-                                      result, &added, proxy->position());
-      if (!added) {
+                                      result, &was_added, proxy->position());
+      if (!was_added) {
         ExpressionScope<Types>::Report(proxy->location(),
                                        MessageTemplate::kParamDupe);
       }

src/parsing/parser.cc

@@ -562,10 +562,10 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
       bool is_rest = false;
       bool is_optional = false;
       VariableMode mode = VariableMode::kVar;
-      bool added;
-      scope->DeclareLocal(name, mode, PARAMETER_VARIABLE, &added,
+      bool was_added;
+      scope->DeclareLocal(name, mode, PARAMETER_VARIABLE, &was_added,
                           Variable::DefaultInitializationFlag(mode));
-      DCHECK(added);
+      DCHECK(was_added);
       auto var = scope->DeclareParameter(name, VariableMode::kVar, is_optional,
                                          is_rest, ast_value_factory(), beg_pos);
       var->AllocateTo(VariableLocation::PARAMETER, 0);
@@ -1360,15 +1360,16 @@ VariableProxy* Parser::DeclareVariable(const AstRawString* name,
   DCHECK_NOT_NULL(name);
   VariableProxy* proxy =
       factory()->NewVariableProxy(name, NORMAL_VARIABLE, position());
-  bool added;
-  DeclareVariable(proxy, NORMAL_VARIABLE, mode, init, scope(), &added, pos,
+  bool was_added;
+  DeclareVariable(proxy, NORMAL_VARIABLE, mode, init, scope(), &was_added, pos,
                   end_position());
   return proxy;
 }
 
 void Parser::DeclareVariable(VariableProxy* proxy, VariableKind kind,
                              VariableMode mode, InitializationFlag init,
-                             Scope* scope, bool* added, int begin, int end) {
+                             Scope* scope, bool* was_added, int begin,
+                             int end) {
   Declaration* declaration;
   if (mode == VariableMode::kVar && !scope->is_declaration_scope()) {
     DCHECK(scope->is_block_scope() || scope->is_with_scope());
@@ -1376,18 +1377,18 @@ void Parser::DeclareVariable(VariableProxy* proxy, VariableKind kind,
   } else {
     declaration = factory()->NewVariableDeclaration(begin);
   }
-  return Declare(declaration, proxy, kind, mode, init, scope, added, end);
+  return Declare(declaration, proxy, kind, mode, init, scope, was_added, end);
 }
 
 void Parser::Declare(Declaration* declaration, VariableProxy* proxy,
                      VariableKind variable_kind, VariableMode mode,
-                     InitializationFlag init, Scope* scope, bool* added,
+                     InitializationFlag init, Scope* scope, bool* was_added,
                      int var_end_pos) {
   bool local_ok = true;
   bool sloppy_mode_block_scope_function_redefinition = false;
-  scope->DeclareVariable(declaration, proxy, mode, variable_kind, init, added,
-                         &sloppy_mode_block_scope_function_redefinition,
-                         &local_ok);
+  scope->DeclareVariable(
+      declaration, proxy, mode, variable_kind, init, was_added,
+      &sloppy_mode_block_scope_function_redefinition, &local_ok);
   if (!local_ok) {
     // If we only have the start position of a proxy, we can't highlight the
     // whole variable name.  Pretend its length is 1 so that we highlight at
@@ -1425,9 +1426,9 @@ Statement* Parser::DeclareFunction(const AstRawString* variable_name,
       factory()->NewVariableProxy(variable_name, NORMAL_VARIABLE, beg_pos);
   Declaration* declaration = factory()->NewFunctionDeclaration(
       function, is_sloppy_block_function, beg_pos);
-  bool added;
+  bool was_added;
   Declare(declaration, proxy, NORMAL_VARIABLE, mode, kCreatedInitialized,
-          scope(), &added);
+          scope(), &was_added);
   if (names) names->Add(variable_name, zone());
   if (is_sloppy_block_function) {
     SloppyBlockFunctionStatement* statement =
@@ -2642,10 +2643,10 @@ Block* Parser::BuildParameterInitializationBlock(
 
 Scope* Parser::NewHiddenCatchScope() {
   Scope* catch_scope = NewScopeWithParent(scope(), CATCH_SCOPE);
-  bool added;
+  bool was_added;
   catch_scope->DeclareLocal(ast_value_factory()->dot_catch_string(),
-                            VariableMode::kVar, NORMAL_VARIABLE, &added);
-  DCHECK(added);
+                            VariableMode::kVar, NORMAL_VARIABLE, &was_added);
+  DCHECK(was_added);
   catch_scope->set_is_hidden();
   return catch_scope;
 }

src/parsing/pattern-rewriter.cc

@@ -27,15 +27,6 @@ namespace internal {
 // TODO(leszeks): Rename or remove this class
 class PatternRewriter final : public AstVisitor<PatternRewriter> {
  public:
-  // Limit the allowed number of local variables in a function. The hard limit
-  // is that offsets computed by FullCodeGenerator::StackOperand and similar
-  // functions are ints, and they should not overflow. In addition, accessing
-  // local variables creates user-controlled constants in the generated code,
-  // and we don't want too much user-controlled memory inside the code (this was
-  // the reason why this limit was introduced in the first place; see
-  // https://codereview.chromium.org/7003030/ ).
-  static const int kMaxNumFunctionLocals = 4194303;  // 2^22-1
-
   typedef Parser::DeclarationDescriptor DeclarationDescriptor;
 
   static void InitializeVariables(
@@ -75,14 +66,6 @@ class PatternRewriter final : public AstVisitor<PatternRewriter> {
 
   void RewriteParameterScopes(Expression* expr);
 
-  AstNodeFactory* factory() const { return parser_->factory(); }
-  AstValueFactory* ast_value_factory() const {
-    return parser_->ast_value_factory();
-  }
-
-  std::vector<void*>* pointer_buffer() { return parser_->pointer_buffer(); }
-
-  Zone* zone() const { return parser_->zone(); }
   Scope* scope() const { return parser_->scope(); }
 
   Parser* const parser_;
@@ -121,15 +104,6 @@ void PatternRewriter::InitializeVariables(
 }
 
 void PatternRewriter::VisitVariableProxy(VariableProxy* proxy) {
-  Scope* target_scope = scope();
-  if (declares_parameter_containing_sloppy_eval_) {
-    // When an extra declaration scope needs to be inserted to account for
-    // a sloppy eval in a default parameter or function body, the parameter
-    // needs to be declared in the function's scope, not in the varblock
-    // scope which will be used for the initializer expression.
-    target_scope = target_scope->outer_scope();
-  }
-
   DCHECK(!parser_->has_error());
   Variable* var =
       proxy->is_resolved()
@@ -143,11 +117,6 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* proxy) {
   DCHECK_NOT_NULL(var);
   DCHECK_NE(initializer_position_, kNoSourcePosition);
   var->set_initializer_position(initializer_position_);
-
-  if (var->scope()->num_var() > kMaxNumFunctionLocals) {
-    parser_->ReportMessage(MessageTemplate::kTooManyVariables);
-    return;
-  }
 }
 
 // When an extra declaration scope needs to be inserted to account for

src/parsing/preparser.h

@@ -1091,14 +1091,14 @@ class PreParser : public ParserBase<PreParser> {
 
   void DeclareVariable(VariableProxy* proxy, VariableKind kind,
                        VariableMode mode, InitializationFlag init, Scope* scope,
-                       bool* added, int position) {
-    DeclareVariableName(proxy->raw_name(), mode, scope, added, kind);
+                       bool* was_added, int position) {
+    DeclareVariableName(proxy->raw_name(), mode, scope, was_added, kind);
   }
 
   void DeclareVariableName(const AstRawString* name, VariableMode mode,
-                           Scope* scope, bool* added,
+                           Scope* scope, bool* was_added,
                            VariableKind kind = NORMAL_VARIABLE) {
-    if (scope->DeclareVariableName(name, mode, added, kind) == nullptr) {
+    if (scope->DeclareVariableName(name, mode, was_added, kind) == nullptr) {
       ReportUnidentifiableError();
     }
   }
@@ -1171,11 +1171,11 @@ class PreParser : public ParserBase<PreParser> {
                   ZonePtrList<const AstRawString>* names) {
     DCHECK_NULL(names);
     if (variable_name.string_ != nullptr) {
-      bool added;
+      bool was_added;
       if (is_strict(language_mode())) {
-        DeclareVariableName(variable_name.string_, mode, scope(), &added);
+        DeclareVariableName(variable_name.string_, mode, scope(), &was_added);
       } else {
-        scope()->DeclareVariableName(variable_name.string_, mode, &added);
+        scope()->DeclareVariableName(variable_name.string_, mode, &was_added);
       }
       if (is_sloppy_block_function) {
         GetDeclarationScope()->DeclareSloppyBlockFunction(variable_name.string_,
@@ -1192,9 +1192,9 @@ class PreParser : public ParserBase<PreParser> {
     // Preparser shouldn't be used in contexts where we need to track the names.
     DCHECK_NULL(names);
     if (variable_name.string_ != nullptr) {
-      bool added;
+      bool was_added;
       DeclareVariableName(variable_name.string_, VariableMode::kLet, scope(),
-                          &added);
+                          &was_added);
     }
     return PreParserStatement::Default();
   }
@@ -1202,8 +1202,9 @@ class PreParser : public ParserBase<PreParser> {
                                       ClassInfo* class_info,
                                       int class_token_pos) {
     if (name.string_ != nullptr) {
-      bool added;
-      DeclareVariableName(name.string_, VariableMode::kConst, scope(), &added);
+      bool was_added;
+      DeclareVariableName(name.string_, VariableMode::kConst, scope(),
+                          &was_added);
     }
   }
   V8_INLINE void DeclareClassProperty(const PreParserIdentifier& class_name,
@@ -1217,15 +1218,15 @@ class PreParser : public ParserBase<PreParser> {
                                    bool is_private, ClassInfo* class_info) {
     DCHECK_IMPLIES(is_computed_name, !is_private);
     if (is_computed_name) {
-      bool added;
+      bool was_added;
       DeclareVariableName(
           ClassFieldVariableName(ast_value_factory(),
                                  class_info->computed_field_count),
-          VariableMode::kConst, scope(), &added);
+          VariableMode::kConst, scope(), &was_added);
     } else if (is_private && property_name.string_ != nullptr) {
-      bool added;
+      bool was_added;
       DeclareVariableName(property_name.string_, VariableMode::kConst, scope(),
-                          &added);
+                          &was_added);
     }
   }
 
@@ -1397,8 +1398,8 @@ class PreParser : public ParserBase<PreParser> {
                                                      const ForInfo& for_info) {
     if (IsLexicalVariableMode(for_info.parsing_result.descriptor.mode)) {
       for (auto name : for_info.bound_names) {
-        bool added;
-        DeclareVariableName(name, VariableMode::kLet, scope(), &added);
+        bool was_added;
+        DeclareVariableName(name, VariableMode::kLet, scope(), &was_added);
       }
       return PreParserBlock::Default();
     }
@@ -1411,9 +1412,9 @@ class PreParser : public ParserBase<PreParser> {
       PreParserStatement body, Scope* inner_scope, const ForInfo& for_info) {
     // See Parser::DesugarLexicalBindingsInForStatement.
     for (auto name : for_info.bound_names) {
-      bool added;
+      bool was_added;
       DeclareVariableName(name, for_info.parsing_result.descriptor.mode,
-                          inner_scope, &added);
+                          inner_scope, &was_added);
     }
     return loop;
   }