Fix readonly+clone JIT issues

[iluuu1994]

No description provided.

[dstogov]

What is IS_PROP_REINITABLE and where it came from?
It looks like I messed some committed hack. Please point me to that PR.

As I understood from a quick look, zend_readonly_property_modification_error() shouldn't be called on some rare condition. I think, Instead of generation this checking code for each property access, it should be moved into a new helper function.

I'll take a deep look on Monday.

[iluuu1994]

This was introduced for https://wiki.php.net/rfc/readonly_amendments (vote 2) in #10389.

[dstogov]

I really don't like the original path 3bcf2c37553
@kocsismate sorry I didn't review that commit. It would be better to ping me.

The patch introduces a hack with complex behaviour and logic spread among the code.
This almost always causes missed edge cases and subsequent issues.
Playing for a few minutes I found this weird case:
In clone we may assign to uninitialized read-only property, but for some reason we can't pass it by reference.

<?php
class Foo {
    readonly int $y;
    public function __construct(
        public readonly int $bar
    ) {}

    public function test(&$x) {
            $x = 4;
    }

    public function __clone()
    {
            $this->test($this->bar);
            $this->test($this->y);
    }
}
$a = new Foo(1);
var_dump($a);
$b = clone $a;
var_dump($b);
?>

I suggest to make a deeper review of 3bcf2c37553 fix all the issues and only then try to fix JIT. If more issues are found, I would prefer to revert 3bcf2c3 and re-commit it when all issues are fixed.

[iluuu1994]

@dstogov Thank you for your comments!

In clone we may assign to uninitialized read-only property, but for some reason we can't pass it by reference.

Unless I'm misunderstanding, I think this is the default behavior without clone too.

https://3v4l.org/ghZR5

The error message is a bit misleading, as we don't disallow all indirect modification but just references. Actually, we changed that. I forgot.

[kocsismate]

Unless I'm misunderstanding, I think this is the default behavior without clone too.

Yes, you cannot acquire reference on readonly properties. The readonly RFC doesn't highlight this too much, but that was the case since PHP 8.1.

[kocsismate]

@dstogov

The patch introduces a hack with complex behaviour and logic spread among the code.
This almost always causes missed edge cases and subsequent issues.

I understand your point of view, and I would appreciate if you could tell me what direction the implementation should rather take?

[dstogov]

I see, we have a difference in behaviour, because now we may work with uninitialized read-only properties and with already initialized ones. So the behaviour of the case above may be considered as expected, but it's not easy to understand.

I don't have any better solution out of the box, and most probably won't. I'll try to think a bit.

[dstogov]

@kocsismate As I expected, I can't suggest any other way to implement cloning of read-only properties. It like adding exceptions for some rule, then exceptions for exceptions, etc. I think, PHP took wrong direction few years ago, and over-complication of the sources that we have now don't cost many of new PHP features :(

@iluuu1994 This patch looks right. It may be improved a bit by removing few instructions on cold path and jumping to the hot path instead. This may fix (or open) some issues with null-to-array promotion. Patch for x86:

diff --git a/ext/opcache/jit/zend_jit_x86.dasc b/ext/opcache/jit/zend_jit_x86.dasc
index 42785e285f..27ed084fd9 100644
--- a/ext/opcache/jit/zend_jit_x86.dasc
+++ b/ext/opcache/jit/zend_jit_x86.dasc
@@ -12983,6 +12983,17 @@ static int zend_jit_fetch_obj(dasm_State          **Dst,
 			|	SET_ZVAL_TYPE_INFO res_addr, IS_OBJECT_EX
 			|	jmp >9
 			|2:
+			|	mov eax, dword [FCARG1a + offsetof(zval, u2.extra)]
+			|	test eax, IS_PROP_REINITABLE
+			|	jz >6
+			|	and eax, ~IS_PROP_REINITABLE
+			|	mov dword [FCARG1a + offsetof(zval, u2.extra)], eax
+			if (flags) {
+				|	jmp >3
+			} else {
+				|	jmp >4
+			}
+			|6:
 			|	mov FCARG1a, FCARG2a
 			|	SET_EX_OPLINE opline, r0
 			|	EXT_CALL zend_readonly_property_modification_error, r0
@@ -13034,7 +13045,7 @@ static int zend_jit_fetch_obj(dasm_State          **Dst,
 		if (opline->opcode == ZEND_FETCH_OBJ_W && (prop_info->flags & ZEND_ACC_READONLY)) {
 			if (!type_loaded) {
 				type_loaded = 1;
-				|	mov edx, dword [FCARG1a + prop_info->offset + 8]
+				|	mov edx, dword [FCARG1a + prop_info->offset + offsetof(zval, u1.type_info)]
 			}
 			|	IF_NOT_TYPE dl, IS_OBJECT, >4
 			|	GET_ZVAL_PTR r0, prop_addr
@@ -13044,12 +13055,20 @@ static int zend_jit_fetch_obj(dasm_State          **Dst,
 			|	jmp >9
 			|.cold_code
 			|4:
+			|	mov eax, dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)]
+			|	test eax, IS_PROP_REINITABLE
+			|	jz >6
+			|	and eax, ~IS_PROP_REINITABLE
+			|	mov dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)], eax
+			|	jmp >4
+			|6:
 			|	LOAD_ADDR FCARG1a, prop_info
 			|	SET_EX_OPLINE opline, r0
 			|	EXT_CALL zend_readonly_property_modification_error, r0
 			|	SET_ZVAL_TYPE_INFO res_addr, _IS_ERROR
 			|	jmp >9
 			|.code
+			|4:
 		}
 		if (opline->opcode == ZEND_FETCH_OBJ_W
 		 && (opline->extended_value & ZEND_FETCH_OBJ_FLAGS)

I think it won't be a problem to update ARM part accordingly.

[iluuu1994]

It may be improved a bit by removing few instructions on cold path and jumping to the hot path instead.

That looks better, thank you! I'll adjust this tomorrow.