Fix constant propagation for JMP_NULL

[iluuu1994]

Don't propagate to JMP_NULL because it doesn't consume OP1. Also don't propagate
variables that were declared in other blocks.

Fixes oss-fuzz #60736

[ndossche]

The code looks correct, I checked the opcodes before optimizer & after optimizer and it looks correct.
Now I do have a question: why isn't the zend_optimizer_update_op1_const call unconditional? I guess it's because SCCP would take care of the constant propagation anyway during SSA optimizations if we don't do it here for the disallowed opcodes? (Disallowed opcodes being ZEND_CASE, ZEND_CASE_STRICT, ...)
Looking at the disallowed opcodes they do indeed look handled in SCCP. Just wanted to make sure :-).

[iluuu1994]

@nielsdos Good point. I think all of those opcodes don't consume op1. I guess the idea was that we're skipping propagating the constant as removing the source would make it unavailable for the following instructions. However, this can't work because the last instruction will still remove the source, leaving the leading instructions referencing an undeclared variable. This demonstrates the problem.

functiontest() {
switch (1?4:y) {
case1: return1;
case2: return2;
case3: return3;
case4: return4;
case5: return5;
}
}
var_dump(test());

In function ::test (before dfa):
var op1 of 0 (ZEND_FREE) does not use/def an ssa var
test:
; (lines=2, args=0, vars=0, tmps=3, ssa_vars=0, no_loops)
; (at SSA integrity verification)
; /home/ilutov/Developer/php-src/Zend/tests/test.php:3-11
; return [long] RANGE[4..4]
BB0:
; start exit lines=[0-1]
; level=0
0000 FREE T0
0001 RETURN int(4)
php: /home/ilutov/Developer/php-src/Zend/Optimizer/ssa_integrity.c:401: ssa_verify_integrity: Assertion `0 && "SSA integrity verification failed"' failed.
Termsig=6

We should do the same for all these opcodes, propagating the constant but keeping the source.

[ndossche]

Nice test, makes sense to me.

As a side note, it may make more sense for this optimization to live in SSA DFA?
I fixed a conceptually similar bug some time ago in the block_pass. It was about the source of ZEND_FREE being removed, leaving one of the basic blocks still with one ZEND_FREE referencing a non-existent source.
I fixed that by disallowing that case.
However, this still leaves the optimization opportunity to remove those ZEND_FREEs and the source too, if and only if we check all uses. I implemented this here: ndossche#21 (check test output diff to see what I mean if it's unclear, also that macOS CI failure is unrelated). Unfortunately for real world applications the benefit seems practically non-existent looking at the benchmark CI numbers.
SSA naturally makes such analysis easy to do, and I believe that might be the case here too? I just wanted to share this food for thought.

[iluuu1994]

@nielsdos Wait, I messed up. I had some local changes that broke this. 🙂 The relevant code is not triggered for the switch example, I'll need to examine more closely how to do that.

[iluuu1994]

@nielsdos I had another look. There was another issue where we propagated constants to "follow" blocks even if the variable was later used in a different block. Apparently this scenario just doesn't appear often in our opcodes. I checked and only 5 existing tests were affected.

[ndossche]

Okay this makes sense to me.

Also, I fixed such a bug with follow blocks in the past (for ZEND_FREE) with a src < op_array->opcodes + block->start check instead of using the bitset, so perhaps I should change that to use the bitset.

[iluuu1994]

Heh, Dmitry already committed it in 9fc0eab.