Repository navigation
Conversation
Assisted-by: Claude
ndossche
approved these changes
Oct 25, 2026
Member
|
cc @iliaal Can you double check please |
iliaal
approved these changes
Oct 25, 2026
iliaal
left a comment
Member
There was a problem hiding this comment.
Was a bit thrown off by the removal as well, but it is a correct change.
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.
@ndossche here is the patch I offered in the issue, as you asked.
With
opcache.jit=tracing, a trace likesink($x = $y + 2)can be recorded while$xis a reference to an untyped property, then run while$xis a reference to a typedfloat(orstring) property. When that happens,sink()receives an int holding the raw bits of the converted value:int(4617315517961601024)instead offloat(5), and for astringproperty an unrelated int (int(1097804056)in my run) instead ofstring(1) "5".When an
ASSIGNresult goes straight into the followingSEND_VAL, the tracing JIT fuses the two. The result is written directly into the callee's argument slot, theSEND_VALis skipped, and the callee's parameter takes the type recorded for theSEND_VALoperand. That type was never checked. It's fine when the result is a copy of the guarded op2, but the typed reference helper can coerce the value, so the recorded type may be wrong. The fix stops fusing in that case: when op1 is a guarded reference (ref_addris set), the result goes to its own slot as usual, andSEND_VALchecks its type and side-exits if it doesn't match. The reference branch also had a second copy of the result address / send-fusion code, which the shared code right after it already does. I removed that copy so there is only one place to apply the condition.The new test
ext/opcache/tests/jit/gh24436.phptruns the same call site with untyped, typedfloatand typedstringproperty references. It fails without the patch and passes with it, both with run-tests' defaults and with CI's tracing JIT options. The reproducer from the issue, run with the command line given there, also printsint(4617315517961601024)before the patch andfloat(5)after.ext/opcache/testspasses in both tracing and function JIT modes. I based this on PHP-8.4 since that's the branch I reproduced and tested on.Fixes #24436.
Found with an LLM-based testing tool; I wrote this fix with help from Claude and reviewed it myself.