JIT: Snapshotted poly_func / poly_this may be spilled#18408
Closed
arnaud-lb wants to merge 4 commits intophp:PHP-8.4from
Closed
JIT: Snapshotted poly_func / poly_this may be spilled#18408arnaud-lb wants to merge 4 commits intophp:PHP-8.4from
arnaud-lb wants to merge 4 commits intophp:PHP-8.4from
Conversation
arnaud-lb
commented
Apr 23, 2025
Comment on lines
-627
to
+632
| if (t->exit_count > 0 | ||
| && jit->ctx.ir_base[addr].val.u64 == (uintptr_t)zend_jit_trace_get_exit_addr(t->exit_count - 1)) { | ||
| exit_point = t->exit_count - 1; | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; | ||
| } | ||
| exit_point = zend_jit_exit_point_by_addr((void*)(uintptr_t) jit->ctx.ir_base[addr].val.u64); | ||
| ZEND_ASSERT(exit_point != -1); | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; |
Member
Author
There was a problem hiding this comment.
addr is not always the one of the last exit point, e.g. when more exit points are created before an earlier exit point is actually used in a guard:
exit_addr = zend_jit_trace_get_exit_addr(exit_point);
...
if (condition) {
int32_t exit_point2 = zend_jit_trace_get_exit_point(jit, opline, 0);
const void *exit_addr2 = zend_jit_trace_get_exit_addr(exit_point2);
...
}
...
ir_GUARD(..., ir_CONST_ADDR(exit_addr)); // not the last exit point
In this case, exit_point was left initialized to 0 and we updated the wrong exit infos.
Member
There was a problem hiding this comment.
Also can't you do zend_jit_exit_point_by_addr(ptr); ?
ndossche
reviewed
Apr 25, 2025
ext/opcache/jit/zend_jit_ir.c
Outdated
| return rs; | ||
| } | ||
|
|
||
| bool zend_jit_ref_snapshot_equals(zend_jit_ref_snapshot *a, zend_jit_ref_snapshot *b) |
Comment on lines
-627
to
+632
| if (t->exit_count > 0 | ||
| && jit->ctx.ir_base[addr].val.u64 == (uintptr_t)zend_jit_trace_get_exit_addr(t->exit_count - 1)) { | ||
| exit_point = t->exit_count - 1; | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; | ||
| } | ||
| exit_point = zend_jit_exit_point_by_addr((void*)(uintptr_t) jit->ctx.ir_base[addr].val.u64); | ||
| ZEND_ASSERT(exit_point != -1); | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; |
Comment on lines
-627
to
+632
| if (t->exit_count > 0 | ||
| && jit->ctx.ir_base[addr].val.u64 == (uintptr_t)zend_jit_trace_get_exit_addr(t->exit_count - 1)) { | ||
| exit_point = t->exit_count - 1; | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; | ||
| } | ||
| exit_point = zend_jit_exit_point_by_addr((void*)(uintptr_t) jit->ctx.ir_base[addr].val.u64); | ||
| ZEND_ASSERT(exit_point != -1); | ||
| if (t->exit_info[exit_point].flags & ZEND_JIT_EXIT_METHOD_CALL) { | ||
| n = 2; |
Member
There was a problem hiding this comment.
Also can't you do zend_jit_exit_point_by_addr(ptr); ?
dstogov
approved these changes
Apr 28, 2025
Member
dstogov
left a comment
There was a problem hiding this comment.
I didn't found obvious problems.
Please verify this using community tests before merging.
I don't object against merging this into PHP-8.4.
ext/opcache/jit/zend_jit_ir.c
Outdated
| return new_exit_point; | ||
| } | ||
|
|
||
| zend_jit_ref_snapshot zend_jit_resolve_ref_snapshot(ir_ctx *ctx, ir_ref snapshot_ref, ir_insn *snapshot, int op) |
Member
There was a problem hiding this comment.
I would prefer not to return "long" structures.
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
3 participants
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.
Polymorphic calls pass
thisand the function to side traces via snapshotting. However, we assume thatthis/funcare in registers, when in fact they may be spilled.zend_jit_trace_exit_info.{poly_func_ref,poly_this_ref}are set byzend_jit_init_method_call():php-src/ext/opcache/jit/zend_jit_ir.c
Lines 9140 to 9141 in 2bba3dc
Then, after compilation,
zend_jit_trace_exit_info.{poly_func_reg,poly_this_reg}are set byzend_jit_snapshot_handler()from register numbers provided by the snapshot mechanism:php-src/ext/opcache/jit/zend_jit_ir.c
Lines 735 to 736 in 2bba3dc
However, these registers may be spilled.
This is properly handled for stack variables, but we can not use the same code for poly_func/poly_this as it's specific to stack vars / zvals.
Here I update snapshotting of poly_func/poly_this to support spilling:
zend_jit_snapshot_handler, store the C stack offset of the spilled register in poly_func_ref/poly_this_ref, in a way similar to how stack variables are handled here:php-src/ext/opcache/jit/zend_jit_ir.c
Line 775 in 2bba3dc
zend_jit_start, do not pre-load the registers if they were spilledzend_jit_trace_exit/zend_jit_trace_deoptimization, load from the stack if the register was spilledzend_jit_ctxso we can use that directly in the side traceNote: