gh-150587: Factor non-negative compact int subscript guards#153004
gh-150587: Factor non-negative compact int subscript guards#153004KRRT7 wants to merge 3 commits into
Conversation
24dd20d to
f1a230b
Compare
Fidget-Spinner
left a comment
There was a problem hiding this comment.
LGTM, thanks for catching this bug, just one request.
|
This PR is marked as fixing #150587, but that issue is closed as not planned. What is this fixing? |
|
@markshannon that is my own issue, the original issue was for a much bigger PR and I've since then, scoped it down, will update the body / title. |
|
Wait upon closer inspection, the current code already deopts in the subscript op if it sees a negative index. I think there's no bug here? Correct me if I'm wrong. Also GUARD_TOS_INT checks for a compact long already, then the subscript op itself checks for the range . |
|
You were right about the original bug claim: after rechecking the guard/specialization path, the non-compact-index issue I described is not actually reachable. I’ve kept the branch but narrowed it into a refactor only: factor the exact non-negative compact-int check into a shared guard uop for the specialized subscript/store-subscript paths that already depend on that invariant, without increasing their uop counts. I reran the focused |
This PR is now a refactor/cleanup, not a bug fix.
The original version assumed there was a correctness issue around non-compact
intindices reaching specialized subscript paths. That turned out to be incorrect: the existing specialization and guard structure already prevents that case from reaching_PyLong_CompactValue()unsafely.What this PR does now is factor the "exact non-negative compact int" check into a shared guard uop and use it in the specialized subscript paths that already rely on that invariant:
BINARY_OP_SUBSCR_LIST_INTBINARY_OP_SUBSCR_STR_INTBINARY_OP_SUBSCR_USTR_INTBINARY_OP_SUBSCR_TUPLE_INTSTORE_SUBSCR_LIST_INTThe refactor keeps the specialized opcode expansions at the same uop counts as before by replacing
_GUARD_TOS_INTwhere appropriate, rather than stacking an additional guard on top.Tests run:
env PYTHONPATH=build/lib.macosx-26.5-arm64-3.16 ./python.exe -m test test_opcache -m test_binary_subscr -m test_store_subscr -venv PYTHONPATH=build/lib.macosx-26.5-arm64-3.16 PYTHON_JIT=1 ./python.exe -m test test_opcache -m test_binary_subscr -m test_store_subscr -v