Fix GH-23628: Tracing JIT reads undefined property slots of lazy proxies and unset properties - #23640
Conversation
…roxies A lazy proxy keeps its own property slots IS_UNDEF|IS_PROP_LAZY even after it has been initialized, and the object handlers forward every property access to the real instance. The tracing JIT was not aware of this in two places: 1. When the recorded trace contained a FETCH_OBJ_R/IS/W on a known property whose slot was IS_UNDEF, the known-offset fast path was still compiled. For a lazy proxy this path never succeeds, and it deoptimized on every execution. Use the generic code path (that falls back to the object handlers for undefined slots) when the slot was IS_UNDEF at recording time. This also covers uninitialized and unset properties. 2. During deoptimization of a failed result type guard after FETCH_OBJ_IS, an IS_UNDEF slot was turned into NULL, assuming an undefined property. For a slot flagged IS_PROP_LAZY the fetch has to be forwarded to the real instance instead, so re-execute the opline in the VM, the same way it is already done for FETCH_OBJ_R. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECekMqERF8jnqBxo1cXe3V
|
For discussion: the compile-side change also routes uninitialized or unset typed properties through the generic path when the trace recorded the slot as undefined. That's a deliberate trade, since the fast path could never succeed in that state. |
|
It looks right to me, tried it on 8.5 branch and it does address the issue based on tests |
There was a problem hiding this comment.
Hi @bukka, @Girgias, @ndossche, please review when you have time, it is a blocker for most of the people.
I independently checked this on a debug NTS build. PHP-8.4 has the same bug: both tests fail there without the fix, and the patch applies cleanly and fixes them. Since 8.4 still gets bug fixes, this probably should target PHP-8.4 and be merged up.
Lazy ghosts are affected too. Without the fix, a trace compiled on a plain Table and run on an uninitialized newLazyGhost() returns 92 from parse(100) instead of 100. A test case next to the proxy one in gh23628_002.phpt would cover it.
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
Good fix, I'd just maybe trim the comments a bit
|
not entirely no please do not merge, let s wait Arnaud review even I can see it does not fully address the issue (I might be wrong of course). |
Yeah, let's wait, I looked at to me it appeared fairly complete, but perhaps a 2nd look is an order, but seemed right |
IS_PROP_UNINIT is set on declared properties that are not initialized. Those skip magic methods. Declared properties that were unset() also need to be excluded here, so we evaluate a potential __isset / __get by repeating the opcode Co-authored-by: Arnaud Le Blanc <365207+arnaud-lb@users.noreply.github.com>
|
PS. Should I also strip extra comments from the code that Claude has added? |
|
Should I also address the @lazerg comment and re-target this PR to the 8.4 branch and then we will have cascade merge? |
- Update the deoptimization comments for the IS_PROP_UNINIT check: only uninitialized declared properties skip the magic methods, unset() properties and lazy object slots re-execute the opcode in the VM - Trim the explanatory comments - Add a test for a declared property that was unset() and is served by __isset()/__get() after the trace was compiled - Add an uninitialized lazy ghost case to gh23628_002.phpt Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECekMqERF8jnqBxo1cXe3V
Please, they mostly seem redundant to me |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECekMqERF8jnqBxo1cXe3V
arnaud-lb
changed the title
|
Thank you! |
* PHP-8.5: Fix phpGH-23628: Tracing JIT reads undefined property slots of lazy proxies and unset properties (php#23640)
5 participants
A lazy proxy keeps its own property slots IS_UNDEF|IS_PROP_LAZY even after it has been initialized, and the object handlers forward every property access to the real instance.
This caused the #23628 bug, because the tracing JIT was not aware of this in two places:
When the recorded trace contained a FETCH_OBJ_R/IS/W on a known property whose slot was IS_UNDEF, the known-offset fast path was still compiled. For a lazy proxy this path never succeeds, and it deoptimized on every execution. Use the generic code path (that falls back to the object handlers for undefined slots) when the slot was IS_UNDEF at recording time. This also covers uninitialized and unset properties.
During deoptimization of a failed result type guard after FETCH_OBJ_IS, an IS_UNDEF slot was turned into NULL, assuming an undefined property. For a slot flagged IS_PROP_LAZY the fetch has to be forwarded to the real instance instead, so re-execute the opline in the VM, the same way it is already done for FETCH_OBJ_R.