JIT: Snapshotted poly_func / poly_this may be spilled#18408
+132 −67
Conversation
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
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; |
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.
There was a problem hiding this comment.
Also can't you do zend_jit_exit_point_by_addr(ptr); ?
| 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; |
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
| @@ -710,6 +710,29 @@ uint32_t zend_jit_duplicate_exit_point(ir_ctx *ctx, zend_jit_trace_info *t, uint | |||
| 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) | |||
There was a problem hiding this comment.
I would prefer not to return "long" structures.
Sign up for freeto 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.
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: