[jorgsowa]

Fixes #23572

[ndossche]

I think this needs a different fix.
Looking at f896b982cce, I think that indeed we have to also fix the optimizer&inference.

Something like this perhaps:

diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c
index b2479a57b6e..223e4c76d48 100644
--- a/Zend/Optimizer/zend_inference.c
+++ b/Zend/Optimizer/zend_inference.c
@@ -2462,7 +2462,7 @@ static const zend_property_info *zend_fetch_static_prop_info(const zend_script *
 	const zend_property_info *prop_info = NULL;
 	if (opline->op1_type == IS_CONST) {
 		zend_class_entry *ce = NULL;
-		if (opline->op2_type == IS_UNUSED) {
+		if (opline->op2_type == IS_UNUSED && !(op_array->fn_flags & ZEND_ACC_TRAIT_CLONE)) {
 			int fetch_type = opline->op2.num & ZEND_FETCH_CLASS_MASK;
 			switch (fetch_type) {
 				case ZEND_FETCH_CLASS_SELF:
diff --git a/Zend/Optimizer/zend_optimizer.c b/Zend/Optimizer/zend_optimizer.c
index b5de6633146..24ca4b3556c 100644
--- a/Zend/Optimizer/zend_optimizer.c
+++ b/Zend/Optimizer/zend_optimizer.c
@@ -843,6 +843,7 @@ zend_class_entry *zend_optimizer_get_class_entry_from_op1(
 			return zend_optimizer_get_class_entry(script, op_array, Z_STR_P(op1 + 1));
 		}
 	} else if (opline->op1_type == IS_UNUSED && op_array->scope
+			&& !(op_array->fn_flags & ZEND_ACC_TRAIT_CLONE)
 			&& !(op_array->scope->ce_flags & ZEND_ACC_TRAIT)
 			&& ((opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF
 				|| ((opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_STATIC
diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index 76510743d33..bbdb56ca221 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -581,6 +581,10 @@ static zend_class_entry* zend_get_known_class(const zend_op_array *op_array, con
 		}
 	} else {
 		ZEND_ASSERT(op_type == IS_UNUSED);
+		if (op_array->fn_flags & ZEND_ACC_TRAIT_CLONE) {
+			/* self and parent depend on the class using the trait */
+			return NULL;
+		}
 		if ((op.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF) {
 			ce = op_array->scope;
 		} else {
diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 4b3207367fe..5e4af150861 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -9216,12 +9216,6 @@ static int zend_jit_init_static_method_call(zend_jit_ctx         *jit,
 	zend_function *func = NULL;
 	ir_ref func_ref, func_ref2, scope_ref, scope_ref2, if_cached, cold_path, ref;
 	ir_ref if_static = IR_UNUSED;
-	bool is_megamorphic_call_from_trait =
-		(op_array->fn_flags & ZEND_ACC_TRAIT_CLONE)
-		&& opline->op1_type != IS_CONST
-		&& trace
-		&& trace->op == ZEND_JIT_TRACE_INIT_CALL
-		&& trace->func;
 
 	if (info) {
 		call_info = info->callee_info;
@@ -9234,9 +9228,6 @@ static int zend_jit_init_static_method_call(zend_jit_ctx         *jit,
 	}
 
 	ce = zend_get_known_class(op_array, opline, opline->op1_type, opline->op1);
-	if (is_megamorphic_call_from_trait) {
-		ce = NULL;
-	}
 	if (!func && ce && (opline->op1_type == IS_CONST || !(ce->ce_flags & ZEND_ACC_TRAIT))) {
 		zval *zv = RT_CONSTANT(opline, opline->op2);
 		zend_string *method_name;
@@ -9290,15 +9281,14 @@ static int zend_jit_init_static_method_call(zend_jit_ctx         *jit,
 	func_ref = ir_PHI_2(IR_ADDR, func_ref2, func_ref);
 	scope_ref = ir_PHI_2(IR_ADDR, scope_ref2, scope_ref);
 
-	if ((is_megamorphic_call_from_trait || !func || zend_jit_may_be_modified(func, op_array))
+	if ((!func || zend_jit_may_be_modified(func, op_array))
 	 && trace
 	 && trace->op == ZEND_JIT_TRACE_INIT_CALL
 	 && trace->func) {
 		int32_t exit_point;
 		const void *exit_addr;
 
-		exit_point = zend_jit_trace_get_exit_point(opline,
-			func && !is_megamorphic_call_from_trait ? ZEND_JIT_EXIT_INVALIDATE : 0);
+		exit_point = zend_jit_trace_get_exit_point(opline, func ? ZEND_JIT_EXIT_INVALIDATE : 0);
 		exit_addr = zend_jit_trace_get_exit_addr(exit_point);
 		if (!exit_addr) {
 			return 0;

I think then the special check at do_fcall may start to become redundant too, including the skip_guard computation at zend_jit_trace.c. But this may need more thought. What do you think?

[ndossche]

If you have code like this, it's better to put the ce = zend_get_known_class(...) call in the else branch to avoid the computation. The compiler can only make such an optimizing transformation itself if it can prove zend_get_known_class is side-effect free.

[ndossche]

This is static megamorphic check, and according to your code, it can only be megamorphic if trace->func is set.

Also I don't think this static check always work. Consider this code:

<?php
trait T {
    public function callSelf() {
        return self::method();
    }
}

class A {
    use T;
    public static function method() {
        return 'A';
    }
}

class B {
    use T;
    public function method() {
        var_dump($this);
        return 'B';
    }
}

$a = new A;
$b = new B;

for ($i = 0; $i < 4; $i++) var_dump($a->callSelf());
for ($i = 0; $i < 4; $i++) var_dump($b->callSelf());
var_dump($a->callSelf());

Run under tracing jit with -d opcache.jit_max_polymorphic_calls=0 -d opcache.jit_hot_func=2
You'll see that it crashes because $this is unset for class B.

I think you need to replicate the check that do_fcall does

[jorgsowa]

Thank you @ndossche My fix was dumb. I applied your suggestion

[ndossche]

This looks right but I'm not 100% sure this is the best way or this covers every possible issue. I'd like to ask @arnaud-lb to double check please

[mgoulart]

The function JIT resolves self:: in trait methods the same wrong way, and with this PR applied opcache.jit=1205 and 1202 still fail. Here's a repro:

<?php
trait T {
    public function outer($x) { return self::inner($x) + 1; }
    public function inner($x) { return $x * 2; }
}
class A { use T; }
class B { use T; }
$s = 0;
for ($i = 0; $i < 20000; $i++) {
    $o = ($i % 2) ? new A : new B;
    try { $s += $o->outer($i); } catch (Error $e) { echo "iter $i: ", get_class($e), ': ', $e->getMessage(), "\n"; exit(1); }
}
echo "ok $s\n";

php -d opcache.enable_cli=1 -d opcache.file_update_protection=0 -d opcache.jit=1205 -d opcache.jit_buffer_size=64M t.php

Opcache has to have the script cached for it to fail, so file_update_protection=0 matters if you've just written the file.

Results on PHP-8.5.12, x86_64, built from the release tarball:

opcache.jit	8.5.12	8.5.12 + #23611	8.5.12 + #23611 + change below
disable	ok	ok	ok
1205	iter 2: Non-static method B::inner() cannot be called statically	same error	ok
1202	iter 2: same error	same error	ok
1215	iter 3: same error	ok	ok
1235	iter 127: same error	ok	ok
1201, 1255			ok
tracing	ok	ok	ok

8.4.27 passes in every mode. 8.5.0 and 8.5.7 fail the same way 8.5.12 does, and so does 8.5.12 on aarch64.

In 1205 and 1202 the JIT compiles the trait's own op_array, whose scope is T and which doesn't have ZEND_ACC_TRAIT_CLONE. The clones in A and B share its opcodes. zend_get_known_class() returns T, and because T is immutable and linked, zend_jit_init_static_method_call() uses ir_CONST_ADDR(ce) as the scope on the cached path instead of the scope from the run-time cache. zend_jit_push_this_method_call_frame() then finds that $this (a B) isn't an instance of T and throws. The first call for each class goes through the cold path and works, so the error shows up on the second call on a B, at iter 2.

Returning NULL when the scope is the trait itself fixes every mode in the table for us:

 		ZEND_ASSERT(op_type == IS_UNUSED);
-		if (op_array->fn_flags & ZEND_ACC_TRAIT_CLONE) {
+		if ((op_array->fn_flags & ZEND_ACC_TRAIT_CLONE)
+			 || (op_array->scope && (op_array->scope->ce_flags & ZEND_ACC_TRAIT))) {
 			/* self and parent depend on the class using the trait */
 			return NULL;
 		}

We see the same thing in a large application's test suite. With #23611 alone, 9 of the 14 tests in one class still throw this error under 1205 and 1202, and with the change they all pass in every mode. ext/opcache/tests/jit gives the same result with and without the change: 500 tests, 491 passed, 9 skipped, 0 failed.

We also built 8.5.12 with the GH-17626 fix (7050d57), this PR and the change above, and ran that application's whole test suite (10,328 tests). Under 1205 and under tracing, the results matched a run with the JIT off exactly. On stock 8.5.12 the same suite gets 32 of these errors under 1205, and with tracing it segfaults partway through, which is GH-17626.

We only looked at zend_jit.c. The optimizer's call-graph path might need the same check.