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.
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
Fixing stack balance under generator/yield #1502
Conversation
We were caching and restoring the sp in the processtryfinally in order to balance the stack when we are in the middle of call. However in the case of generator we can yield out of the call. In that case we actually haven't finished the try part. So once we comeback after yield we don't need to cache the stack again. Introduced the flag to capture such state and fix the problem.
@aneeshdk @tcare @ThomsonTan please have a look. |
Looks good |
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 try
s 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.
There was a problem hiding this comment.
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)
Can you also add one test case for async functions too? |
Merge pull request #1502 from akroshg:fix1 We were caching and restoring the sp in the processtryfinally in order to balance the stack when we are in the middle of call. However in the case of generator we can yield out of the call. In that case we actually haven't finished the try part. So once we comeback after yield we don't need to cache the stack again. Introduced the flag to capture such state and fix the problem.
We were caching and restoring the sp in the processtryfinally in order to balance the stack when we are in the middle of call. However in the case of generator we can yield out of the call. In that case we actually haven't finished the try part.
So once we comeback after yield we don't need to cache the stack again. Introduced the flag to capture such state and fix the problem.