Change YIELD/YIELD_FROM to do not increment opline (#15328)

YIELD and YIELD_FROM increment opline before returning, but in most places
we need the opline to point to the YIELD and YIELD_FROM.

Here I change YIELD / YIELD_FROM to not increment opline. This simplifies the
code and fixes GH-15275 in a better way.

Closes GH-15328

Author: arnaud-lb

UPGRADING.INTERNALS

@@ -360,6 +360,11 @@ PHP 8.4 INTERNALS UPGRADE NOTES
   that provide a frameless handler (search for usages of the
   ZEND_FRAMELESS_FUNCTION macro).
 
+* CREATE_GENERATOR now initializes the generator with opline pointing to the
+  CREATE_GENERATOR op (before, opline was set to the next op).
+
+* YIELD and YIELD_FROM do not increment the opline anymore.
+
 ========================
 5. SAPI changes
 ========================

Zend/zend_execute.c

@@ -4711,19 +4711,7 @@ ZEND_API void zend_cleanup_unfinished_execution(zend_execute_data *execute_data,
 
 ZEND_API ZEND_ATTRIBUTE_DEPRECATED HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer)
 {
-	bool suspended_by_yield = false;
-
-	if (Z_TYPE_INFO(EX(This)) & ZEND_CALL_GENERATOR) {
-		ZEND_ASSERT(EX(return_value));
-
-		/* The generator object is stored in EX(return_value) */
-		zend_generator *generator = (zend_generator*) EX(return_value);
-		ZEND_ASSERT(execute_data == generator->execute_data);
-
-		suspended_by_yield = !(generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING);
-	}
-
-	return zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, suspended_by_yield);
+	return zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, false);
 }
 
 ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer, bool suspended_by_yield)
@@ -4761,27 +4749,20 @@ ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_d
 		zend_get_gc_buffer_add_zval(gc_buffer, &extra_named_params);
 	}
 
+	uint32_t op_num;
+	if (UNEXPECTED(execute_data->opline->opcode == ZEND_HANDLE_EXCEPTION)) {
+		op_num = EG(opline_before_exception) - op_array->opcodes;
+	} else {
+		op_num = execute_data->opline - op_array->opcodes;
+	}
+	ZEND_ASSERT(op_num < op_array->last);
+
 	if (call) {
-		uint32_t op_num;
-		if (UNEXPECTED(execute_data->opline->opcode == ZEND_HANDLE_EXCEPTION)) {
-			op_num = EG(opline_before_exception) - op_array->opcodes;
-		} else {
-			op_num = execute_data->opline - op_array->opcodes;
-		}
-		ZEND_ASSERT(op_num < op_array->last);
-		if (suspended_by_yield) {
-			/* When the execution was suspended by yield, EX(opline) points to
-			 * next opline to execute. Otherwise, it points to the opline that
-			 * suspended execution. */
-			op_num--;
-			ZEND_ASSERT(EX(func)->op_array.opcodes[op_num].opcode == ZEND_YIELD
-				|| EX(func)->op_array.opcodes[op_num].opcode == ZEND_YIELD_FROM);
-		}
 		zend_unfinished_calls_gc(execute_data, call, op_num, gc_buffer);
 	}
 
 	if (execute_data->opline != op_array->opcodes) {
-		uint32_t i, op_num = execute_data->opline - op_array->opcodes - 1;
+		uint32_t i;
 		for (i = 0; i < op_array->last_live_range; i++) {
 			const zend_live_range *range = &op_array->live_range[i];
 			if (range->start > op_num) {

Zend/zend_generators.c

@@ -116,8 +116,7 @@ static void zend_generator_cleanup_unfinished_execution(
 {
 	zend_op_array *op_array = &execute_data->func->op_array;
 	if (execute_data->opline != op_array->opcodes) {
-		/* -1 required because we want the last run opcode, not the next to-be-run one. */
-		uint32_t op_num = execute_data->opline - op_array->opcodes - 1;
+		uint32_t op_num = execute_data->opline - op_array->opcodes;
 
 		if (UNEXPECTED(generator->frozen_call_stack)) {
 			/* Temporarily restore generator->execute_data if it has been NULLed out already. */
@@ -299,9 +298,7 @@ static void zend_generator_dtor_storage(zend_object *object) /* {{{ */
 		return;
 	}
 
-	/* -1 required because we want the last run opcode, not the
-	 * next to-be-run one. */
-	op_num = ex->opline - ex->func->op_array.opcodes - 1;
+	op_num = ex->opline - ex->func->op_array.opcodes;
 	try_catch_offset = -1;
 
 	/* Find the innermost try/catch that we are inside of. */
@@ -331,7 +328,8 @@ static void zend_generator_dtor_storage(zend_object *object) /* {{{ */
 			Z_OBJ_P(fast_call) = NULL;
 			Z_OPLINE_NUM_P(fast_call) = (uint32_t)-1;
 
-			ex->opline = &ex->func->op_array.opcodes[try_catch->finally_op];
+			/* -1 because zend_generator_resume() will increment it */
+			ex->opline = &ex->func->op_array.opcodes[try_catch->finally_op] - 1;
 			generator->flags |= ZEND_GENERATOR_FORCED_CLOSE;
 			zend_generator_resume(generator);
 
@@ -516,9 +514,6 @@ static void zend_generator_throw_exception(zend_generator *generator, zval *exce
 	/* Throw the exception in the context of the generator. Decrementing the opline
 	 * to pretend the exception happened during the YIELD opcode. */
 	EG(current_execute_data) = generator->execute_data;
-	generator->execute_data->opline--;
-	ZEND_ASSERT(generator->execute_data->opline->opcode == ZEND_YIELD
-			|| generator->execute_data->opline->opcode == ZEND_YIELD_FROM);
 	generator->execute_data->prev_execute_data = original_execute_data;
 
 	if (exception) {
@@ -527,8 +522,6 @@ static void zend_generator_throw_exception(zend_generator *generator, zval *exce
 		zend_rethrow_exception(EG(current_execute_data));
 	}
 
-	generator->execute_data->opline++;
-
 	/* if we don't stop an array/iterator yield from, the exception will only reach the generator after the values were all iterated over */
 	if (UNEXPECTED(Z_TYPE(generator->values) != IS_UNDEF)) {
 		zval_ptr_dtor(&generator->values);
@@ -621,7 +614,7 @@ ZEND_API zend_generator *zend_generator_update_current(zend_generator *generator
 	zend_generator_remove_child(&new_root_parent->node, new_root);
 
 	if (EXPECTED(EG(exception) == NULL) && EXPECTED((OBJ_FLAGS(&generator->std) & IS_OBJ_DESTRUCTOR_CALLED) == 0)) {
-		zend_op *yield_from = (zend_op *) new_root->execute_data->opline - 1;
+		zend_op *yield_from = (zend_op *) new_root->execute_data->opline;
 
 		if (yield_from->opcode == ZEND_YIELD_FROM) {
 			if (Z_ISUNDEF(new_root_parent->retval)) {
@@ -636,8 +629,6 @@ ZEND_API zend_generator *zend_generator_update_current(zend_generator *generator
 					generator->execute_fake.prev_execute_data = original_execute_data;
 				}
 
-				/* ZEND_YIELD(_FROM) already advance, so decrement opline to throw from correct place */
-				new_root->execute_data->opline--;
 				zend_throw_exception(zend_ce_ClosedGeneratorException, "Generator yielded from aborted, no return value available", 0);
 
 				EG(current_execute_data) = original_execute_data;
@@ -815,15 +806,6 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */
 			generator->flags &= ~ZEND_GENERATOR_IN_FIBER;
 			return;
 		}
-		if (UNEXPECTED(EG(exception))) {
-			/* Decrementing opline_before_exception to pretend the exception
-			 * happened during the YIELD_FROM opcode. */
-			if (generator->execute_data) {
-				ZEND_ASSERT(generator->execute_data->opline == EG(exception_op));
-				ZEND_ASSERT((EG(opline_before_exception)-1)->opcode == ZEND_YIELD_FROM);
-				EG(opline_before_exception)--;
-			}
-		}
 		/* If there are no more delegated values, resume the generator
 		 * after the "yield from" expression. */
 	}
@@ -834,6 +816,16 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */
 	}
 
 	/* Resume execution */
+	ZEND_ASSERT(generator->execute_data->opline->opcode == ZEND_GENERATOR_CREATE
+			|| generator->execute_data->opline->opcode == ZEND_YIELD
+			|| generator->execute_data->opline->opcode == ZEND_YIELD_FROM
+			/* opline points to EG(exception_op), which is a sequence of
+			 * ZEND_HANDLE_EXCEPTION ops, so the following increment is safe */
+			|| generator->execute_data->opline->opcode == ZEND_HANDLE_EXCEPTION
+			/* opline points to the start of a finally block minus one op to
+			 * account for the following increment */
+			|| (generator->flags & ZEND_GENERATOR_FORCED_CLOSE));
+	generator->execute_data->opline++;
 	generator->flags |= ZEND_GENERATOR_CURRENTLY_RUNNING;
 	if (!ZEND_OBSERVER_ENABLED) {
 		zend_execute_ex(generator->execute_data);
@@ -880,7 +872,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */
 	}
 
 	/* yield from was used, try another resume. */
-	if (UNEXPECTED((generator != orig_generator && !Z_ISUNDEF(generator->retval)) || (generator->execute_data && (generator->execute_data->opline - 1)->opcode == ZEND_YIELD_FROM))) {
+	if (UNEXPECTED((generator != orig_generator && !Z_ISUNDEF(generator->retval)) || (generator->execute_data && generator->execute_data->opline->opcode == ZEND_YIELD_FROM))) {
 		generator = zend_generator_get_current(orig_generator);
 		goto try_again;
 	}

Zend/zend_vm_def.h

@@ -4609,7 +4609,7 @@ ZEND_VM_HANDLER(139, ZEND_GENERATOR_CREATE, ANY, ANY)
 		generator->execute_fake.prev_execute_data = NULL;
 		ZVAL_OBJ(&generator->execute_fake.This, (zend_object *) generator);
 
-		gen_execute_data->opline = opline + 1;
+		gen_execute_data->opline = opline;
 		/* EX(return_value) keeps pointer to zend_object (not a real zval) */
 		gen_execute_data->return_value = (zval*)generator;
 		call_info = Z_TYPE_INFO(EX(This));
@@ -8416,10 +8416,6 @@ ZEND_VM_HANDLER(160, ZEND_YIELD, CONST|TMP|VAR|CV|UNUSED, CONST|TMPVAR|CV|UNUSED
 		generator->send_target = NULL;
 	}
 
-	/* We increment to the next op, so we are at the correct position when the
-	 * generator is resumed. */
-	ZEND_VM_INC_OPCODE();
-
 	/* The GOTO VM uses a local opline variable. We need to set the opline
 	 * variable in execute_data so we don't resume at an old position. */
 	SAVE_OPLINE();
@@ -8522,10 +8518,6 @@ ZEND_VM_C_LABEL(yield_from_try_again):
 	/* This generator has no send target (though the generator we delegate to might have one) */
 	generator->send_target = NULL;
 
-	/* We increment to the next op, so we are at the correct position when the
-	 * generator is resumed. */
-	ZEND_VM_INC_OPCODE();
-
 	/* The GOTO VM uses a local opline variable. We need to set the opline
 	 * variable in execute_data so we don't resume at an old position. */
 	SAVE_OPLINE();