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

Closed
wants to merge 4 commits into from
Closed

JIT: Snapshotted poly_func / poly_this may be spilled #18408

Show file tree
Hide file tree
Changes from 2 commits
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
24 changes: 15 additions & 9 deletions ext/opcache/jit/zend_jit_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,16 +420,22 @@ struct _zend_jit_trace_rec {

#define ZEND_JIT_TRACE_START_REC_SIZE 2

typedef struct _zend_jit_ref_snapshot {
union {
int32_t ref; /* While tracing */
int32_t offset; /* After compilation / during deopt: C stack offset if 'reg' is spilled */
};
int8_t reg; /* Set after compilation by zend_jit_snapshot_handler() */
} zend_jit_ref_snapshot;

typedef struct _zend_jit_trace_exit_info {
const zend_op *opline; /* opline where VM should continue execution */
const zend_op_array *op_array;
uint32_t flags; /* set of ZEND_JIT_EXIT_... */
uint32_t stack_size;
uint32_t stack_offset;
int32_t poly_func_ref;
int32_t poly_this_ref;
int8_t poly_func_reg;
int8_t poly_this_reg;
const zend_op *opline; /* opline where VM should continue execution */
const zend_op_array *op_array;
uint32_t flags; /* set of ZEND_JIT_EXIT_... */
uint32_t stack_size;
uint32_t stack_offset;
zend_jit_ref_snapshot poly_func;
zend_jit_ref_snapshot poly_this;
} zend_jit_trace_exit_info;

typedef struct _zend_jit_trace_stack {
Expand Down
94 changes: 66 additions & 28 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 @@ -660,8 +660,8 @@ static void jit_SNAPSHOT(zend_jit_ctx *jit, ir_ref addr)
ir_SNAPSHOT_SET_OP(snapshot, i + 1, ref);
}
if (n) {
ir_SNAPSHOT_SET_OP(snapshot, snapshot_size + 1, t->exit_info[exit_point].poly_func_ref);
ir_SNAPSHOT_SET_OP(snapshot, snapshot_size + 2, t->exit_info[exit_point].poly_this_ref);
ir_SNAPSHOT_SET_OP(snapshot, snapshot_size + 1, t->exit_info[exit_point].poly_func.ref);
ir_SNAPSHOT_SET_OP(snapshot, snapshot_size + 2, t->exit_info[exit_point].poly_this.ref);
}
}
}
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to return "long" structures.

{
int8_t *reg_ops = ctx->regs[snapshot_ref];
ZEND_ASSERT(reg_ops[op] != ZREG_NONE);

zend_jit_ref_snapshot rs = {
.reg = reg_ops[op],
};

if (IR_REG_SPILLED(rs.reg)) {
rs.reg = ((ctx->flags & IR_USE_FRAME_POINTER) ? IR_REG_FP : IR_REG_SP) | IR_REG_SPILL_LOAD;
rs.offset = ir_get_spill_slot_offset(ctx, ir_insn_op(snapshot, op));
}

return rs;
}

bool zend_jit_ref_snapshot_equals(zend_jit_ref_snapshot *a, zend_jit_ref_snapshot *b)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could be const pointers

{
return a->reg == b->reg
&& (!IR_REG_SPILLED(a->reg) || (a->offset == b->offset));
}

void *zend_jit_snapshot_handler(ir_ctx *ctx, ir_ref snapshot_ref, ir_insn *snapshot, void *addr)
{
zend_jit_trace_info *t = ((zend_jit_ctx*)ctx)->trace;
Expand All @@ -722,18 +745,18 @@ void *zend_jit_snapshot_handler(ir_ctx *ctx, ir_ref snapshot_ref, ir_insn *snaps
exit_flags = t->exit_info[exit_point].flags;

if (exit_flags & ZEND_JIT_EXIT_METHOD_CALL) {
int8_t *reg_ops = ctx->regs[snapshot_ref];
zend_jit_ref_snapshot func = zend_jit_resolve_ref_snapshot(ctx, snapshot_ref, snapshot, n - 1);
zend_jit_ref_snapshot this = zend_jit_resolve_ref_snapshot(ctx, snapshot_ref, snapshot, n);

ZEND_ASSERT(reg_ops[n - 1] != -1 && reg_ops[n] != -1);
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])) {
&& (!zend_jit_ref_snapshot_equals(&t->exit_info[exit_point].poly_func, &func)
|| !zend_jit_ref_snapshot_equals(&t->exit_info[exit_point].poly_this, &this))) {
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 = func;
t->exit_info[exit_point].poly_this = this;
n -= 2;
}

Expand Down Expand Up @@ -2751,6 +2774,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 +4448,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, int32_t 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 +8514,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 +8998,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 +9024,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 @@ -9137,8 +9171,8 @@ static int zend_jit_init_method_call(zend_jit_ctx *jit,
return 0;
}

jit->trace->exit_info[exit_point].poly_func_ref = func_ref;
jit->trace->exit_info[exit_point].poly_this_ref = this_ref;
jit->trace->exit_info[exit_point].poly_func.ref = func_ref;
jit->trace->exit_info[exit_point].poly_this.ref = this_ref;

func = (zend_function*)trace->func;

Expand Down Expand Up @@ -16991,9 +17025,13 @@ 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);
ZEND_ASSERT(parent->exit_info[exit_num].poly_func.reg >= 0 && parent->exit_info[exit_num].poly_this.reg >= 0);
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
81 changes: 51 additions & 30 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 @@ -198,10 +198,8 @@ static uint32_t zend_jit_trace_get_exit_point(const zend_op *to_opline, uint32_t
t->exit_info[exit_point].flags = flags;
t->exit_info[exit_point].stack_size = stack_size;
t->exit_info[exit_point].stack_offset = stack_offset;
t->exit_info[exit_point].poly_func_ref = 0;
t->exit_info[exit_point].poly_this_ref = 0;
t->exit_info[exit_point].poly_func_reg = ZREG_NONE;
t->exit_info[exit_point].poly_this_reg = ZREG_NONE;
t->exit_info[exit_point].poly_func = (zend_jit_ref_snapshot){.reg = ZREG_NONE};
t->exit_info[exit_point].poly_this = (zend_jit_ref_snapshot){.reg = ZREG_NONE};
}

return exit_point;
Expand Down Expand Up @@ -3484,17 +3482,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 +3637,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.offset);
jit->poly_this_ref = zend_jit_deopt_rload_spilled(jit, IR_ADDR,
exit_info->poly_this.reg, exit_info->poly_this.offset);

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

Expand Down Expand Up @@ -4235,11 +4241,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 +6327,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 +7352,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 @@ -7902,6 +7904,17 @@ static void zend_jit_dump_trace(zend_jit_trace_rec *trace_buffer, zend_ssa *tssa
}
}

static void zend_jit_dump_ref_snapshot(zend_jit_ref_snapshot *rs)
{
if (rs->reg == ZREG_NONE) {
fprintf(stderr, "?");
} else if (!IR_REG_SPILLED(rs->reg)) {
fprintf(stderr, "%s", zend_reg_name(rs->reg));
} else {
fprintf(stderr, "0x%x(%s)", rs->offset, zend_reg_name(IR_REG_NUM(rs->reg)));
}
}

static void zend_jit_dump_exit_info(zend_jit_trace_info *t)
{
int i, j;
Expand Down Expand Up @@ -7932,9 +7945,11 @@ static void zend_jit_dump_exit_info(zend_jit_trace_info *t)
if (t->exit_info[i].flags & (ZEND_JIT_EXIT_POLYMORPHISM|ZEND_JIT_EXIT_METHOD_CALL|ZEND_JIT_EXIT_CLOSURE_CALL)) {
fprintf(stderr, "/POLY");
if (t->exit_info[i].flags & ZEND_JIT_EXIT_METHOD_CALL) {
fprintf(stderr, "(%s, %s)",
t->exit_info[i].poly_func_reg != ZREG_NONE ? zend_reg_name(t->exit_info[i].poly_func_reg) : "?",
t->exit_info[i].poly_this_reg != ZREG_NONE ? zend_reg_name(t->exit_info[i].poly_this_reg) : "?");
fprintf(stderr, "(");
zend_jit_dump_ref_snapshot(&t->exit_info[i].poly_func);
fprintf(stderr, ", ");
zend_jit_dump_ref_snapshot(&t->exit_info[i].poly_this);
fprintf(stderr, ")");
}
}
if (t->exit_info[i].flags & ZEND_JIT_EXIT_FREE_OP1) {
Expand Down Expand Up @@ -8660,9 +8675,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];
zend_jit_ref_snapshot *func_snapshot = &t->exit_info[exit_num].poly_func;
ZEND_ASSERT(func_snapshot->reg >= 0);

zend_function *func;
if (IR_REG_SPILLED(func_snapshot->reg)) {
func = *(zend_function**)(regs->gpr[IR_REG_NUM(func_snapshot->reg)] + func_snapshot->offset);
} else {
func = (zend_function*)regs->gpr[func_snapshot->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