gh-151321: Fix opcode metadata flags for LOAD_DEREF and LOAD_CLOSURE - #151386
gh-151321: Fix opcode metadata flags for LOAD_DEREF and LOAD_CLOSURE#151386harjothkhara wants to merge 2 commits into
Conversation
…OSURE LOAD_DEREF and LOAD_CLOSURE were missing HAS_FREE_FLAG in the generated opcode metadata, and both also carried HAS_LOCAL_FLAG, so dis.hasfree no longer reported them while dis.haslocal wrongly included them. This regressed the documented dis.hasfree / dis.haslocal contract: LOAD_CLOSURE broke in 3.13 (when it became a pseudo-op inheriting LOAD_FAST's flags) and LOAD_DEREF broke in 3.14 (when it started using _PyCell_GetStackRef, which the cases generator's has_free heuristic did not recognize). Teach the cases generator to treat _PyCell_GetStackRef as a free-variable access, and give the LOAD_CLOSURE pseudo an explicit HAS_FREE flag. A pseudo that explicitly accesses a free variable now suppresses the inherited HAS_LOCAL, mirroring the existing "uses_locals and not has_free" rule for real instructions, so HAS_FREE and HAS_LOCAL are never both set. The change is to introspection metadata only; the generated interpreter and optimizer code are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a0a2d2f to
9ea5992
Compare
|
The remaining CI failure appears unrelated to this PR. The failing job is Windows (free-threading) / Build and test (x64, switch-case): It failed in: This PR only changes opcode metadata/generation and test__opcode. I also reran the focused test locally after updating the branch onto current main, and it passed 6/6 times on my local macOS debug build. Could someone with Actions permissions rerun the failed Windows job? |
|
CI often fails, and it is generally unnecessary to deal with the failure that ensure unrelated |
|
@markshannon, this PR has been open about a week. Would you mind taking a look when you get a chance? |
|
@iritkatriel would you mind taking a look at this one? gh-151321: on main |
|
Closing in favour of #154778. |
3 participants
LOAD_DEREFandLOAD_CLOSUREwere missingHAS_FREE_FLAGin the generated opcode metadata, and both also carriedHAS_LOCAL_FLAG. As a resultdis.hasfreestopped reporting them anddis.haslocalwrongly included them, breaking the documenteddis.hasfree/dis.haslocalcontract.LOAD_CLOSUREregressed in 3.13 (it became a pseudo-op inheritingLOAD_FAST's flags);LOAD_DEREFregressed in 3.14 (it started using_PyCell_GetStackRef, which the cases generator'shas_freeheuristic didn't recognize). This restores the 3.12 classification.The fix teaches the cases generator that
_PyCell_GetStackRefis a free-variable access, and gives theLOAD_CLOSUREpseudo an explicitHAS_FREE. A pseudo that explicitly accesses a free variable now suppresses the inheritedHAS_LOCAL, mirroring the existinguses_locals and not has_freerule. Only introspection metadata changes — the generated interpreter/optimizer code is unchanged.After the fix
dis.hasfreecontains both ops anddis.haslocalcontains neither.make regen-allproduces no other churn, andtest__opcode test_dis test_compilepass.Backport note: this won't cherry-pick cleanly to 3.13 — there
LOAD_DEREFstill usesPyCell_GetRef(already detected correctly), so 3.13 needs aLOAD_CLOSURE-only manual backport. 3.14 takes the full diff. Requesting backport to 3.14 and 3.13.Disclosure: I used AI assistance on this; I've reviewed it and can walk through the generator change.