Fix block_pass JMP[N]Z optimization #20850
Conversation
In the following optimization: JMPZ(X,L1) JMP(L2) L1: -> JMPNZ(X,L2) NOP L1 must not be followed by another block, so that it may safely be followed by the block containing the JMPNZ. get_next_block() is used to verify L1 is the direct follower. This function also skips empty blocks, including live, empty target blocks, which will then implicitly follow the new follow block. This will result in L1 being followed by two separate blocks, which is not possible. Resolve this by get_next_block() stopping at target blocks. Fixes OSS-Fuzz #472563272
| next_block++; | ||
| } | ||
| while (next_block->len == 0 && !(next_block->flags & ZEND_BB_PROTECTED)) { | ||
| while (next_block->len == 0 && !(next_block->flags & (ZEND_BB_TARGET|ZEND_BB_PROTECTED))) { |
There was a problem hiding this comment.
I feel that stopping at ZEND_BB_TARGET will often disable this optimization, as next_block will be ZEND_BB_TARGET most of the time (since it's the next block after an unconditional JMP, it must be a jump target), and this causes target_block == next_block to be false when there where blocks to skip here.
Maybe only in master, but would it make sense to relax the rule JMPZ(X,L1) JMP(L2) L1: -> JMPNZ(X,L2) NOP to JMPZ(X,L1) JMP(L2) L3: -> JMPNZ(X,L2) NOP with L3 being either L1 itself or part of a chain of empty predecessors to L1?
There was a problem hiding this comment.
FWIW, it seems to affect only 5 of our tests. Another solution might be to immediately remove references to blocks when they become empty, and relink them to the following block, and remove ZEND_BB_TARGET from the empty block that will then be properly dead.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Perhaps not the best solution, but it can be iterated upon on master
3 participants
In the following optimization:
L1must not be followed by another block, so that it may safely be followed by the block containing theJMPNZ.get_next_block()is used to verifyL1is the direct follower. This function also skips empty blocks, including live, empty target blocks, which will then implicitly follow the new follow block. This will result inL1being followed by two separate blocks, which is not possible.Resolve this by
get_next_block()stopping at target blocks.Fixes OSS-Fuzz #472563272