I asked to the rest of the team if your PR would qualify as security fix (8.1 is EOL), if not then it would need to target PHP-8.2 instead. We shall see.

I asked to the rest of the team if your PR would qualify as security fix (8.1 is EOL), if not then it would need to target PHP-8.2 instead. We shall see.

Ah, I misread the contributing guide and version table. I'm happy to rebase on 8.2

Re-based to PHP-8.2

There was another small fix regarding parameters,

In non-JIT mode, IS_CONST types pass a pointer to a temporary _zval_struct into zend_observer_fcall_end for the retval parameter, resulting in two different _zval_struct objects with a matching zend_value. The new hook function in zend_test copies the zval from the executed hook return value to execute_data->return_value. Consequently, the retval parameter in observer_end still referred to the original zval stored in the temporary _zval_struct.

This caused the test to display different return values for JIT and non-JIT modes, while still producing the expected final result outside the observer.

Data comparison inside observer_end:

BEFORE 2nd Fix:
JIT:
execute_data->return_value: ptr:d0216080 zval.value.lval:41b3cf90
retval:			 ptr:421e0830 zval.value.lval:41b3cf90
NON:
execute_data->return_value: ptr:fc016080 zval.value.lval:4065bf90
retval:			 ptr:fc016080 zval.value.lval:4065bf90
AFTER 2nd Fix:
JIT:
execute_data->return_value: ptr:b5016080 zval.value.lval:6aa32c78
retval:			 ptr:b5016080 zval.value.lval:6aa32c78
NON:
execute_data->return_value: ptr:c1616080 zval.value.lval:20232c78
retval:			 ptr:c1616080 zval.value.lval:20232c78

@segrax Thank you for your PR! Note that Dmitry, who is responsible for the JIT, is on vacation for the next two weeks. So it might take a minute for you to get a proper review.

cc: @bwoebi

Thanks!
Now it's clear that the patch doesn't affect JIT without observer.
Porting to master also should be simpler.

@bwoebi please check the observer related behavior and merge this.

I'm confused, like the patch seems to be only concerned with the pass case and IS_CONST and IS_TMP_VAR?

Reading the IS_CV case, isn't it sending the pointer to the CV rather than the ret_addr to zend_observer_fcall_end?
And same for the IS_TMP_VAR and the IS_VAR case (the else part)?

I must admit I don't really understand what the

		if (Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);
			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}

part is doing.

At least - IF the value is returned - it's always supposed to be the return address, not sometimes op1_addr. Like, wouldn't this be correct:

	if (return_value_used == 0) {
		if (ZEND_OBSERVER_ENABLED && Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);
			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}
		ret_addr = op1_addr;
	} else if (..) {
		// everything else unchanged here for IS_CV/VAR/TMP_VAR/CONST handling
	}
	if (ZEND_OBSERVER_ENABLED) {
		|	LOAD_ZVAL_ADDR FCARG2a, ret_addr
		|	mov FCARG1a, FP
		|	SET_EX_OPLINE opline, r0
		|	EXT_CALL zend_observer_fcall_end, r0
	}

Maybe @dstogov can help here?

The last @bwoebi suggestion seems right!

I'm confused, like the patch seems to be only concerned with the pass case and IS_CONST and IS_TMP_VAR?

@bwoebi yeah, currently it only fixes the issue i found while working with the OpenTelemetry extension, there is likely the same issue with the other types, but too be honest, i wasn't entirely sure what everything here was doing.

Figured I would submit what I had, and work from there

At least - IF the value is returned - it's always supposed to be the return address, not sometimes op1_addr. Like, wouldn't this be correct:

	if (return_value_used == 0) {
		if (ZEND_OBSERVER_ENABLED && Z_MODE(op1_addr) == IS_REG) {
			zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);
			if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
				return 0;
			}
			op1_addr = dst;
		}
		ret_addr = op1_addr;
	} else if (..) {
		// everything else unchanged here for IS_CV/VAR/TMP_VAR/CONST handling
	}
	if (ZEND_OBSERVER_ENABLED) {
		|	LOAD_ZVAL_ADDR FCARG2a, ret_addr
		|	mov FCARG1a, FP
		|	SET_EX_OPLINE opline, r0
		|	EXT_CALL zend_observer_fcall_end, r0
	}

It seems this change causes a test failure, but only in the debug builds (my local build is fine, but I also haven't checked what flags are being used in the debug pipeline builds).

EX(opline) is correctly set for nested JIT user code calls [ext/opcache/tests/jit/gh13772.phpt]

@segrax did you see ext/opcache/tests/jit/gh13772.phpt test failure?

@segrax did you see ext/opcache/tests/jit/gh13772.phpt test failure?

Hey @dstogov,

Yeah, I did see the failure, it came from the change in the last recommendation, but it seems to have caused the test failure, So was waiting to hear back from @bwoebi before reverting it or making any further changes

It seems this change causes a test failure, but only in the debug builds (my local build is fine, but I also haven't checked what flags are being used in the debug pipeline builds).

EX(opline) is correctly set for nested JIT user code calls [ext/opcache/tests/jit/gh13772.phpt]

OK. ping we when you'll need the next round of my review.

Hey @dstogov, I've reverted that last change

Hey @dstogov and @bwoebi,

Just checking to see if there's been any progress on this,
Let me know if there’s anything I can help with to move things along

Thanks!