Revision: 14454
Author: [email protected]
Date: Fri Apr 26 04:55:22 2013
Log: 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.
[email protected]
BUG=v8:2355
TEST=mjsunit/harmony/generators-iteration
Review URL: https://codereview.chromium.org/14416011
Patch from Andy Wingo <[email protected]>.
http://code.google.com/p/v8/source/detail?r=14454
Modified:
/branches/bleeding_edge/src/ast.h
/branches/bleeding_edge/src/full-codegen.cc
/branches/bleeding_edge/src/parser.cc
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/src/scopes.cc
/branches/bleeding_edge/test/mjsunit/harmony/generators-iteration.js
=======================================
--- /branches/bleeding_edge/src/ast.h Wed Apr 24 04:32:17 2013
+++ /branches/bleeding_edge/src/ast.h Fri Apr 26 04:55:22 2013
@@ -942,15 +942,18 @@
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 @@
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)
}
=======================================
--- /branches/bleeding_edge/src/full-codegen.cc Wed Apr 24 06:00:16 2013
+++ /branches/bleeding_edge/src/full-codegen.cc Fri Apr 26 04:55:22 2013
@@ -1255,9 +1255,12 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/parser.cc Fri Apr 19 07:11:23 2013
+++ /branches/bleeding_edge/src/parser.cc Fri Apr 26 04:55:22 2013
@@ -2571,7 +2571,7 @@
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);
}
=======================================
--- /branches/bleeding_edge/src/runtime.cc Fri Apr 26 01:52:35 2013
+++ /branches/bleeding_edge/src/runtime.cc Fri Apr 26 04:55:22 2013
@@ -2475,15 +2475,17 @@
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.
=======================================
--- /branches/bleeding_edge/src/scopes.cc Fri Apr 5 06:19:31 2013
+++ /branches/bleeding_edge/src/scopes.cc Fri Apr 26 04:55:22 2013
@@ -726,7 +726,9 @@
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;
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/harmony/generators-iteration.js
Thu Apr 25 03:59:09 2013
+++ /branches/bleeding_edge/test/mjsunit/harmony/generators-iteration.js
Fri Apr 26 04:55:22 2013
@@ -233,6 +233,17 @@
"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(); }
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
