Let a JIT'd isset() reach __isset() on an unset property - #33
Open
EdmondDantes wants to merge 1 commit into
Open
EdmondDantes wants to merge 1 commit into
EdmondDantes wants to merge 1 commit into
Conversation
The tracing JIT knows the offset of a declared property and reads its slot directly, deferring the IS_UNDEF check to the result type guard that follows. The guard admits IS_UNDEF only for ZEND_FETCH_OBJ_IS with a NULL result, and its deoptimization resumes at the next opline with the slot copied into the result rather than re-running the fetch. A property removed by unset() and served by __isset()/__get() traces to the type those return, so IS_UNDEF fails the guard and isset($obj->prop[$key]) answers false for a key the array holds, with the magic handler never called. Check IS_UNDEF before the fetch where the guard cannot stand in for it. FETCH_OBJ_R defers the same check and its deoptimization has the same shape, but no script made it answer wrongly, so its condition is left alone. Reported as true-async/php-async#223, where Laravel's Blade emitted an @once block twice under concurrent renders. Reproduces on upstream master, which carries the same condition. php#223
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
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 true-async/php-async#223.
What answers wrongly
isset($obj->prop[$key])returns false for a key the array holds, whenpropis adeclared property removed by
unset()and served by&__get()/__isset(), and thetracing JIT has compiled the accessor.
opcache.jit=functionand no JIT answercorrectly. Found in Laravel's Blade under
laravel-spawn: the view factory keeps itsper-request render state exactly this way, so
@onceblocks were emitted twice underconcurrent renders.
Neither coroutines nor fibers are needed.
ext/opcache/tests/jit/fetch_obj_is_unset_prop.phpt,added here, is 40 lines of plain PHP and fails on every run before this change.
Why
With a known
prop_infothe JIT reads the property slot at its offset and, on a hottrace, defers the
IS_UNDEFcheck to the result type guard that follows. That guardadmits
IS_UNDEFonly forZEND_FETCH_OBJ_ISwith a NULL result, and itsdeoptimization exits at
opline + 1with the slot copied into the result rather thanre-running the fetch. A property served by
__get()traces to the type the handlerreturns, so an
IS_UNDEFslot fails the guard, the deoptimization answers from theempty slot, and the handler is never called. The trace dump shows both halves:
The recorded slot is
undefwhile the inferred result isarray; the side trace picksup at
ISSET_ISEMPTY_DIM_OBJwith the value already turned into null.The change
Emit the
IS_UNDEFguard before the fetch where the result type guard cannot stand infor it, which is
ZEND_FETCH_OBJ_ISwith a non-NULL traced result. The guard exits atopline, so the VM re-runs the fetch and reaches the magic handler.ZEND_FETCH_OBJ_Rdefers the same check and its deoptimization has the same shape, butno script made it answer wrongly here, so its condition is untouched.
Upstream
php/php-srcmaster carries the identical condition atext/opcache/jit/zend_jit_ir.c:14534, so this is an upstream defect rather than a forkregression. Sending it there is a separate step.
Checks
Run from this tree, ZTS debug build:
ext/opcache/tests/jit/fetch_obj_is_unset_prop.phpt— fails on the same tree with thiscommit reverted and the JIT rebuilt, passes with it.
ext/opcache/tests/jit— 488 passed, 0 failed.ext/opcache/tests— 908 passed, 1 failed:zend_version.phpt, which fails on thereverted build too.
Zend/testswithopcache.jit=tracing— 5414 passed, 2 failed:arginfo_zpp_mismatch.phptandarginfo_zpp_mismatch_strict.phpt, both of which failwith the JIT off as well.