Allow assignment to properties of objects stored in constants

[iluuu1994]

Fixes GH-11781

[iluuu1994]

Hmm, FOO->array[] = 42; is still an issue. I'll see how easy it is to fix.

[iluuu1994]

Ok, I found a much simpler solution that works for this case too.

[dstogov]

Can you clarify? The lhs of -> is neither a reference nor an indirect value in this case, as it is the value of the constant, i.e. a IS_OBJ zval.

Looking into this patch it's not clear what instructions may be changed. I guess it is only FETCH_CONST and it is probably safe to change its result from TMP to VAR, but it's defenetly not safe for other instructions (as FETCH_DIM/OBJ_R).

If this patch intends to change only the FETCH_CONST instruction, it's better to do this in a more obvious way.

I can think of three options:

Change the result of FETCH_CONST to VAR only where necessary (what I do in this PR).

Adding ZEND_ASSERT(some_opline->opcode == ZEND_FETCH_CONSTANT) is going to be enough to make this clear.
What about FETCH_CLASS_CONSTANT?

Add TMP to OP1 in all the relevant handlers (ZEND_ASSIGN_OBJ, ZEND_FETCH_OBJ_W, etc). With TMPVAR this shouldn't actually change the size of the VM, or impact performance.
Change the result of all FETCH_CONST to VAR. This would have a negative performance impact due to added reference/indirect checks when using the result.

These approches look worse.

[iluuu1994]

Ah, thank you for clarifying. You're right, adding an assert would certainly be more clear.

but it's defenetly not safe for other instructions (as FETCH_DIM/OBJ_R).

I'm still a bit confused about this. This is my understanding, please correct me if I'm wrong. VAR differs from TMP in that it adds IS_REF and IS_INDIRECT unwrapping when necessary. Thus, changing an operand from TMP to VAR would perform these checks, even if they are unnecessary. As long as the handler has a specialization for VAR, that should work fine. The opposite is of course not sound, as omitting the IS_REF and IS_INDIRECT unwrapping would lead to unexpected values. It seems to me FETCH_DIM_R with the lhs being a VAR should work fine, even if it is "only" a TMP (i.e. we know it can't contain references or indirect values.

Sorry if I'm missing the point.

[dstogov]

Sorry if I'm missing the point.

If you change FETCH_DIM_R/TMP to FETCH_DIM_R/VAR, the following ASSING_DIM/OBJ won't change the element of the first array anyway. In addition the behaviour of FETCH_OBJ_R/TMP -> VAR may be affected by custom object handlers.

May be I'm wrong and changing TMP to VAR will work, but anyway I wouldn't recommend to do this without an extra care.

[valerio-bozzolan]

(Title typo: -cosntants +constants)

[dstogov]

This means that FETCH_CONSTANT opline has result_type IS_TMP_VAR and the following instruction use it as IS_VAR.

<?php
class Foo {
    public $array = [];
}
const FOO = new Foo();
FOO->array[] = 42;

0000 DECLARE_CONST string("FOO") zval(type=11)
0001 T1 = FETCH_CONSTANT string("FOO")
0002 V0 = FETCH_OBJ_W (dim write) V1 string("array")
0003 ASSIGN_DIM V0 NEXT
0004 OP_DATA int(42)

This may confuse some code in optimizer where we check if result and usage types are the same.
Can you also change the result_type of FETCH_CONSANT?

[iluuu1994]

Good catch. I wonder if at that point it's better to just adjust the VM handlers to TMPVAR, rather than convoluting the compiler. I suppose alternatively we could use a QM_ASSIGN here too, unless the optimizer removes it again.

[dstogov]

Do you see any problem in setting FETCH_CONSANT.result_type to IS_TMP_VAR?

I wonder if at that point it's better to just adjust the VM handlers to TMPVAR

This should introduce some overhead for all modified FETCH_..._W/RW or no?

[iluuu1994]

Do you see any problem in setting FETCH_CONSANT.result_type to IS_TMP_VAR?

I don't see a problem. It might just introduce slightly more complexity in the compiler.

This should introduce some overhead for all modified FETCH_..._W/RW or no?

I didn't think so, because we're changing them from VAR to TMPVAR. This should not actually change the specialized handlers. I will verify whether my assumption is correct. However, I will try your suggestion (changing result of FETCH_CONSTANT) first.

[dstogov]

I mean changing FETCH_CONSTANT.result_type only for this case.

ZEND_ASSERT(opline->opcode == ZEND_FETCH_CONSTANT);
opline->result_type = IS_TMP_VAR;

May be my idea is wrong.

[iluuu1994]

Doesn't seem worth the trouble.