Fix yield inside with

This patch makes it so that suspending generators always saves the
context.  Previously we erroneously assumed that if the operand stack
was empty, that the context would be unchanged, but that is not the case
with "with".

Fixing this brought out an interesting bug in the variable allocator.
Yield inside with will reference a context-allocated temporary holding
the generator object.  Before the fix, this object was looked up in the
with context instead of the function context, because with contexts were
not being simulated during full-codegen.  Previously this was OK as all
variables would be given LOOKUP allocation instead of CONTEXT, but the
context-allocated temporary invalidated this assumption.  The fix is to
simulate the context chain more accurately in full-codegen.

R=mstarzinger@chromium.org
BUG=v8:2355
TEST=mjsunit/harmony/generators-iteration

Review URL: https://codereview.chromium.org/14416011

Patch from Andy Wingo <wingo@igalia.com>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14454 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/ast.h b/src/ast.h
index 2d5c200..10ae7de 100644 (file)
--- a/src/ast.h
+++ b/src/ast.h
@@ -942,15 +942,18 @@ class WithStatement: public Statement {
  public:
   DECLARE_NODE_TYPE(WithStatement)
 
+  Scope* scope() { return scope_; }
   Expression* expression() const { return expression_; }
   Statement* statement() const { return statement_; }
 
  protected:
-  WithStatement(Expression* expression, Statement* statement)
-      : expression_(expression),
+  WithStatement(Scope* scope, Expression* expression, Statement* statement)
+      : scope_(scope),
+        expression_(expression),
         statement_(statement) { }
 
  private:
+  Scope* scope_;
   Expression* expression_;
   Statement* statement_;
 };
@@ -2787,9 +2790,11 @@ class AstNodeFactory BASE_EMBEDDED {
     VISIT_AND_RETURN(ReturnStatement, stmt)
   }
 
-  WithStatement* NewWithStatement(Expression* expression,
+  WithStatement* NewWithStatement(Scope* scope,
+                                  Expression* expression,
                                   Statement* statement) {
-    WithStatement* stmt = new(zone_) WithStatement(expression, statement);
+    WithStatement* stmt = new(zone_) WithStatement(
+        scope, expression, statement);
     VISIT_AND_RETURN(WithStatement, stmt)
   }
 

diff --git a/src/full-codegen.cc b/src/full-codegen.cc
index 81f4c1d..dc646b1 100644 (file)
--- a/src/full-codegen.cc
+++ b/src/full-codegen.cc
@@ -1255,9 +1255,12 @@ void FullCodeGenerator::VisitWithStatement(WithStatement* stmt) {
   __ CallRuntime(Runtime::kPushWithContext, 2);
   StoreToFrameField(StandardFrameConstants::kContextOffset, context_register());
 
+  Scope* saved_scope = scope();
+  scope_ = stmt->scope();
   { WithOrCatch body(this);
     Visit(stmt->statement());
   }
+  scope_ = saved_scope;
 
   // Pop context.
   LoadContextField(context_register(), Context::PREVIOUS_INDEX);

diff --git a/src/parser.cc b/src/parser.cc
index 23fa9fe..267b872 100644 (file)
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2571,7 +2571,7 @@ Statement* Parser::ParseWithStatement(ZoneStringList* labels, bool* ok) {
     stmt = ParseStatement(labels, CHECK_OK);
     with_scope->set_end_position(scanner().location().end_pos);
   }
-  return factory()->NewWithStatement(expr, stmt);
+  return factory()->NewWithStatement(with_scope, expr, stmt);
 }
 
 

diff --git a/src/runtime.cc b/src/runtime.cc
index f7bd598..ebe88fe 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -2475,16 +2475,18 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_SuspendJSGeneratorObject) {
     ASSERT_EQ(generator_object->operand_stack(),
               isolate->heap()->empty_fixed_array());
     // If there are no operands on the stack, there shouldn't be a handler
-    // active either.  Also, the active context will be the same as the function
-    // itself, so there is no need to save the context.
-    ASSERT_EQ(frame->context(), generator_object->context());
+    // active either.
     ASSERT(!frame->HasHandler());
   } else {
-    generator_object->set_context(Context::cast(frame->context()));
     // TODO(wingo): Save the operand stack and/or the stack handlers.
     UNIMPLEMENTED();
   }
 
+  // It's possible for the context to be other than the initial context even if
+  // there is no stack handler active.  For example, this is the case in the
+  // body of a "with" statement.  Therefore we always save the context.
+  generator_object->set_context(Context::cast(frame->context()));
+
   // The return value is the hole for a suspend return, and anything else for a
   // resume return.
   return isolate->heap()->the_hole_value();

diff --git a/src/scopes.cc b/src/scopes.cc
index 10548f9..5ad970a 100644 (file)
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -726,7 +726,9 @@ int Scope::ContextChainLength(Scope* scope) {
   int n = 0;
   for (Scope* s = this; s != scope; s = s->outer_scope_) {
     ASSERT(s != NULL);  // scope must be in the scope chain
-    if (s->num_heap_slots() > 0) n++;
+    if (s->is_with_scope() || s->num_heap_slots() > 0) n++;
+    // Catch scopes always have heap slots.
+    ASSERT(!s->is_catch_scope() || s->num_heap_slots() > 0);
   }
   return n;
 }

diff --git a/test/mjsunit/harmony/generators-iteration.js b/test/mjsunit/harmony/generators-iteration.js
index 5d41fc2..bc0bde0 100644 (file)
--- a/test/mjsunit/harmony/generators-iteration.js
+++ b/test/mjsunit/harmony/generators-iteration.js
@@ -233,6 +233,17 @@ TestGenerator(
     "foo",
     [1, 2, undefined]);
 
+TestGenerator(
+    function* g() {
+      var x = 1;
+      yield x;
+      with({x:2}) { yield x; }
+      yield x;
+    },
+    [1, 2, 1, undefined],
+    "foo",
+    [1, 2, 1, undefined]);
+
 function TestRecursion() {
   function TestNextRecursion() {
     function* g() { yield iter.next(); }