Skip to content
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

Jump to bottom

Internal closure causes JIT failure #17307

Open
YuanchengJiang opened this issue Dec 31, 2024 · 7 comments · May be fixed by #17319
Open

Internal closure causes JIT failure #17307

YuanchengJiang opened this issue Dec 31, 2024 · 7 comments · May be fixed by #17319
Labels
Bug Category: JIT Extension: opcache Status: Verified

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
$simple = simplexml_load_file(__DIR__."/book.xml");
$fusion = $simple;
require(__DIR__ . "/run_bcmath_tests_function.inc");
$minuends = ["15", "-15", "1", "-9", "14.14", "-16.60", "0.15", "-0.01"];
$subtrahends = array_merge($minuends, [
]);
run_bcmath_tests($fusion, $subtrahends, "-", bcsub(...));

Resulted in this output:

php: /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_API.c:142: const char *zend_get_type_by_const(int): Assertion `0' failed.
Aborted (core dumped)

To reproduce:

-d "opcache.jit_hot_func=1" -d "zend_extension=/home/phpfuzz/WorkSpace/flowfusion/php-src/modules/opcache.so" -d "opcache.enable_cli=1" -d "opcache.jit=1254"

PHP Version

nightly

Operating System

No response
@nielsdos
Copy link
Member

Similar to the other issue with these bcmath loops, I can't reproduce this one either...
But @devnexen can apparently... And IIRC he's on ARM, so I wonder if these issues are ARM specific.
@YuanchengJiang Can you please share the following: CPU used, OS used, configure command, stacktrace?

@devnexen
Copy link
Member

Yes I can indeed

image

@YuanchengJiang
Copy link
Author

My CPU is the Intel i7 14gen and my OS is Ubuntu. I use docker: https://hub.docker.com/layers/0599jiangyc/flowfusion/latest/images/sha256-ee8d6d8bbec99203c39f40042587d5d7eb5a17422ec160b7061f04c91af08301

I guess it is not ARM specific.

@nielsdos
Copy link
Member

nielsdos commented Jan 1, 2025

I could not reproduce this even in the container.
What I did, inside the container:

./configure --enable-debug --enable-address-sanitizer --enable-bcmath
make -j4
cp ext/simplexml/tests/book.xml .
cp ext/bcmath/tests/run_bcmath_tests_function.inc .
# Copied the OP code into test.php
./sapi/cli/php -d "opcache.jit_hot_func=1" -d "zend_extension=/home/phpfuzz/WorkSpace/php-src/modules/opcache.so" -d "opcache.enable_cli=1" -d "opcache.jit=1254" test.php

I also tried ZTS, makes no difference for me.
I can see with opcache.jit_debug the traces, so I know for a fact it is JITting.

Also not all questions were answered: please share the configure line and stacktrace.

@nielsdos
Copy link
Member

nielsdos commented Jan 1, 2025

Oh interesting, I am able to reproduce it in Valgrind, but not outside of it for some reason...

@nielsdos
Copy link
Member

nielsdos commented Jan 1, 2025 •
edited
Loading

Slightly reduced:

<?php

$simple = new SimpleXMLElement("<root><a/><b/></root>");

function run_bcmath_tests(
    $firstTerms,
    $closure
) {
    foreach ($firstTerms as $firstTerm) {
        $closure($firstTerm, "10", 10);
    }
}

run_bcmath_tests($simple, bcsub(...));

@nielsdos
Copy link
Member

nielsdos commented Jan 1, 2025 •
edited
Loading

Computed used stack size seems wrong.
Quick and dirty patch (needs more work which I'll do a bit later, but this makes the test work):

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 79ddfae4d6f..81bf9a9c922 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -8518,6 +8518,13 @@ static int zend_jit_push_call_frame(zend_jit_ctx *jit, const zend_op *opline, co
 			ir_ref tmp = ir_LOAD_U8(ir_ADD_OFFSET(func_ref, offsetof(zend_function, type)));
 			if_internal_func = ir_IF(ir_AND_U8(tmp, ir_CONST_U8(1)));
 			ir_IF_FALSE(if_internal_func);
+		} else {
+			used_stack_ref = ir_HARD_COPY_A(used_stack_ref); /* load constant once */
+
+			// JIT: if (EXPECTED(ZEND_USER_CODE(func->type))) {
+			ir_ref tmp = ir_LOAD_U8(ir_ADD_OFFSET(func_ref, offsetof(zend_closure, func.type)));
+			if_internal_func = ir_IF(ir_AND_U8(tmp, ir_CONST_U8(1)));
+			ir_IF_FALSE(if_internal_func);
 		}
 
 		// JIT: used_stack += (func->op_array.last_var + func->op_array.T - MIN(func->op_array.num_args, num_args)) * sizeof(zval);
@@ -8545,7 +8552,7 @@ static int zend_jit_push_call_frame(zend_jit_ctx *jit, const zend_op *opline, co
 		}
 		ref = ir_SUB_A(used_stack_ref, ref);
 
-		if (is_closure) {
+		if (0&&is_closure) {
 			used_stack_ref = ref;
 		} else {
 			ir_MERGE_WITH_EMPTY_TRUE(if_internal_func);

This patch fixes #17196 too, so I'm closing that one as a duplicate.

@nielsdos nielsdos mentioned this issue Jan 1, 2025
Closed
@nielsdos nielsdos changed the title Assertion failure Zend/zend_API.c:142 Internal closure causes JIT failure Jan 1, 2025
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 1, 2025
@nielsdos
Fix phpGH-17307: Internal closure causes JIT failure
c16fe8d 
`bcadd(...)` is a closure for an internal function, and
`zend_jit_push_call_frame` takes into account both last_var and the
difference in argument numbers not only for user code but also for
internal code. However, this is inconsistent with
`zend_vm_calc_used_stack`, causing argument corruption.
Making this consistent fixes the issue.

I could only reproduce the assertion failure when using Valgrind.
@nielsdos nielsdos linked a pull request Jan 1, 2025 that will close this issue
Fix GH-17307: Internal closure causes JIT failure #17319
Open
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 1, 2025
@nielsdos
Fix phpGH-17307: Internal closure causes JIT failure
ac05bb7 
`bcadd(...)` is a closure for an internal function, and
`zend_jit_push_call_frame` takes into account both last_var and the
difference in argument numbers not only for user code but also for
internal code. However, this is inconsistent with
`zend_vm_calc_used_stack`, causing argument corruption.
Making this consistent fixes the issue.

I could only reproduce the assertion failure when using Valgrind.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Category: JIT Extension: opcache Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-17307: Internal closure causes JIT failure nielsdos/php-src
3 participants
@devnexen @nielsdos @YuanchengJiang