Split the live-ranges of loop variables again

b0af9ac removed the live-range splitting of foreach variables, however it only added handling to ZEND_HANDLE_EXCEPTION.
This was sort-of elegant, until it was realized in 8258b77 that it would leak the return variable, requiring some more special handling.
At some point we added live tmpvar rooting in 52cf7ab, but this did not take into account already freed loop variables, which also might happen during ZEND_RETURN, which cannot be trivially accounted for, without even more complicated handling in zend_gc_*_tmpvars() functions.

This commit also proposes a simpler way of tracking the loop end in loopvar freeing ops: handle it directly during live range computation rather than during compilation, eliminating the need for opcache to handle it specifically.
Further, opcache was using live_ranges in its basic block computation in the past, which it no longer does. Thus this complication is no longer necessary and this approach should be actually simpler now.

Closes #20766.

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

Author: bwoebi

Zend/Optimizer/zend_dump.c

@@ -122,6 +122,10 @@ static void zend_dump_unused_op(const zend_op *opline, znode_op op, uint32_t fla
 		if (op.num != (uint32_t)-1) {
 			fprintf(stderr, " try-catch(%u)", op.num);
 		}
+	} else if (ZEND_VM_OP_LOOP_END == (flags & ZEND_VM_OP_MASK)) {
+		if (opline->extended_value & ZEND_FREE_ON_RETURN) {
+			fprintf(stderr, " loop-end(%u)", op.num);
+		}
 	} else if (ZEND_VM_OP_THIS == (flags & ZEND_VM_OP_MASK)) {
 		fprintf(stderr, " THIS");
 	} else if (ZEND_VM_OP_NEXT == (flags & ZEND_VM_OP_MASK)) {

Zend/tests/gc_050.phpt

@@ -0,0 +1,40 @@
+--TEST--
+GC 050: Try/finally in foreach should create separate live ranges
+--FILE--
+<?php
+
+function f(int $n): object {
+    try {
+        foreach ((array) $n as $v) {
+            if ($n === 1) {
+                try {
+                    $a = new stdClass;
+                    return $a;
+                } finally {
+                    return $ret = $a;
+                }
+            }
+            if ($n === 2) {
+                $b = new stdClass;
+                return $ret = $b;
+            }
+        }
+    } finally {
+        $ret->v = 1;
+    }
+    return new stdClass;
+}
+
+for ($i = 0; $i < 100000; $i++) {
+    // Create cyclic garbage to trigger GC
+    $a = new stdClass;
+    $b = new stdClass;
+    $a->r = $b;
+    $b->r = $a;
+
+    $r = f($i % 2 + 1);
+}
+echo "OK\n";
+?>
+--EXPECT--
+OK

Zend/zend_execute.c

@@ -4472,20 +4472,6 @@ static void cleanup_unfinished_calls(zend_execute_data *execute_data, uint32_t o
 }
 /* }}} */
 
-static const zend_live_range *find_live_range(const zend_op_array *op_array, uint32_t op_num, uint32_t var_num) /* {{{ */
-{
-	int i;
-	for (i = 0; i < op_array->last_live_range; i++) {
-		const zend_live_range *range = &op_array->live_range[i];
-		if (op_num >= range->start && op_num < range->end
-				&& var_num == (range->var & ~ZEND_LIVE_MASK)) {
-			return range;
-		}
-	}
-	return NULL;
-}
-/* }}} */
-
 static void cleanup_live_vars(zend_execute_data *execute_data, uint32_t op_num, uint32_t catch_op_num) /* {{{ */
 {
 	int i;

Zend/zend_opcode.c

@@ -963,38 +963,35 @@ static void zend_calc_live_ranges(
 					/* OP_DATA is really part of the previous opcode. */
 					last_use[var_num] = opnum - (opline->opcode == ZEND_OP_DATA);
 				}
-			} else if (opline->opcode == ZEND_FE_FREE
-					&& opline->extended_value & ZEND_FREE_ON_RETURN
-					&& opnum + 1 < op_array->last
-					&& ((opline + 1)->opcode == ZEND_RETURN
-						|| (opline + 1)->opcode == ZEND_RETURN_BY_REF
-						|| (opline + 1)->opcode == ZEND_GENERATOR_RETURN)) {
-				/* FE_FREE with ZEND_FREE_ON_RETURN immediately followed by RETURN frees
+			} else if ((opline->opcode == ZEND_FREE || opline->opcode == ZEND_FE_FREE) && opline->extended_value & ZEND_FREE_ON_RETURN) {
+				int jump_offset = 1;
+				while (((opline + jump_offset)->opcode == ZEND_FREE || (opline + jump_offset)->opcode == ZEND_FE_FREE)
+					&& (opline + jump_offset)->extended_value & ZEND_FREE_ON_RETURN) {
+					++jump_offset;
+				}
+				// loop var frees directly precede the jump (or return) operand, except that ZEND_VERIFY_RETURN_TYPE may happen first.
+				if ((opline + jump_offset)->opcode == ZEND_VERIFY_RETURN_TYPE) {
+					++jump_offset;
+				}
+				/* FREE with ZEND_FREE_ON_RETURN immediately followed by RETURN frees
 				 * the loop variable on early return. We need to split the live range
-				 * so GC doesn't access the freed variable after this FE_FREE.
-				 *
-				 * FE_FREE is included in the range only if it pertains to an early
-				 * return. */
-				uint32_t opnum_last_use = last_use[var_num]; // likely a FE_FREE
-				__auto_type opline_last_use = &op_array->opcodes[opnum_last_use];
-				if (opline_last_use->opcode == ZEND_FE_FREE &&
-						opline_last_use->extended_value & ZEND_FREE_ON_RETURN) {
-					/* another early return; we include the FE_FREE */
-					emit_live_range_raw(op_array, var_num, ZEND_LIVE_LOOP,
-						opnum + 2, opnum_last_use + 1);
-				} else if (opline_last_use->opcode == ZEND_FE_FREE &&
-						!(opline_last_use->extended_value & ZEND_FREE_ON_RETURN)) {
-					/* the normal return; don't include the FE_FREE */
-					emit_live_range_raw(op_array, var_num, ZEND_LIVE_LOOP,
-						opnum + 2, opnum_last_use);
-				} else {
-					/* if the last use is not FE_FREE, include it */
+				 * so GC doesn't access the freed variable after this FREE. */
+				uint32_t opnum_last_use = last_use[var_num];
+				zend_op *opline_last_use = op_array->opcodes + opnum_last_use;
+				ZEND_ASSERT(opline_last_use->opcode == opline->opcode); // any ZEND_FREE_ON_RETURN must be followed by a FREE without
+				if (opnum + jump_offset + 1 != opnum_last_use) {
 					emit_live_range_raw(op_array, var_num, ZEND_LIVE_LOOP,
-						opnum + 2, opnum_last_use + 1);
+							opnum + jump_offset + 1, opnum_last_use);
 				}
 
-				/* Update last_use so next range includes this FE_FREE */
-				last_use[var_num] = opnum + 1;
+				/* Update last_use so next range includes this FREE */
+				last_use[var_num] = opnum;
+
+				/* Store opline offset to loop end */
+				opline->op2.opline_num = opnum_last_use - opnum;
+				if (opline_last_use->extended_value & ZEND_FREE_ON_RETURN) {
+					opline->op2.opline_num += opline_last_use->op2.opline_num;
+				}
 			}
 		}
 		if (opline->op2_type & (IS_TMP_VAR|IS_VAR)) {

Zend/zend_vm_def.h

@@ -3134,7 +3134,7 @@ ZEND_VM_COLD_CONST_HANDLER(47, ZEND_JMPNZ_EX, CONST|TMPVAR|CV, JMP_ADDR)
 	ZEND_VM_JMP(opline);
 }
 
-ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, ANY)
+ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, LOOP_END)
 {
 	USE_OPLINE
 
@@ -3143,7 +3143,7 @@ ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, ANY)
 	ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
 }
 
-ZEND_VM_HOT_HANDLER(127, ZEND_FE_FREE, TMPVAR, ANY)
+ZEND_VM_HOT_HANDLER(127, ZEND_FE_FREE, TMPVAR, LOOP_END)
 {
 	zval *var;
 	USE_OPLINE
@@ -8055,50 +8055,11 @@ ZEND_VM_HANDLER(149, ZEND_HANDLE_EXCEPTION, ANY, ANY)
 		&& throw_op->extended_value & ZEND_FREE_ON_RETURN) {
 		/* exceptions thrown because of loop var destruction on return/break/...
 		 * are logically thrown at the end of the foreach loop, so adjust the
-		 * throw_op_num.
+		 * throw_op_num to the final loop variable FREE.
 		 */
-		const zend_live_range *range = find_live_range(
-			&EX(func)->op_array, throw_op_num, throw_op->op1.var);
-
-		/* free op1 of the corresponding RETURN - must use original throw_op_num
-		 * and first range, before any split-range skipping */
-		uint32_t range_end = range->end;
-		for (i = throw_op_num; i < range_end; i++) {
-			__auto_type current_opline = EX(func)->op_array.opcodes[i];
-			if (current_opline.opcode == ZEND_FREE
-			 || current_opline.opcode == ZEND_FE_FREE) {
-				if (current_opline.extended_value & ZEND_FREE_ON_RETURN) {
-					/* if this is a split end, the ZEND_RETURN is not included
-					 * in the range, so extend the range */
-					range_end++;
-				}
-				/* pass */
-			} else {
-				if (current_opline.opcode == ZEND_RETURN
-				 && (current_opline.op1_type & (IS_VAR|IS_TMP_VAR))) {
-					zval_ptr_dtor(EX_VAR(current_opline.op1.var));
-				}
-				break;
-			}
-		}
-
-		/* skip any split ranges to find the final range of the loop var and
-		 * adjust throw_op_num */
-		for (;;) {
-			if (range->end < EX(func)->op_array.last) {
-				__auto_type last_range_opline = EX(func)->op_array.opcodes[range->end - 1];
-				if (last_range_opline.opcode == ZEND_FE_FREE &&
-						(last_range_opline.extended_value & ZEND_FREE_ON_RETURN)) {
-					/* the range was split, skip to find the final range */
-					throw_op_num = range->end + 1;
-					range = find_live_range(
-						&EX(func)->op_array, throw_op_num, throw_op->op1.var);
-					continue;
-				}
-			}
-			break;
-		}
-		throw_op_num = range->end;
+		uint32_t new_throw_op_num = throw_op_num + throw_op->op2.opline_num;
+		cleanup_live_vars(execute_data, throw_op_num, new_throw_op_num);
+		throw_op_num = new_throw_op_num;
 	}
 
 	/* Find the innermost try/catch/finally the exception was thrown in */

Zend/zend_vm_execute.h

@@ -63,7 +63,7 @@
     "ZEND_VM_OP_NUM"          => 0x10,
     "ZEND_VM_OP_JMP_ADDR"     => 0x20,
     "ZEND_VM_OP_TRY_CATCH"    => 0x30,
-    // unused 0x40
+    "ZEND_VM_OP_LOOP_END"     => 0x40,
     "ZEND_VM_OP_THIS"         => 0x50,
     "ZEND_VM_OP_NEXT"         => 0x60,
     "ZEND_VM_OP_CLASS_FETCH"  => 0x70,
@@ -111,6 +111,7 @@
     "NUM"                  => ZEND_VM_OP_NUM,
     "JMP_ADDR"             => ZEND_VM_OP_JMP_ADDR,
     "TRY_CATCH"            => ZEND_VM_OP_TRY_CATCH,
+    "LOOP_END"             => ZEND_VM_OP_LOOP_END,
     "THIS"                 => ZEND_VM_OP_THIS,
     "NEXT"                 => ZEND_VM_OP_NEXT,
     "CLASS_FETCH"          => ZEND_VM_OP_CLASS_FETCH,

Zend/zend_vm_gen.php

@@ -28,9 +28,9 @@ $_main:
 0000 T1 = PRE_INC_STATIC_PROP string("prop") string("X")
 0001 T2 = ISSET_ISEMPTY_CV (empty) CV0($xx)
 0002 JMPZ T2 0005
-0003 FREE T1
+0003 FREE T1 loop-end(2)
 0004 RETURN null
 0005 FREE T1
 0006 RETURN int(1)
 LIVE RANGES:
-     1: 0001 - 0005 (tmp/var)
+     1: 0001 - 0003 (tmp/var)