JIT Index invalid or out of range error

[danog]

Description

The following code: https://paste.daniil.it/BaconQrCode.tar.xz, which is https://github.com/Bacon/BaconQrCode/ with an additional unit test that reproduces the issue.

Ref Bacon/BaconQrCode#92

Resulted in this output:

php -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.jit_buffer_size=1G -d opcache.jit_max_root_traces=1000000 -d opcache.jit_max_side_traces=1000000 -d opcache.jit_max_exit_counters=1000000 -d opcache.jit_hot_loop=1 -d opcache.jit_hot_func=1 -d opcache.jit_hot_return=1 -d opcache.jit_hot_side_exit=1 -d memory_limit=-1 vendor/bin/phpunit
PHPUnit 9.6.13 by Sebastian Bergmann and contributors.

...............................................................  63 / 159 ( 39%)
..........................................................E.... 126 / 159 ( 79%)
...............................SS                               159 / 159 (100%)

Time: 00:01.736, Memory: 6.00 MB

There was 1 error:

1) BaconQrCodeTest\Encoder\EncoderTest::testSimpleUtf8WithoutEci
RuntimeException: Index invalid or out of range

/root/BaconQrCode/src/Encoder/MaskUtil.php:134
/root/BaconQrCode/src/Encoder/Encoder.php:199
/root/BaconQrCode/src/Encoder/Encoder.php:246
/root/BaconQrCode/src/Encoder/Encoder.php:137
/root/BaconQrCode/test/Encoder/EncoderTest.php:166

ERRORS!
Tests: 159, Assertions: 79764, Errors: 1, Skipped: 2.

But I expected this output instead: no failed tests.

PHP Version

#12381

Operating System

No response

[nielsdos]

Reproducible only using tracing JIT, not e.g. function JIT.

[danog]

Basic reproducer:

<?php

function applyMaskPenaltyRule3(array $array) : void
{
    for ($y = 0; $y < 21; ++$y) {
        for ($x = 0; $x < 21; ++$x) {
            if (
                (
                    $x + 10 < 21
                    && 0 === $array[$y][$x + 7]
                )
                || (
                    $x - 4 >= 0
                    && 0 === $array[$y][$x - 1]
                )
            ) {
            }
        }
    }
}

$matrix = [[1, 1, 1, 1, 1, 1, 1, 0, 1, 0, 0, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, ], [1, 0, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 1, 0, 1, 0, 0, 0, 0, 0, 1, ], [1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 1, 1, 1, 0, 1, ], [1, 0, 1, 1, 1, 0, 1, 0, 1, 0, 1, 1, 1, 0, 1, 0, 1, 1, 1, 0, 1, ], [1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 1, 1, 0, 1, ], [1, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 1, ], [1, 1, 1, 1, 1, 1, 1, 0, 1, 0, 1, 0, 1, 0, 1, 1, 1, 1, 1, 1, 1, ], [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ], [0, 0, 1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 0, 1, 0, 0, 0, 1, 0, 0, 1, ], [0, 0, 1, 1, 0, 0, 0, 1, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 1, 1, 0, ], [0, 1, 1, 0, 1, 0, 1, 0, 1, 1, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 1, ], [1, 0, 0, 1, 0, 1, 0, 0, 0, 1, 1, 0, 1, 0, 1, 0, 0, 0, 1, 0, 0, ], [1, 1, 0, 0, 1, 0, 1, 0, 1, 1, 0, 0, 0, 0, 1, 0, 1, 0, 1, 0, 1, ], [0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 1, 1, 0, 1, 0, 1, 0, 1, 1, ], [1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1, 1, 1, 0, 1, 1, 1, 1, ], [1, 0, 0, 0, 0, 0, 1, 0, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 0, 0, 0, ], [1, 0, 1, 1, 1, 0, 1, 0, 1, 1, 0, 0, 0, 0, 1, 1, 0, 1, 1, 0, 1, ], [1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 1, 1, 0, ], [1, 0, 1, 1, 1, 0, 1, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 0, 0, 0, 1, ], [1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 1, 1, 0, ], [1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 1, 1, 1, 0, 1, 0, 1, 1, 1, ], ];

applyMaskPenaltyRule3($matrix);
applyMaskPenaltyRule3($matrix);

Emits

PHP Warning:  Undefined array key -1 in /home/daniil/repos/BaconQrCode/a.php on line 14
PHP Warning:  Undefined array key -1 in /home/daniil/repos/BaconQrCode/a.php on line 14

Only if tracing JIT is enabled.

[danog]

@nielsdos @dstogov A thing that's been bothering me this bug in particular is that it could in theory be reproduced with an appropriate testcase, combined with the --repeat option of phpunit, but in practice it can only be reproduced if it's the only testcase, any idea of the cause/solution?

For example, adding the following testcase to EncoderTest.php:

    public function testEncodeSmall() : void
    {
        $qrCode = Encoder::encode('a', ErrorCorrectionLevel::H());
        $this->assertTrue(true);
    }

And running the entire testsuite with:

vendor/bin/phpunit  --repeat 100

does not reproduce the issue, while running just that testcase with:

vendor/bin/phpunit  --repeat 100 test/Encoder/EncoderTest.php --filter testEncodeSmall

does.

I've been thinking of adding a --repeat to all phpunit calls in the community tests (and something similar in the unit tests ran by run-tests.php) to improve the chances of hitting a tracing JIT bug, but this weird behavior would make it all useless unless I also add manual per-testcase filtering and execution.

Any idea of the possible cause?

[nielsdos]

The issue seems to be related to the skipping of comparisons. If I remove the strict comparison and just use a boolean cast it works. Disabling the optimization with ZEND_SUB in the first case of zend_jit_may_skip_comparison also makes the code work.

Looking at the diff in assembly instructions between forcing skip_comparison to off and normal execution shows this:

 	mov 0x70(%r14), %rcx
 	lea -0x4(%rcx), %rax
+	test %rax, %rax
 	jge jit$$trace_exit_0

So: in the working version there's an additional test instruction. It seems that the subtraction was implemented with a lea instruction, but lea does NOT set the flags register.

Ideas to fix this:

• Either the skip comparison optimization must not be performed when emitting lea.
• Or sub should be used when we can predict skip_comparison will be true

[dstogov]

@danog thanks for the reproducible case
@nielsdos thanks for the analyses