[parsing] Fix context allocation for async functions.

src/parsing/parser.cc

 // Copyright 2012 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.
 
 #include "src/parsing/parser.h"
 
 #include <memory>
 
 #include "src/api.h"
 #include "src/ast/ast-expression-rewriter.h"
@@ skipping 736 matching lines @@
       // Declare the special module parameter.
       auto name = ast_value_factory()->empty_string();
       bool is_duplicate;
       bool is_rest = false;
       bool is_optional = false;
       auto var = scope->DeclareParameter(name, VAR, is_optional, is_rest,
                                          &is_duplicate, ast_value_factory());
       DCHECK(!is_duplicate);
       var->AllocateTo(VariableLocation::PARAMETER, 0);
 
-      PrepareGeneratorVariables(&function_state);
+      PrepareGeneratorVariables();
       Expression* initial_yield =
           BuildInitialYield(kNoSourcePosition, kGeneratorFunction);
       body->Add(
           factory()->NewExpressionStatement(initial_yield, kNoSourcePosition),
           zone());
 
       ParseModuleItemList(body, &ok);
       ok = ok &&
            module()->Validate(this->scope()->AsModuleScope(),
                               &pending_error_handler_, zone());
@@ skipping 1728 matching lines @@
 
     for (auto p : parameters.params) {
       if (p->pattern != nullptr) reindexer.Reindex(p->pattern);
       if (p->initializer != nullptr) reindexer.Reindex(p->initializer);
     }
 
     DCHECK(reindexer.count() <= function_state_->materialized_literal_count());
   }
 }
 
-void Parser::PrepareGeneratorVariables(FunctionState* function_state) {
+void Parser::PrepareGeneratorVariables() {
   // For generators, allocating variables in contexts is currently a win because
   // it minimizes the work needed to suspend and resume an activation.  The
-  // code produced for generators relies on this forced context allocation, but
-  // not in an essential way.
-  scope()->ForceContextAllocation();
+  // code produced for generators relies on this forced context allocation (it
+  // does not restore the frame's parameters upon resume).
+  function_state_->scope()->ForceContextAllocation();

[Dan Ehrenberg, 2016/12/09 06:02:21]

I take it this is the key fix and the rest is cleanup, right?

[neis, 2016/12/09 10:54:52]

Yes this is the main fix.  But I think it was also fishy that we would force the
context allocation quite late, after potentially other stuff had been created.

 
   // Calling a generator returns a generator object.  That object is stored
   // in a temporary variable, a definition that is used by "yield"
   // expressions.
   Variable* temp =
       NewTemporary(ast_value_factory()->dot_generator_object_string());
-  function_state->set_generator_object_variable(temp);
+  function_state_->set_generator_object_variable(temp);
 }
 
 FunctionLiteral* Parser::ParseFunctionLiteral(
     const AstRawString* function_name, Scanner::Location function_name_location,
     FunctionNameValidity function_name_validity, FunctionKind kind,
     int function_token_pos, FunctionLiteral::FunctionType function_type,
     LanguageMode language_mode, bool* ok) {
   // Function ::
   //   '(' FormalParameterList? ')' '{' FunctionBody '}'
   //
@@ skipping 537 matching lines @@
     finally_block->statements()->Add(promise_release, zone());
   }
 
   Statement* try_finally_statement = factory()->NewTryFinallyStatement(
       outer_try_block, finally_block, kNoSourcePosition);
 
   result->statements()->Add(try_finally_statement, zone());
   return result;
 }
 
-Expression* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) {
+Assignment* Parser::BuildCreateJSGeneratorObject(int pos, FunctionKind kind) {
+  // .generator = %CreateGeneratorObject(...);
   DCHECK_NOT_NULL(function_state_->generator_object_variable());
   ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
   args->Add(factory()->NewThisFunction(pos), zone());
   args->Add(IsArrowFunction(kind) ? GetLiteralUndefined(pos)
                                   : ThisExpression(kNoSourcePosition),
             zone());
-  return factory()->NewCallRuntime(Runtime::kCreateJSGeneratorObject, args,
-                                   pos);
+  Expression* allocation =
+      factory()->NewCallRuntime(Runtime::kCreateJSGeneratorObject, args, pos);
+  VariableProxy* proxy =
+      factory()->NewVariableProxy(function_state_->generator_object_variable());
+  return factory()->NewAssignment(Token::INIT, proxy, allocation,
+                                  kNoSourcePosition);
 }
 
 Expression* Parser::BuildResolvePromise(Expression* value, int pos) {
   // %ResolvePromise(.promise, value), .promise
   ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
   args->Add(factory()->NewVariableProxy(PromiseVariable()), zone());
   args->Add(value, zone());
   Expression* call_runtime =
       factory()->NewCallRuntime(Context::PROMISE_RESOLVE_INDEX, args, pos);
   return factory()->NewBinaryOperation(
@@ skipping 22 matching lines @@
   // comes first should create it and stash it in the FunctionState.
   Variable* promise = function_state_->promise_variable();
   if (function_state_->promise_variable() == nullptr) {
     promise = scope()->NewTemporary(ast_value_factory()->empty_string());
     function_state_->set_promise_variable(promise);
   }
   return promise;
 }
 
 Expression* Parser::BuildInitialYield(int pos, FunctionKind kind) {
-  Expression* allocation = BuildCreateJSGeneratorObject(pos, kind);
-  VariableProxy* init_proxy =
-      factory()->NewVariableProxy(function_state_->generator_object_variable());
-  Assignment* assignment = factory()->NewAssignment(
-      Token::INIT, init_proxy, allocation, kNoSourcePosition);
-  VariableProxy* get_proxy =
+  Assignment* assignment = BuildCreateJSGeneratorObject(pos, kind);
+  VariableProxy* generator =
       factory()->NewVariableProxy(function_state_->generator_object_variable());
   // The position of the yield is important for reporting the exception
   // caused by calling the .throw method on a generator suspended at the
   // initial yield (i.e. right after generator instantiation).
-  return factory()->NewYield(get_proxy, assignment, scope()->start_position(),
+  return factory()->NewYield(generator, assignment, scope()->start_position(),
                              Yield::kOnExceptionThrow);
 }
 
 ZoneList<Statement*>* Parser::ParseFunction(
     const AstRawString* function_name, int pos, FunctionKind kind,
     FunctionLiteral::FunctionType function_type,
     DeclarationScope* function_scope, int* num_parameters, int* function_length,
     bool* has_duplicate_parameters, int* materialized_literal_count,
     int* expected_property_count, bool* ok) {
   ParsingModeScope mode(this, allow_lazy_ ? PARSE_LAZILY : PARSE_EAGERLY);
 
   FunctionState function_state(&function_state_, &scope_state_, function_scope);
 
   DuplicateFinder duplicate_finder;
   ExpressionClassifier formals_classifier(this, &duplicate_finder);
 
-  if (IsGeneratorFunction(kind)) PrepareGeneratorVariables(&function_state);
+  if (IsResumableFunction(kind)) PrepareGeneratorVariables();

[adamk, 2016/12/09 03:12:31]

Why wasn't this needed before?

[Dan Ehrenberg, 2016/12/09 06:02:21]

I believe because the logic was (duplicated) in PrepareAsyncFunctionBody.

[neis, 2016/12/09 10:54:52]

Yes.

 
   ParserFormalParameters formals(function_scope);
   ParseFormalParameterList(&formals, CHECK_OK);
   Expect(Token::RPAREN, CHECK_OK);
   int formals_end_position = scanner()->location().end_pos;
   *num_parameters = formals.num_parameters();
   *function_length = formals.function_length;
 
   CheckArityRestrictions(formals.arity, kind, formals.has_rest,
                          function_scope->start_position(), formals_end_position,
@@ skipping 994 matching lines @@
     expr = factory()->NewBinaryOperation(Token::COMMA, expr, args->at(i),
                                          expr->position());
   }
   return expr;
 }
 
 // This method intoduces the line initializing the generator object
 // when desugaring the body of async_function.
 void Parser::PrepareAsyncFunctionBody(ZoneList<Statement*>* body,
                                       FunctionKind kind, int pos) {
-  // function async_function() {
-  //   .generator_object = %CreateGeneratorObject();
-  //   BuildRejectPromiseOnException({
-  //     ... block ...
-  //     return %ResolvePromise(.promise, expr), .promise;
-  //   })
-  // }
-
-  Variable* temp =
-      NewTemporary(ast_value_factory()->dot_generator_object_string());
-  function_state_->set_generator_object_variable(temp);
-
-  Expression* init_generator_variable = factory()->NewAssignment(
-      Token::INIT, factory()->NewVariableProxy(temp),
-      BuildCreateJSGeneratorObject(pos, kind), kNoSourcePosition);
-  body->Add(factory()->NewExpressionStatement(init_generator_variable,
-                                              kNoSourcePosition),
+  if (function_state_->generator_object_variable() == nullptr) {

[adamk, 2016/12/09 03:12:31]

How would this be already-initialized at this point?

[Dan Ehrenberg, 2016/12/09 06:02:21]

Wondering the same

[neis, 2016/12/09 10:54:52]

It is initialized by the PrepareGeneratorVariables above when we enter via
ParseFunction -> ParseEagerFunctionBody -> ParseAsyncFunctionBody.

+    PrepareGeneratorVariables();
+  }
+  body->Add(factory()->NewExpressionStatement(
+                BuildCreateJSGeneratorObject(pos, kind), kNoSourcePosition),
             zone());
 }
 
 // This method completes the desugaring of the body of async_function.
 void Parser::RewriteAsyncFunctionBody(ZoneList<Statement*>* body, Block* block,
                                       Expression* return_value, bool* ok) {
   // function async_function() {
   //   .generator_object = %CreateGeneratorObject();
   //   BuildRejectPromiseOnException({
   //     ... block ...
@@ skipping 1247 matching lines @@
 
   return final_loop;
 }
 
 #undef CHECK_OK
 #undef CHECK_OK_VOID
 #undef CHECK_FAILED
 
 }  // namespace internal
 }  // namespace v8