Skip to content

Fixing stack balance under generator/yield #1502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 27, 2016
Merged

Fixing stack balance under generator/yield #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/Runtime/Language/InterpreterStackFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,7 @@ namespace Js
newInstance->m_flags = InterpreterStackFrameFlags_None;
newInstance->closureInitDone = false;
newInstance->isParamScopeDone = false;
newInstance->shouldCacheSP = true;
#if ENABLE_PROFILE_INFO
newInstance->switchProfileMode = false;
newInstance->isAutoProfiling = false;
Expand Down Expand Up @@ -6744,7 +6745,10 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
// mark the stackFrame as 'in try block'
this->m_flags |= InterpreterStackFrameFlags_WithinTryBlock;

CacheSp();
if (shouldCacheSP)
Copy link
Collaborator

@ThomsonTan ThomsonTan Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldCacheSP [](start = 16, length = 13)

Do we also need flip shouldCacheSP right after this if in case we re-enter ProcessTryFinally?

Copy link
Contributor Author

@akroshg akroshg Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do that, look at the second example in the test file we have two trys so we should not cache-sp in both cases. however say if you have two destructuring pattern in the call-expression we would have to cache sp on both occasion.

Copy link
Collaborator

@ThomsonTan ThomsonTan Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yes, we need ignore cache SP for both ProcessTryFinally for your second test, but how about if there are some more ProcessTryFinally following but which are not for the previous yield? Seems this fix will also ignore caching SP for them.

In reply to: 76500192 [](ancestors = 76500192)

{
CacheSp();
}

if (this->IsInDebugMode())
{
Expand Down Expand Up @@ -6772,10 +6776,10 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
this->m_flags &= ~InterpreterStackFrameFlags_WithinTryBlock;
}

shouldCacheSP = !skipFinallyBlock;

if (skipFinallyBlock)
{
RestoreSp();

// A leave occurred due to a yield
return;
}
Expand Down
1 change: 1 addition & 0 deletions lib/Runtime/Language/InterpreterStackFrame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ namespace Js

bool closureInitDone : 1;
bool isParamScopeDone : 1;
bool shouldCacheSP : 1; // Helps in determining if we need to cache the sp in ProcessTryFinally
#if ENABLE_PROFILE_INFO
bool switchProfileMode : 1;
bool isAutoProfiling : 1;
Expand Down
41 changes: 41 additions & 0 deletions test/es6/iteratorclose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,47 @@ var tests = [
});
}
},
{
name : "BugFix : yielding in the call expression under generator function",
body : function () {
var val = 0;
function bar(a, b, c) { val = b; }
function *foo(d) {
for (var k of [2, 3]) {
bar(1, d ? yield : d, k);
}
}
var iter = foo(true);
iter.next();
iter.next();
iter.next(10);
assert.areEqual(val, 10, "yielding in the call expression under for..of is working correctly");
}
},
{
name : "BugFix : yielding in the call expression under try catch",
body : function () {
var val = 0;
var counter = 0;
function bar(a, b, c) { val = b; }
function *foo(d) {
try {
try {
bar(1, d ? yield : d, 11);
} finally {
counter++;
}
} finally {
counter++;
}
}
var iter = foo(true);
iter.next();
iter.next(10);
assert.areEqual(val, 10, "yielding in the call expression under try/catch is working correctly");
assert.areEqual(counter, 2, "both finally called after yielding");
}
},
];

testRunner.runTests(tests, {
Expand Down