JIT dead code skipping does not update call_level

[YuanchengJiang]

Description

The following code:

<?php
match($y=y){};
session_set_save_handler(new SessionHandler);
session_start();

Resulted in this output:

php: ext/opcache/jit/zend_jit_ir.c:8816: int zend_jit_init_fcall(zend_jit_ctx *, const zend_op *, uint32_t, const zend_op_array *, zend_ssa *, const zend_ssa_op *, int, zend_jit_trace_rec *, int): Assertion `call_level > 0' failed.
Aborted (core dumped)

To reproduce:

-d "opcache.jit_hot_func=1" -d "zend_extension=/php-src/modules/opcache.so" -d "opcache.enable_cli=1" -d "opcache.jit=1235"

PHP Version

nightly

Operating System

ubuntu 22.04

[nielsdos]

Simplified:

<?php
match(y){};
var_dump(new stdClass);
var_dump(3);

This is the bytecode that is used for JITting:

BB0:
     ; start lines=[0-5]
     ; to=(BB1)
     ; level=0
     ; children=(BB1)
0000 T0 = FETCH_CONSTANT string("y")
0001 MATCH_ERROR T0
0002 FREE T0
0003 INIT_FCALL 1 96 string("var_dump")
0004 V0 = NEW 0 string("stdClass")
0005 DO_FCALL

BB1:
     ; follow exit entry lines=[6-11]
     ; from=(BB0)
     ; idom=BB0
     ; level=1
0006 SEND_VAR V0 1
0007 DO_ICALL
0008 INIT_FCALL 1 96 string("var_dump")
0009 SEND_VAL int(3) 1
0010 DO_ICALL
0011 RETURN int(1)

We intend to execute MATCH_ERROR in the VM and return to trace a hot function in BB1. We generate a tail handler and skip all remaining oplines of BB0. That means the INIT_FCALL in BB0 is missed and call_level is not increased to 1. This leads to the assertion failure.

[nielsdos]

Here is a patch that fixes the problem for me, but this may not be the best approach. It also duplicates code, although that can be solved by refactoring the call_level increment/decrement checks into separate functions. I may also be wrong, but intuitively I think at least the idea is right, I can update the patch with suggested improvements.

cc @dstogov

Patch:

diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index 0c6ab6c5cbc..cc22e7375a8 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -2576,7 +2576,34 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
 					}
 					/* THROW and EXIT may be used in the middle of BB */
 					/* don't generate code for the rest of BB */
-					i = end;
+
+					/* Skip current opline for call_level computation
+					 * Don't include last opline because end of loop already checks call level of last opline */
+					i++;
+					for (; i < end; i++) {
+						opline = op_array->opcodes + i;
+						switch (opline->opcode) {
+							case ZEND_INIT_FCALL:
+							case ZEND_INIT_FCALL_BY_NAME:
+							case ZEND_INIT_NS_FCALL_BY_NAME:
+							case ZEND_INIT_METHOD_CALL:
+							case ZEND_INIT_DYNAMIC_CALL:
+							case ZEND_INIT_STATIC_METHOD_CALL:
+							case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL:
+							case ZEND_INIT_USER_CALL:
+							case ZEND_NEW:
+								call_level++;
+								break;
+							case ZEND_DO_FCALL:
+							case ZEND_DO_ICALL:
+							case ZEND_DO_UCALL:
+							case ZEND_DO_FCALL_BY_NAME:
+							case ZEND_CALLABLE_CONVERT:
+								call_level--;
+								break;
+						}
+					}
+					opline = op_array->opcodes + i;
 					break;
 				/* stackless execution */
 				case ZEND_INCLUDE_OR_EVAL:

[dstogov]

@nielsdos thanks for the patch. It's right. Do we need this for PHP-8.4 or for PHP-8.2/3 as well?

[nielsdos]

@dstogov We only need this for PHP 8.4 and higher. This is because 8.2-8.3 JIT still loops over all the opcodes and generates dead code after ZEND_MATCH_ERROR opline is handled. Therefore call_level is updated too. PHP 8.4 instead skips the oplines, causing this issue by not updating call_level.

I will make a PR for PHP 8.4. I mentioned factoring out the check when call level should be increased or decreased to reduce repetition, I can make a separate PR for that later against the master branch.

[dstogov]

I will make a PR for PHP 8.4. I mentioned factoring out the check when call level should be increased or decreased to reduce repetition, I can make a separate PR for that later against the master branch.

Thanks. Please do. I don't think a separate PR for master is required.