gh-155151: Check the recursion limit in CALL_EX_PY and CALL_KW_BOUND_METHOD - #155272
Closed
tekinertekin wants to merge 1 commit into
Closed
gh-155151: Check the recursion limit in CALL_EX_PY and CALL_KW_BOUND_METHOD#155272tekinertekin wants to merge 1 commit into
tekinertekin wants to merge 1 commit into
Conversation
…BOUND_METHOD The generic CALL_FUNCTION_EX and CALL_KW opcodes reach start_frame, which calls _Py_EnterRecursivePy() and raises RecursionError before the callee runs. Their specialized forms end in _PUSH_FRAME, which decrements py_recursion_remaining without checking it, so once a call site had been warmed up the callee was entered and returned normally where the unspecialized instruction raised. Warming a call site should not change whether the target function executes. CALL_KW_PY and CALL_BOUND_METHOD_EXACT_ARGS already guard their frame push with _CHECK_RECURSION_REMAINING; add it to these two as well. It deopts, so it goes where a deopt is already safe: after the existing flush in CALL_KW_BOUND_METHOD, following the pattern of CALL_BOUND_METHOD_EXACT_ARGS, and after _CHECK_IS_PY_CALLABLE_EX in CALL_EX_PY, which already exits at that point.
Member
|
The description of this PR and #155269 is almost identical, and thus even the description was probably generated entirely by an agent. In particular, I consider this as an excessive use of agents and will reject this PR especially since it touches the core interpreter. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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 #155151.
The generic
CALL_FUNCTION_EXandCALL_KWreachstart_frame, which calls_Py_EnterRecursivePy()and raisesRecursionErrorbefore the callee runs.Their specialized forms end in
_PUSH_FRAME, which decrementspy_recursion_remainingwithout checking it. So once a call site had beenwarmed up, the callee was entered and returned normally where the
unspecialized instruction raised — warming a call site changed whether the
target function executed.
CALL_KW_PYandCALL_BOUND_METHOD_EXACT_ARGSalready guard their frame pushwith
_CHECK_RECURSION_REMAINING. This adds it to the two that were missingit.
Where the check goes
_CHECK_RECURSION_REMAININGdeopts, so it cannot follow a uop that hasalready mutated the stack. Both placements sit where a deopt is already safe:
CALL_KW_BOUND_METHOD: after the existingflushthat follows_EXPAND_METHOD_KW. This is the patternCALL_BOUND_METHOD_EXACT_ARGSuses, where the same
flushcarries the comment "In case the followingdeopt" ahead of
_CHECK_STACK_SPACEand_CHECK_RECURSION_REMAINING.CALL_EX_PY: after_CHECK_IS_PY_CALLABLE_EX, which alreadyEXIT_IFs atthat point.
No new uop is introduced —
_CHECK_RECURSION_REMAININGis already present inexecutor_cases.c.handoptimizer_cases.c.h, andmake regen-casesleftboth files unchanged.
Verification
Built from
main(free-threaded,--with-pydebug, TSAN) on macOS/arm64.The reporter's four-case reproducer, before and after:
CALL_FUNCTION_EX(control)CALL_EX_PY(specialized)CALL_KW(control)CALL_KW_BOUND_METHOD(specialized)The call sites still specialize afterwards —
dis(..., adaptive=True)stillreports
CALL_EX_PYandCALL_KW_BOUND_METHOD, so this adds the check ratherthan suppressing the specialization.
Two regression tests were added next to
test_recursion_check_for_general_callsin
test_opcache.py. They warm the call site, assert it specialized, then makethe call from the deepest live frame at the recursion limit and assert the
target never ran. Both fail on unpatched
mainwithAssertionError: RecursionError not raised. The targets bump the counter with plain bytecoderather than through another call, so reaching them cannot be masked by a second
recursion check.
test_opcachepasses (84 tests), including under-R 3:3.test_call,test_dis,test_generated_cases,test_monitoring,test_optimizer,test_sys_settraceandtest_capipass.Tools/patchcheckis clean.I did not build with
--enable-experimental-jitlocally, so the JIT job herewill be the first check of the tier 2 side.
AI tools were used on this PR: Claude Code (Opus) reproduced the issue, wrote
the patch and the tests, and ran the verification above. I reviewed the change,
understand it and can explain it, and I take responsibility for it.