php / php-src

The PHP Interpreter
https://www.php.net
Other
38.27k stars 7.76k forks source link

Assertion failure in ext/opcache/jit/zend_jit_ir.c:8816 #16879

Open YuanchengJiang opened 1 day ago

YuanchengJiang commented 1 day ago

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 commented 20 hours ago

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 commented 19 hours ago

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 75698cdc054..1bec8869dfe 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -2574,7 +2574,33 @@ 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 outer 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;
+                       }
+                   }
                    break;
                /* stackless execution */
                case ZEND_INCLUDE_OR_EVAL: