8.4 function JIT overflow bug

[danog]

Description

Running:

bugs/psalm_function.sh /app/tests/ExpressionTest.php

in https://github.com/danog/jit_bugs/ causes a test failure only with function JIT on x86-64-v4

ping @dstogov

PHP Version

nigtly

Operating System

No response

[nielsdos]

I can reproduce this, and I managed to reduce this to a standalone test case:

<?php


final class Test {
  public int $integer = -1;

  public function foo(int $x) {
    return $x;
  }
}

function foo(Test $test, int $value) {
  $val = $test->foo($value);
  if ($val <= PHP_INT_MAX) {
    $test->integer = $val;
  }
}

function main() {
  $test = new Test;
  foo($test, 9223372036854775806);
  foo($test, 9223372036854775807); // Also reproduces without this call, but this imitates the psalm code
  var_dump($test->integer);
}

main();

Run with: ./sapi/cli/php -c . -d opcache.jit=function -d opcache.jit_buffer_size=64M --repeat 2 x.php

Without JIT it prints int(9223372036854775807), but with JIt it prints int(-1).

[nielsdos]

I tried tackling this, and I figured out it was something related to zend_jit_is_constant_cmp_long_long. If I lower the check limit from PHP_INT_MAX to a lower number, the bug does not reproduce because the ranges are not covered in zend_jit_is_constant_cmp_long_long.
I then looked at the branching code for when the comparison is constant. I believe the bug might be there and it works when I change the code to this (which I found more intuitive):

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index f1b02ecbc4e..411d71b64d0 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -6775,10 +6775,10 @@ static ir_ref zend_jit_cmp_long_long(zend_jit_ctx   *jit,
 		if (smart_branch_opcode && !exit_addr) {
 			if (smart_branch_opcode == ZEND_JMPZ ||
 			    smart_branch_opcode == ZEND_JMPZ_EX) {
-				return jit_IF_ex(jit, IR_FALSE, result ? target_label : target_label2);
+				return jit_IF_ex(jit, result ? IR_TRUE : IR_FALSE, target_label2);
 			} else if (smart_branch_opcode == ZEND_JMPNZ ||
 			           smart_branch_opcode == ZEND_JMPNZ_EX) {
-				return jit_IF_ex(jit, IR_TRUE, result ? target_label : target_label2);
+				return jit_IF_ex(jit, result ? IR_TRUE : IR_FALSE, target_label);
 			} else {
 				ZEND_UNREACHABLE();
 			}

Although I'm a bit confused why this variant works and the original code does not.
I don't know if this is right, it's also quite late here and I lack some experience.
This fixes my reproducer and also fixes the original test case for me.
EDIT: Yes I think something more complex is going on... but I give up for now

[dstogov]

@nielsdos Thanks for the fix!
I'm also confused why this makes difference.
May be we have a hidden bug somewhere deeper...
I'll try to pay with this next week.

[nielsdos]

May be we have a hidden bug somewhere deeper...

I'm afraid this is indeed the case because the code generation is identical if I switch target_label and target_label2 in my patch; that is unexpected.

[dstogov]

I've found the real bug. When zend_jit_cmp() merges multiple conditions it ignores the previous decisions about true/false paths. This is the fix:

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index e4b68d23520..beab53894a1 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -7204,9 +7204,9 @@ static int zend_jit_cmp(zend_jit_ctx   *jit,
 
 				while (n) {
 					n--;
-					ir_IF_TRUE(end_inputs->refs[n]);
+					jit_IF_TRUE_FALSE_ex(jit, end_inputs->refs[n], label);
 					ir_END_list(true_inputs);
-					ir_IF_FALSE(end_inputs->refs[n]);
+					jit_IF_TRUE_FALSE_ex(jit, end_inputs->refs[n], label2);
 					ir_END_list(false_inputs);
 				}
 				ir_MERGE_list(true_inputs);