JIT Index invalid or out of range error #12382

Activity

1. danog

   added

   Bug

   Status: Needs Triage

   on Oct 8, 2023
2. ndossche commented on Oct 8, 2023

   

   ndossche

   on Oct 8, 2023

   Member

   More actions

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

   added

   Category: JIT

   Extension: opcache

   Status: Verified

   and removed

   Status: Needs Triage

   on Oct 8, 2023
4. danog commented on Oct 8, 2023

   

   danog

   on Oct 8, 2023

   Last edited by danog

   ContributorAuthor

   More actions

   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.
5. danog commented on Oct 8, 2023

   

   danog

   on Oct 8, 2023

   ContributorAuthor

   More actions

   @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?
6. ndossche commented on Oct 8, 2023

   

   ndossche

   on Oct 8, 2023

   Last edited by ndossche

   Member

   More actions

   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
7. danog

   added a commit that references this issue on Oct 8, 2023

   Bypass JIT bug php/php-src#12382

   0ab6f4a
8. dstogov

   closed this as completedin 5a8f96bon Oct 9, 2023
9. dstogov

   added 3 commits that reference this issue on Oct 9, 2023

   Merge branch 'PHP-8.1' into PHP-8.2

   ...

   This issue will close once commit b38c57b is merged into the 'master' branch.

   b38c57b

   Merge branch 'PHP-8.2' into PHP-8.3

   ...

   This issue will close once commit 9e1e1bc is merged into the 'master' branch.

   9e1e1bc

   Merge branch 'PHP-8.3'

   ...

   This issue will close once commit d17c672 is merged into the 'master' branch.

   d17c672
10. dstogov commented on Oct 9, 2023

    

    dstogov

    on Oct 9, 2023

    Member

    More actions

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

    ❤️React with ❤️1Reacted by Daniil Gentili
11. jmikola

    added a commit that references this issue on Jan 25, 2024

    Fixed phpGH-12382: JIT Index invalid or out of range error

    This issue will close once commit 5a8f96b is merged into the 'master' branch.

    5a8f96b