Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Great job! Especially for someone who's new.
This is right, most VM opcodes don't leave IS_INDIRECT zvals, but a few do as you found out. Indeed, we may have no preciser type information, and it must be conservative. It's understandable why this is forgotten.
One note though: your commit doesn't seem linked to your account, is there a typo in your git config email address?
|
@ndossche Yeah there is a typo i accidentally put a first name |
d07e075 to
f2e088e
Compare
|
@ndossche Forgot to put my complete answer i had just waken up: thanks for the technical review and i fixed the commit author issue |
Fixes #24438
the tracing jit can enter a side trace at
SEND_FUNC_ARGafter aFETCH_DIM_FUNC_ARGhas already executed in the parent trace.when regular ssa information isn't available,
zend_jit_trace_build_tssa()initializes live-in temporaries with a fallback type mask that doesn't includeMAY_BE_INDIRECT. this can leave an indirect operand without the corresponding type information.zend_jit_send_ref()relies on that bit to emit the conditional indirect dereference. without it, the jit can handle the temporary incorrectly when a dynamic call switches between by-value and by-reference arguments.the fix adds
MAY_BE_INDIRECTto the temporary fallback mask. it also adds a phpt regression test that alternates between by-value and by-reference calls and verifies that the correct array elements are updated.i tested the change on php 8.5.13-dev using linux x86_64 debug nts builds from the same source revision. the original implementation fails the regression test with an assertion in
zval_get_long_func(), while the patched version passes with the expected values.the full
ext/opcache/tests/jit/suite completed with 515 passes, no failures and 10 skips due to missing extensions or platform requirements.i also verified that the original reproducer produces the same warnings as the interpreter after the fix. additional checks involving existing references, array aliasing and copy-on-write behavior matched interpreter results.
the original issue reproduces on php 8.4.28-dev as well. the patch was developed and validated on php 8.5; php 8.4 patch validation is not yet included in these results.
release, zts and other architectures haven't been tested locally.