Fix RC tracking of op1 of FETCH_OBJ and INIT_METHOD_CALL

[iluuu1994]

Fixes GH-17151

We may want to backport this to older branches.

[iluuu1994]

I see that even this basic case is faulty:

class C {
    public static $prop;

    public function storeThis() {
        self::$prop = $this;
    }
}

function test() {
    $c = new C();
    $c->storeThis();
    $c = null;
}

test();

...
0002 QM_ASSIGN #1.V1 [rc1, object (C)] #0.CV0($c) NOVAL [undef] -> #2.CV0($c) [rc1, object (C)]
0003 INIT_METHOD_CALL 0 #2.CV0($c) [rc1, object (C)] string("storeThis")
0004 DO_FCALL

BB2:
     ; follow exit entry lines=[5-6]
     ; from=(BB1)
     ; idom=BB1
     ; level=2
0005 ASSIGN #2.CV0($c) [rc1, object (C)] -> #3.CV0($c) NOVAL [null] null
...

ASSIGN (0005) thinks it is overwriting a rc1. I can't get this to crash atm, and I'm done for this week. I'll have a look on Monday.

Edit: I see, we're just not making use of RC inference in this case yet. With this, the issue is reproducible.

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 6eb5d4c7b4..381dcf3ec3 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -6539,14 +6539,20 @@ static int zend_jit_assign_to_variable(zend_jit_ctx   *jit,
 			}
 			counter = jit_GC_DELREF(jit, ref);
 
-			if_not_zero = ir_IF(counter);
-			ir_IF_FALSE(if_not_zero);
-			jit_ZVAL_DTOR(jit, ref, var_info, opline);
-			if (check_exception) {
-				zend_jit_check_exception(jit);
+			if (RC_MAY_BE_1(var_info) && RC_MAY_BE_N(var_info)) {
+				if_not_zero = ir_IF(counter);
+				ir_IF_FALSE(if_not_zero);
+			}
+			if (RC_MAY_BE_1(var_info)) {
+				jit_ZVAL_DTOR(jit, ref, var_info, opline);
+				if (check_exception) {
+					zend_jit_check_exception(jit);
+				}
+			}
+			if (RC_MAY_BE_1(var_info) && RC_MAY_BE_N(var_info)) {
+				ir_refs_add(end_inputs, ir_END());
+				ir_IF_TRUE(if_not_zero);
 			}
-			ir_refs_add(end_inputs, ir_END());
-			ir_IF_TRUE(if_not_zero);
 			if (RC_MAY_BE_N(var_info) && (var_info & (MAY_BE_ARRAY|MAY_BE_OBJECT)) != 0) {
 				ir_ref if_may_leak = jit_if_GC_MAY_NOT_LEAK(jit, ref);
 				ir_IF_FALSE(if_may_leak);

[dstogov]

After this, I'm not sure if it makes sense to relay on IS_OBJECT RC inference at all.
Almost any operation makes objects RC1|RCN anyway.

[iluuu1994]

Ok. In what way would you prefer fixing this? Adding a check to UPDATE_SSA_TYPE()? This may prevent RC-inference from being utilized for pi variables, but might not make a big difference in practice.

[dstogov]

Adding new SSA variables for IS_VAR/IS_TMP_VAR doesn't make any sense, because they are not goinf to be used (IS_VAR/IS_TMP_VAR are used once).

[dstogov]

Finally I think the problem may be fixed by disabling RC inference for IS_CV objects and special handling for magic methods in JIT.

diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c
index 71692388931..fc6b9b421b6 100644
--- a/Zend/Optimizer/zend_inference.c
+++ b/Zend/Optimizer/zend_inference.c
@@ -1968,6 +1968,10 @@ static uint32_t get_ssa_alias_types(zend_ssa_alias_kind alias) {
 					/* TODO: support for array keys and ($str . "")*/   \
 					__type |= MAY_BE_RCN;                               \
 				}                                                       \
+				if ((__type & MAY_BE_RC1) && (__type & MAY_BE_OBJECT)) {\
+					/* TODO: object may be captured by magic handlers */\
+					__type |= MAY_BE_RCN;                               \
+				}                                                       \
 				if (__ssa_var->alias) {									\
 					__type |= get_ssa_alias_types(__ssa_var->alias);	\
 				}														\
diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 0082d531103..b300f31a7e4 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -14621,6 +14621,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx         *jit,
 		ir_MERGE_list(slow_inputs);
 		jit_SET_EX_OPLINE(jit, opline);
 
+		op1_info |= MAY_BE_RC1 | MAY_BE_RCN; /* object may be captured/released in magic handler */
 		if (opline->opcode == ZEND_FETCH_OBJ_W) {
 			ir_CALL_1(IR_VOID, ir_CONST_FC_FUNC(zend_jit_fetch_obj_w_slow), obj_ref);
 			ir_END_list(end_inputs);

May be I missed something. Please, verify..

[dstogov]

Adding new SSA variables for IS_VAR/IS_TMP_VAR doesn't make any sense, because they are not goinf to be used (IS_VAR/IS_TMP_VAR are used once).

[dstogov]

Instead of this I would set op1_info |= MAY_BE_RC1|MAY_BE_RCN in case we call "slow path".

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 0082d531103..b300f31a7e4 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -14621,6 +14621,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx         *jit,
 		ir_MERGE_list(slow_inputs);
 		jit_SET_EX_OPLINE(jit, opline);
 
+		op1_info |= MAY_BE_RC1 | MAY_BE_RCN; /* object may be captured/released in magic handler */
 		if (opline->opcode == ZEND_FETCH_OBJ_W) {
 			ir_CALL_1(IR_VOID, ir_CONST_FC_FUNC(zend_jit_fetch_obj_w_slow), obj_ref);
 			ir_END_list(end_inputs);