Smoke me/davem/exception issues #19778
Closed
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
GH #19680 Normally in code like eval {.... }; then even if the eval is the last statement in the file or sub, the OP_LEAVETRY isn't the last op in the execution path: it's followed by an OP_LEAVE or OP_LEAVESUB or whatever, which will be the op to resume execution from after an exception is caught. However, if the eval is the *last* thing within a regex code block: /(?{ ...; eval {....}; })/ then the op_next pointer of the OP_LEAVETRY op is actually NULL. This confused S_docatch(), which wrongly assumed that a NULL PL_restartop indicated that the caught exception should be rethrown, popping execution back to the outer perl_run() call and hence leading to the confused panic warning: "panic: restartop in perl_run" The fix is to to separate out the "do we need to re-throw" test, (PL_restartjmpenv != PL_top_env), from the "no more ops so no need to re-enter the runops loop" test, (!PL_restartop).
Document fully what S_docatch() does, and remove the one macro which calls it, replacing all the call-sites with a direct call to docatch(). docatch() is a core part of perl's internal exception handling, and as such is conceptually complex. It was made even more complex by v5.27.0-75-gd7e3f70f30, which causes functions like pp_entertry() to recursively call themselves sometimes. But this detail was hidden by the RUN_PP_CATCHABLY() macro, which included a hidden 'return'. By unwrapping this trivial macro, it makes the flow of control much easier to understand. I've also added an extra assert(!CATCH_GET), to emphasise that this setting is reset following the JMPENV_PUSH() call. Apart from that, there should be no functional differences.
I first created this section back in 2005. Some things have changed since then, and my understanding of some details has improved. So this commit is a major rewrite of this section to make it more comprehensible, as well as lots of little pod fixups.
Although one of the macros associated with the JMPENV facility is inconsistently called JMPENV_JUMP(), fix all code comments and debugging output to eliminate any references to JUMPENV.
This test file didn't have any comments explaining what it was for: especially confusing as there's no 'catch' op.
The comment explaining what the je_mustcatch field does was incorrectly updated in 1999! Change it back to what it was originally.
hvds
reviewed
May 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fix
panic: restartop in perl_run(Issue #19680),then do tweaks and doc improvements to perl's internal exception handling