Skip to content

JIT: Snapshotted poly_func / poly_this may be spilled #18408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: PHP-8.4
Choose a base branch
from
Open

JIT: Snapshotted poly_func / poly_this may be spilled #18408

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61469bd
Snapshot may be spilled
arnaud-lb Apr 23, 2025
6146b82
Refacto
arnaud-lb Apr 23, 2025
d713178
Comment
arnaud-lb Apr 29, 2025
e8d9681
Review
arnaud-lb Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 57 additions & 22 deletions ext/opcache/jit/zend_jit_ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ typedef struct _zend_jit_ctx {
ir_ref tls;
#endif
ir_ref fp;
ir_ref poly_func_ref; /* restored from parent trace snapshot */
ir_ref poly_this_ref; /* restored from parent trace snapshot */
ir_ref trace_loop_ref;
ir_ref return_inputs;
const zend_op_array *op_array;
Expand Down Expand Up @@ -624,12 +626,10 @@ static void jit_SNAPSHOT(zend_jit_ctx *jit, ir_ref addr)
uint32_t exit_point = 0, n = 0;

if (addr < 0) {
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;
Copy link
Member Author

@arnaud-lb arnaud-lb Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can't you do zend_jit_exit_point_by_addr(ptr); ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed :)

}
}

Expand Down Expand Up @@ -723,17 +723,37 @@ void *zend_jit_snapshot_handler(ir_ctx *ctx, ir_ref snapshot_ref, ir_insn *snaps

if (exit_flags & ZEND_JIT_EXIT_METHOD_CALL) {
int8_t *reg_ops = ctx->regs[snapshot_ref];

ZEND_ASSERT(reg_ops[n - 1] != -1 && reg_ops[n] != -1);

int8_t func_reg = reg_ops[n - 1];
ir_ref func_ref = ir_insn_op(snapshot, n - 1);
if (IR_REG_SPILLED(func_reg)) {
func_reg = ((ctx->flags & IR_USE_FRAME_POINTER) ? IR_REG_FP : IR_REG_SP) | IR_REG_SPILL_LOAD;
func_ref = ir_get_spill_slot_offset(ctx, func_ref);
}

int8_t this_reg = reg_ops[n];
ir_ref this_ref = ir_insn_op(snapshot, n);
if (IR_REG_SPILLED(this_reg)) {
this_reg = ((ctx->flags & IR_USE_FRAME_POINTER) ? IR_REG_FP : IR_REG_SP) | IR_REG_SPILL_LOAD;
this_ref = ir_get_spill_slot_offset(ctx, this_ref);
}

if ((exit_flags & ZEND_JIT_EXIT_FIXED)
&& (t->exit_info[exit_point].poly_func_reg != reg_ops[n - 1]
|| t->exit_info[exit_point].poly_this_reg != reg_ops[n])) {
&& ((t->exit_info[exit_point].poly_func_reg != func_reg
|| (IR_REG_SPILLED(func_reg)
&& t->exit_info[exit_point].poly_func_ref != func_ref))
|| (t->exit_info[exit_point].poly_this_reg != this_reg
|| (IR_REG_SPILLED(this_reg)
&& t->exit_info[exit_point].poly_this_ref != this_ref)))) {
exit_point = zend_jit_duplicate_exit_point(ctx, t, exit_point, snapshot_ref);
addr = (void*)zend_jit_trace_get_exit_addr(exit_point);
exit_flags &= ~ZEND_JIT_EXIT_FIXED;
}
t->exit_info[exit_point].poly_func_reg = reg_ops[n - 1];
t->exit_info[exit_point].poly_this_reg = reg_ops[n];
t->exit_info[exit_point].poly_func_reg = func_reg;
t->exit_info[exit_point].poly_func_ref = func_ref;
t->exit_info[exit_point].poly_this_reg = this_reg;
t->exit_info[exit_point].poly_this_ref = this_ref;
n -= 2;
}

Expand Down Expand Up @@ -2751,6 +2771,8 @@ static void zend_jit_init_ctx(zend_jit_ctx *jit, uint32_t flags)
jit->tls = IR_UNUSED;
#endif
jit->fp = IR_UNUSED;
jit->poly_func_ref = IR_UNUSED;
jit->poly_this_ref = IR_UNUSED;
jit->trace_loop_ref = IR_UNUSED;
jit->return_inputs = IR_UNUSED;
jit->bb_start_ref = NULL;
Expand Down Expand Up @@ -4423,6 +4445,18 @@ static ir_ref zend_jit_deopt_rload(zend_jit_ctx *jit, ir_type type, int32_t reg)
return ir_RLOAD(type, reg);
}

/* Same as zend_jit_deopt_rload(), but 'reg' may be spilled on C stack */
static ir_ref zend_jit_deopt_rload_spilled(zend_jit_ctx *jit, ir_type type, int8_t reg, ir_ref offset)
{
ZEND_ASSERT(reg >= 0);

if (IR_REG_SPILLED(reg)) {
return ir_LOAD(type, ir_ADD_OFFSET(zend_jit_deopt_rload(jit, type, IR_REG_NUM(reg)), offset));
} else {
return zend_jit_deopt_rload(jit, type, reg);
}
}

static int zend_jit_store_const_long(zend_jit_ctx *jit, int var, zend_long val)
{
zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, EX_NUM_TO_VAR(var));
Expand Down Expand Up @@ -8477,10 +8511,9 @@ static int zend_jit_stack_check(zend_jit_ctx *jit, const zend_op *opline, uint32
return 1;
}

static int zend_jit_free_trampoline(zend_jit_ctx *jit, int8_t func_reg)
static int zend_jit_free_trampoline(zend_jit_ctx *jit, ir_ref func)
{
// JIT: if (UNEXPECTED(func->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE))
ir_ref func = ir_RLOAD_A(func_reg);
ir_ref if_trampoline = ir_IF(ir_AND_U32(
ir_LOAD_U32(ir_ADD_OFFSET(func, offsetof(zend_function, common.fn_flags))),
ir_CONST_U32(ZEND_ACC_CALL_VIA_TRAMPOLINE)));
Expand Down Expand Up @@ -8962,15 +8995,15 @@ static int zend_jit_init_method_call(zend_jit_ctx *jit,
zend_class_entry *trace_ce,
zend_jit_trace_rec *trace,
int checked_stack,
int8_t func_reg,
int8_t this_reg,
ir_ref func_ref,
ir_ref this_ref,
bool polymorphic_side_trace)
{
zend_func_info *info = ZEND_FUNC_INFO(op_array);
zend_call_info *call_info = NULL;
zend_function *func = NULL;
zval *function_name;
ir_ref if_static = IR_UNUSED, cold_path, this_ref = IR_NULL, func_ref = IR_NULL;
ir_ref if_static = IR_UNUSED, cold_path;

ZEND_ASSERT(opline->op2_type == IS_CONST);
ZEND_ASSERT(op1_info & MAY_BE_OBJECT);
Expand All @@ -8988,10 +9021,8 @@ static int zend_jit_init_method_call(zend_jit_ctx *jit,
}

if (polymorphic_side_trace) {
/* function is passed in r0 from parent_trace */
ZEND_ASSERT(func_reg >= 0 && this_reg >= 0);
func_ref = zend_jit_deopt_rload(jit, IR_ADDR, func_reg);
this_ref = zend_jit_deopt_rload(jit, IR_ADDR, this_reg);
/* function is passed from parent snapshot */
ZEND_ASSERT(func_ref != IR_UNUSED && this_ref != IR_UNUSED);
} else {
ir_ref ref, ref2, if_found, fast_path, run_time_cache, this_ref2;

Expand Down Expand Up @@ -16992,8 +17023,12 @@ static int zend_jit_trace_start(zend_jit_ctx *jit,

if (parent && parent->exit_info[exit_num].flags & ZEND_JIT_EXIT_METHOD_CALL) {
ZEND_ASSERT(parent->exit_info[exit_num].poly_func_reg >= 0 && parent->exit_info[exit_num].poly_this_reg >= 0);
ir_RLOAD_A(parent->exit_info[exit_num].poly_func_reg);
ir_RLOAD_A(parent->exit_info[exit_num].poly_this_reg);
if (!IR_REG_SPILLED(parent->exit_info[exit_num].poly_func_reg)) {
ir_RLOAD_A(parent->exit_info[exit_num].poly_func_reg);
}
if (!IR_REG_SPILLED(parent->exit_info[exit_num].poly_this_reg)) {
ir_RLOAD_A(parent->exit_info[exit_num].poly_this_reg);
}
}

ir_STORE(jit_EG(jit_trace_num), ir_CONST_U32(trace_num));
Expand Down
56 changes: 33 additions & 23 deletions ext/opcache/jit/zend_jit_trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3484,17 +3484,18 @@ static int zend_jit_trace_exit_needs_deoptimization(uint32_t trace_num, uint32_t
}

static int zend_jit_trace_deoptimization(
zend_jit_ctx *jit,
uint32_t flags,
const zend_op *opline,
zend_jit_trace_stack *parent_stack,
int parent_vars_count,
zend_ssa *ssa,
zend_jit_trace_stack *stack,
zend_jit_exit_const *constants,
int8_t func_reg,
bool polymorphic_side_trace)
zend_jit_ctx *jit,
const zend_jit_trace_exit_info *exit_info,
zend_jit_trace_stack *parent_stack,
int parent_vars_count,
zend_ssa *ssa,
zend_jit_trace_stack *stack,
zend_jit_exit_const *constants,
bool polymorphic_side_trace)
{
uint32_t flags = exit_info->flags;
const zend_op *opline = exit_info->opline;

int i;
int check2 = -1;

Expand Down Expand Up @@ -3638,9 +3639,16 @@ static int zend_jit_trace_deoptimization(
zend_jit_check_exception(jit);
}

if ((flags & ZEND_JIT_EXIT_METHOD_CALL) && !polymorphic_side_trace) {
if (!zend_jit_free_trampoline(jit, func_reg)) {
return 0;
if (flags & ZEND_JIT_EXIT_METHOD_CALL) {
jit->poly_func_ref = zend_jit_deopt_rload_spilled(jit, IR_ADDR,
exit_info->poly_func_reg, exit_info->poly_func_ref);
jit->poly_this_ref = zend_jit_deopt_rload_spilled(jit, IR_ADDR,
exit_info->poly_this_reg, exit_info->poly_this_ref);

if (!polymorphic_side_trace) {
if (!zend_jit_free_trampoline(jit, jit->poly_func_ref)) {
return 0;
}
}
}

Expand Down Expand Up @@ -4235,11 +4243,9 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
if (parent_trace) {
/* Deoptimization */
if (!zend_jit_trace_deoptimization(&ctx,
zend_jit_traces[parent_trace].exit_info[exit_num].flags,
zend_jit_traces[parent_trace].exit_info[exit_num].opline,
&zend_jit_traces[parent_trace].exit_info[exit_num],
parent_stack, parent_vars_count, ssa, stack,
zend_jit_traces[parent_trace].constants,
zend_jit_traces[parent_trace].exit_info[exit_num].poly_func_reg,
polymorphic_side_trace)) {
goto jit_failure;
}
Expand Down Expand Up @@ -6323,8 +6329,8 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
op_array, ssa, ssa_op, frame->call_level,
op1_info, op1_addr, ce, ce_is_instanceof, on_this, delayed_fetch_this, op1_ce,
p + 1, peek_checked_stack - checked_stack,
polymorphic_side_trace ? zend_jit_traces[parent_trace].exit_info[exit_num].poly_func_reg : -1,
polymorphic_side_trace ? zend_jit_traces[parent_trace].exit_info[exit_num].poly_this_reg : -1,
polymorphic_side_trace ? jit->poly_func_ref : -1,
polymorphic_side_trace ? jit->poly_this_ref : -1,
polymorphic_side_trace)) {
goto jit_failure;
}
Expand Down Expand Up @@ -7348,11 +7354,9 @@ static const void *zend_jit_trace_exit_to_vm(uint32_t trace_num, uint32_t exit_n
NULL;

if (!zend_jit_trace_deoptimization(&ctx,
zend_jit_traces[trace_num].exit_info[exit_num].flags,
zend_jit_traces[trace_num].exit_info[exit_num].opline,
&zend_jit_traces[trace_num].exit_info[exit_num],
stack, stack_size, NULL, NULL,
zend_jit_traces[trace_num].constants,
zend_jit_traces[trace_num].exit_info[exit_num].poly_func_reg,
0)) {
goto jit_failure;
}
Expand Down Expand Up @@ -8660,9 +8664,15 @@ int ZEND_FASTCALL zend_jit_trace_exit(uint32_t exit_num, zend_jit_registers_buf
}
}
if (t->exit_info[exit_num].flags & ZEND_JIT_EXIT_METHOD_CALL) {
ZEND_ASSERT(t->exit_info[exit_num].poly_func_reg >= 0);
zend_function *func = (zend_function*)regs->gpr[t->exit_info[exit_num].poly_func_reg];
int8_t reg = t->exit_info[exit_num].poly_func_reg;
ZEND_ASSERT(reg >= 0);

zend_function *func;
if (IR_REG_SPILLED(reg)) {
func = *(zend_function**)(regs->gpr[IR_REG_NUM(reg)] + t->exit_info[exit_num].poly_func_ref);
} else {
func = (zend_function*)regs->gpr[reg];
}
if (UNEXPECTED(func->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
zend_string_release_ex(func->common.function_name, 0);
zend_free_trampoline(func);
Expand Down
Loading