Fix setting hasBailout when there are inlined functions in try/catch #4535
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a3b84f6 to
22f4fa4
Compare
rajatd
reviewed
Jan 12, 2018
Contributor
rajatd
left a comment
There was a problem hiding this comment.
Another way to fix the problem could be to set hasBailedOut when we return after interpreting the outermost inlinee to the bailout helper code. Of course, we'll have to pass in the offset for hasBailedOut and that could probably be tricky. How does that sound?
22f4fa4 to
775a263
Compare
Contributor
Author
|
I tried it out - looks much better now. |
Contributor
Author
|
FYI, both approaches will not work as is once we start inlining functions within try/catch/finally. |
Contributor
|
Hmm, yes, unfortunately |
775a263 to
48454b9
Compare
Contributor
Author
|
@Penguinwizzard @agarwal-sandeep Can you help review this PR ? |
48454b9 to
2869b17
Compare
Penguinwizzard
approved these changes
Jan 16, 2018
lib/Runtime/Base/ThreadContext.h
Outdated
| StackProber * stackProber; | ||
| bool isThreadBound; | ||
| bool hasThrownPendingException; | ||
| bool * hasBailedOutBitOffset; |
Contributor
There was a problem hiding this comment.
Nit: This naming is slightly off, I feel, as this isn't really an offset, but is a full-fledged pointer. This becomes more of an issue in cases like https://github.com/Microsoft/ChakraCore/pull/4535/files#diff-b1ac070605da21777f9c4d31b69a2014R162, where you're setting it with a pointer plus an actual offset.
| { | ||
| private: | ||
| void * m_prevTryCatchFrameAddr; | ||
| bool m_prevDoneBailOutEhRegion; |
Contributor
There was a problem hiding this comment.
Nit: is this used in the current version of the changes?
Contributor
Author
There was a problem hiding this comment.
No, removed it. Thanks for catching this.
00221a3 to
cdca4eb
Compare
Fixes OS#15078638 When we bailout executing trycode from within OP_TryCatch, we complete the execution of the current function enclosing the try/catch in the interpreter. If there was an exception within the try region, it is caught and handled accordingly in ProcessTryHandlerBailOut which reconstructs try/catch/finally frames when we bailout midway executing code within OP_TryCatch. If there was an exception outside the try region, the catch of the OP_TryCatch ends up catching it, because it happens to be on the callstack. For this we use the hasBailOut bit which is per function, so we know that this exception has to be passed above. When we have inlined functions inside the try, and for bailouts inside the inlined code, we do not set the hasBailedOut bit, so that the enclosing functions catch in OP_TryCatch catches it. But when we bailout from inlined code inside try, we execute inlined code, as well as the enclosing function's code in the interpreter. We will be execution code past the try/catch of the current function in the interpreter. Now if any code outside the eh region throws, we will catch that in OP_TryCatch which happens to be on the callstack. And we will end up handling it instead of passing above because we have not set the hasBailedOutBit from the bailout point. This change fixes this issue. We pass a pointer to the hasBailedOutBit and set it once we have finished executing the inlined frames and are ready to process the interpreter frame of the current function.
cdca4eb to
dee125d
Compare
chakrabot
pushed a commit
that referenced
this pull request
Jan 17, 2018
…d functions in try/catch Merge pull request #4535 from meg-gupta:fixCatchEatingUpEx Fixes OS#15078638 When we bailout executing trycode from within OP_TryCatch, we complete the execution of the current function enclosing the try/catch in the interpreter. If there was an exception within the try region, it is caught and handled accordingly in ProcessTryHandlerBailOut which reconstructs try/catch/finally frames when we bailout midway executing code within OP_TryCatch. If there was an exception outside the try region, the catch of the OP_TryCatch ends up catching it, because it happens to be on the callstack. For this we use the hasBailOut bit which is per function, so we know that this exception has to be passed above. When we have inlined functions inside the try, and for bailouts inside the inlined code, we do not set the hasBailedOut bit, so that the enclosing functions catch in OP_TryCatch catches it. But when we bailout from inlined code inside try, we execute inlined code, as well as the enclosing function's code in the interpreter. We will be execution code past the try/catch of the current function in the interpreter. Now if any code outside the eh region throws, we will catch that in OP_TryCatch which happens to be on the callstack. And we will end up handling it instead of passing above because we have not set the hasBailedOutBit from the bailout point. This change fixes this issue. We pass a pointer to the hasBailedOutBit and set it once we have finished executing the inlined frames and are ready to process the interpreter frame of the current function.
chakrabot
pushed a commit
that referenced
this pull request
Jan 17, 2018
…are inlined functions in try/catch Merge pull request #4535 from meg-gupta:fixCatchEatingUpEx Fixes OS#15078638 When we bailout executing trycode from within OP_TryCatch, we complete the execution of the current function enclosing the try/catch in the interpreter. If there was an exception within the try region, it is caught and handled accordingly in ProcessTryHandlerBailOut which reconstructs try/catch/finally frames when we bailout midway executing code within OP_TryCatch. If there was an exception outside the try region, the catch of the OP_TryCatch ends up catching it, because it happens to be on the callstack. For this we use the hasBailOut bit which is per function, so we know that this exception has to be passed above. When we have inlined functions inside the try, and for bailouts inside the inlined code, we do not set the hasBailedOut bit, so that the enclosing functions catch in OP_TryCatch catches it. But when we bailout from inlined code inside try, we execute inlined code, as well as the enclosing function's code in the interpreter. We will be execution code past the try/catch of the current function in the interpreter. Now if any code outside the eh region throws, we will catch that in OP_TryCatch which happens to be on the callstack. And we will end up handling it instead of passing above because we have not set the hasBailedOutBit from the bailout point. This change fixes this issue. We pass a pointer to the hasBailedOutBit and set it once we have finished executing the inlined frames and are ready to process the interpreter frame of the current function.
chakrabot
pushed a commit
that referenced
this pull request
Jan 17, 2018
…t when there are inlined functions in try/catch Merge pull request #4535 from meg-gupta:fixCatchEatingUpEx Fixes OS#15078638 When we bailout executing trycode from within OP_TryCatch, we complete the execution of the current function enclosing the try/catch in the interpreter. If there was an exception within the try region, it is caught and handled accordingly in ProcessTryHandlerBailOut which reconstructs try/catch/finally frames when we bailout midway executing code within OP_TryCatch. If there was an exception outside the try region, the catch of the OP_TryCatch ends up catching it, because it happens to be on the callstack. For this we use the hasBailOut bit which is per function, so we know that this exception has to be passed above. When we have inlined functions inside the try, and for bailouts inside the inlined code, we do not set the hasBailedOut bit, so that the enclosing functions catch in OP_TryCatch catches it. But when we bailout from inlined code inside try, we execute inlined code, as well as the enclosing function's code in the interpreter. We will be execution code past the try/catch of the current function in the interpreter. Now if any code outside the eh region throws, we will catch that in OP_TryCatch which happens to be on the callstack. And we will end up handling it instead of passing above because we have not set the hasBailedOutBit from the bailout point. This change fixes this issue. We pass a pointer to the hasBailedOutBit and set it once we have finished executing the inlined frames and are ready to process the interpreter frame of the current function.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes OS#15078638
When we bailout executing trycode from within OP_TryCatch, we complete the execution of the current function enclosing the try/catch in the interpreter.
If there was an exception within the try region, it is caught and handled accordingly in ProcessTryHandlerBailOut which reconstructs try/catch/finally frames
when we bailout midway executing code within OP_TryCatch. If there was an exception outside the try region, the catch of the OP_TryCatch ends up catching it,
because it happens to be on the callstack. For this we use the hasBailOut bit which is per function, so we know that this exception has to be passed above.
When we have inlined functions inside the try, and for bailouts inside the inlined code, we do not set the hasBailedOut bit, so that the enclosing functions catch in OP_TryCatch catches it.
But when we bailout from inlined code inside try, we execute inlined code, as well as the enclosing function's code in the interpreter.
We will be execution code past the try/catch of the current function in the interpreter. Now if any code outside the eh region throws,
we will catch that in OP_TryCatch which happens to be on the callstack. And we will end up handling it instead of passing above because we have not set the hasBailedOutBit from the bailout point.
This change fixes this issue. We pass a pointer to the hasBailedOutBit and set it once we have finished executing the inlined frames and are ready to process the interpreter frame of the current function.