Deprecate autovivification on false

[kamil-tekiela]

RFC: https://wiki.php.net/rfc/autovivification_false

Based on implementation TysonAndre#17

[nikic]

This will also need adjustments in zend_jit_helpers.c (grep for the error message).

[kamil-tekiela]

I have tried using this code in the test, but it doesn't even compile.

<?php

class A
{
    public  const array1 = [false];
}

A::array1[0][] = 42;

I get the error:

Cannot use temporary expression in write context

[nikic]

@dstogov Could you please take a look at my JIT implementation? I assumed it would be best to move handling of false to the slow path entirely, rather than generating JIT code to emit the deprecation. Does this make sense to you?

[nikic]

The arm64 build fails in zend_vm_execute.h, but I don't understand the error at all. There are no interesting changes to zend_vm_def.h and nothing that is ARM specific.

[nikic]

@shqking Do you have an idea where the ARM build failure on Travis is coming from? It's failing to build zend_execute.c, which doesn't have ARM specific code, so I'm a bit stumped.

[shqking]

@shqking Do you have an idea where the ARM build failure on Travis is coming from? It's failing to build zend_execute.c, which doesn't have ARM specific code, so I'm a bit stumped.

Hi @nikic as commented in this file zend_jit_arm64.dasc, I think one ')' is removed by accident. In my local machine, the building succeeds if we fix this typo.

[nikic]

@shqking Thanks! Unfortunately, the build still fails after fixing the typo: https://app.travis-ci.com/github/php/php-src/jobs/524706767#L1960

[shqking]

@shqking Thanks! Unfortunately, the build still fails after fixing the typo: https://app.travis-ci.com/github/php/php-src/jobs/524706767#L1960

It seems like one compiler warning.

However, in my local machine Ubuntu20+gcc10 (Ubuntu18+gcc7 is used in travis-ci)), using the same configure options as travis-ci, I didn't get this compiler warning. Let me try more times...

./configure --enable-option-checking=fatal --prefix=/home/travis/php-install --enable-debug --enable-zts --enable-phpdbg --enable-fpm --with-pdo-mysql=mysqlnd --with-mysqli=mysqlnd --with-pgsql --with-pdo-pgsql --with-pdo-sqlite --enable-intl --without-pear --enable-gd --with-jpeg --with-webp --with-freetype --with-xpm --enable-exif --with-zip --with-zlib --with-zlib-dir=/usr --enable-soap --enable-xmlreader --with-xsl --with-tidy --enable-sysvsem --enable-sysvshm --enable-shmop --enable-pcntl --with-readline --enable-mbstring --with-curl --with-gettext --enable-sockets --with-bz2 --with-openssl --with-gmp --enable-bcmath --enable-calendar --enable-ftp --with-pspell=/usr --with-enchant=/usr --with-kerberos --enable-sysvmsg --with-ffi --with-sodium --enable-zend-test=shared --enable-werror --with-pear

[nikic]

Okay, I suspect this is some kind of race condition in our build system, where we try to regenerate zend_vm_execute.h while building zend_execute.c. Probably the only relation this has to ARM is that the ARM builder on Travis has high parallelism. I'm not sure what exactly is wrong, but looking at the order in the log it seems plausible.

[shqking]

I still cannot reproduce this failure in my environment.

Yes. Your conjecture sounds reasonable.

[shqking]

The building on arm64 machine can pass now, but JIT/arm64 failed for the new test case, i.e. falsetoarray.phpt.

The error might be related to the opcode sequence ZEND_FETCH_DIM_W -> ZEND_ASSIGN_DIM.
As note in https://github.com/php/php-src/blob/master/ext/opcache/jit/zend_jit_arm64.dasc#L14027, RAX is reused in JIT/x86, I suspect in JIT/arm64, the corresponding REG0 doesn't hold the expected address.

Let me dig it deeper.

[shqking]

The patch below can fix this test failure.

diff --git a/ext/opcache/jit/zend_jit_arm64.dasc b/ext/opcache/jit/zend_jit_arm64.dasc
index 94595b66f7..1d93b298b7 100644
--- a/ext/opcache/jit/zend_jit_arm64.dasc
+++ b/ext/opcache/jit/zend_jit_arm64.dasc
@@ -11222,9 +11222,11 @@ static int zend_jit_fetch_dim(dasm_State    **Dst,
                        case ZEND_FETCH_DIM_W:
                        case ZEND_FETCH_LIST_W:
                                |       EXT_CALL zend_jit_fetch_dim_obj_w_helper, REG0
+                               |       mov REG0, RETVALx
                                break;
                        case ZEND_FETCH_DIM_RW:
                                |       EXT_CALL zend_jit_fetch_dim_obj_rw_helper, REG0
+                               |       mov REG0, RETVALx
                                break;
 //                     case ZEND_FETCH_DIM_UNSET:
 //                             |       EXT_CALL zend_jit_fetch_dim_obj_unset_helper, REG0

In JIT/x86, the return register, i.e. r0, is updated silently in [1]. However, when implementing JIT/arm64, we thought the return value of function zend_jit_fetch_dim_obj_rw_helper() is void, and hence mov REG0, RETVALx is omitted.

It seems that the return register is updated by one callee inside function zend_jit_fetch_dim_obj_rw_helper(). I suspect it's [2], which is added in this patch.
Note that this return value, i.e. r0 in x86 or REG0 in arm64 is used at [3] and [4] in JIT/x86 and JIT/arm64 respectively.

[1] https://github.com/php/php-src/blob/master/ext/opcache/jit/zend_jit_x86.dasc#L11883
[2] https://github.com/php/php-src/pull/7131/files?file-filters%5B%5D=.c&file-filters%5B%5D=.dasc&file-filters%5B%5D=.phpt#diff-8cb8c8a4f53eff1333144fb356d40fef020fffec5743f744da4547fc6b9526bdR1083
[3] https://github.com/php/php-src/blob/master/ext/opcache/jit/zend_jit_x86.dasc#L14877
[4] https://github.com/php/php-src/blob/master/ext/opcache/jit/zend_jit_arm64.dasc#L14027

IMHO, the workaround can fix the test failure, but I don't think it's a good fix...

Besides, with this workaround, I tested all the .phpt test cases under tests/, Zend/tests, ext/opcache/tests/jit/ with tracing JIT on arm64, and didn't find any test failure.

[nikic]

I believe the build system race condition should be fixed by 3025126.

I've tried to address the problem you mention in 8c2c5d9, by not setting the indirect_reference flag for MAY_BE_FALSE containers. That is, if we go through the slow path of fetch_dim, also bypass this optimization. Does that seem like a reasonable solution?

[shqking]

I believe the build system race condition should be fixed by 3025126.

I've tried to address the problem you mention in 8c2c5d9, by not setting the indirect_reference flag for MAY_BE_FALSE containers. That is, if we go through the slow path of fetch_dim, also bypass this optimization. Does that seem like a reasonable solution?

LGTM. Thanks.

[dstogov]

I checked only the JIT part and the general approach looks fine.