assertion failure spl_fixedarray

Description

The following code:

<?php
set_error_handler('func_get_args');
function foo() {
for ($cnt = 0; $cnt < 6; $cnt++) {
$b = new SplFixedArray(1);
$b[0] = $a;
}
}
@foo();

Resulted in this output:

php: /home/phpfuzz/WorkSpace/flowfusion/php-src/ext/spl/spl_fixedarray.c:357: zend_ulong spl_offset_convert_to_ulong(const zval *): Assertion `!(((zend_executor_globals *) (((char*) _tsrm_ls_cache)+(executor_globals_offset)))->exception)' failed.
Aborted (core dumped)

To reproduce:

./php-src/sapi/cli/php  ./test.php

Commit:

ac9392b855cc4db6792a8379223e1fd1bbebd157

Configurations:

CC="clang-12" CXX="clang++-12" CFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" CXXFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" ./configure --enable-debug --enable-address-sanitizer --enable-undefined-sanitizer --enable-re2c-cgoto --enable-fpm --enable-litespeed --enable-phpdbg-debug --enable-zts --enable-bcmath --enable-calendar --enable-dba --enable-dl-test --enable-exif --enable-ftp --enable-gd --enable-gd-jis-conv --enable-mbstring --enable-pcntl --enable-shmop --enable-soap --enable-sockets --enable-sysvmsg --enable-zend-test --with-zlib --with-bz2 --with-curl --with-enchant --with-gettext --with-gmp --with-mhash --with-ldap --with-libedit --with-readline --with-snmp --with-sodium --with-xsl --with-zip --with-mysqli --with-pdo-mysql --with-pdo-pgsql --with-pgsql --with-sqlite3 --with-pdo-sqlite --with-webp --with-jpeg --with-freetype --enable-sigchild --with-readline --with-pcre-jit --with-iconv

Operating System:

Ubuntu 20.04 Host, Docker 0599jiangyc/flowfusion:latest

This report is automatically generated by FlowFusion

PHP Version

ac9392b

Operating System

No response

@arnaud-lb

This feels like a VM problem to me. The point is that after getting the zval pointer for the dimension CV or the value CV, we don't check for an exception. So that means the dimension handlers are executed regardless of there being an exception, and then the assertion in SplFixedArray fails of course.

That also means that the following code:

<?php
set_error_handler(fn ($_,$str) => throw new Error($str));
$b = [];
try {
    $b[0] = $a;
} catch(Error $e) {
    echo $e->getMessage(), "\n";
}
var_dump($b);

results in

Undefined variable $a
array(1) {
  [0]=>
  NULL
}

despite the access to $a already throwing an exception. So I expected an empty array at the end honestly.
This can be fixed in the VM, and maybe should be to be 100% correct, but then we have to add a few more checks for something that should likely never happen anyway. So I'm reluctant do that. Especially since we'll need to revise CV accesses for PHP9 anyway.
Another fix is to just comment out the assertion in SplFixedArray. The reason it was added in the first place is to avoid an exception check for the common case (the compiler was able to utilize this assertion/assume).
cc @arnaud-lb @iluuu1994

Interestingly the following snippet works as expected (offsetSet() is not called) because zend_call_function() returns early when EG(exception) is set:

set_error_handler(fn ($_,$str) => throw new Error($str));

$b = new class implements ArrayAccess {
    function offsetSet($name, $value): void {
        var_dump($name, $value);
    }
    function offsetGet($value): mixed {
        var_dump($value);
    }
    function offsetExists($name): bool {
        return true;
    }
    function offsetUnset($name): void {
    }
};

$b[0] = $a;

However this doesn't feel right. I agree the VM should check for exceptions here, after calling zval_undefined_cv() (which calls the error handler). The extra check should have minimal overhead for being in a slow path, and will be eliminated from non-OP_DATA_CV handlers.

Possibly we have the same issue for ASSIGN_OBJ and all op handlers that use zval_undefined_cv() before interacting with other zvals.

The right long term fix is probably #12805. IMHO we should delay error handlers until the next interrupt check, just like signals and timeouts.

I think this is the default behavior for pretty much all handlers. E.g. GET_OP1_ZVAL_PTR() can call zval_undefined_cv(). However, it won't actually end execution, it will just use NULL (&EG(uninitialized_zval)) for the execution of the current handler. There might be other exception checks in the handler which will stop execution. Changing this might take quite a bit of refactoring, given that many get_zval_ptr functions are affected. They are forcefully inlined, so converting them to macros that can return from the caller using HANDLE_EXCEPTION() might be an option. This should only slightly increase assembly size, namely by inserting returns for these error paths.

The right long term fix is probably #12805. IMHO we should delay error handlers until the next interrupt check, just like signals and timeouts.

I think this will be the wrong solution again once we promote access on undeclared variables to errors.

I just realized this won't work well either, because we very often need to clean up on exit, e.g. FREE_OP1();. An option would be to make the get_zval_ptr functions that potentially operate on CVs macros, and add a {} block that injects some cleanup code into the zval_undefined_cv() branch, but this means a lot of code churn. The alternative would be to just accept execution can continue for the remainder of the handler, with null used as a value. This doesn't matter much for some handlers, but more for others. E.g. $object + $undef will call do_operation() with null.

I'm not sure yet what the best approach is.

We could move the undefined check to a separate opcode, emit the opcode when a CV is accessed, and rely on the optimizer to remove the unnecessary ones (when we can determine that a CV is always defined before a use).

This may have a performance impact, but if most uses can be determined to be defined, this may be positive.

@Girgias

We could move the undefined check to a separate opcode

@Girgias got this idea before and talked to me about that briefly. I was skeptical because it's an extra dispatch and handler overhead. If the optimizer can eliminate most of them then this can be positive indeed, but I don't know.

Anyway, I'm leaning towards just fixing this in SPL (by refactoring the code in such a way we don't need the exception assertion as an optimization hint anyway).

This should indeed work well with opcache. I wonder if we can have very simple liveliness tracking during compilation to avoid a penalty without opcache. This shouldn't be too hard.

I would imagine that any function body would be able to elide them at compile time, as it seems very unlikely that an undefined variable is used in such a body.

The annoying thing for global variables is that it is possible to unset them via $GLOBALS https://3v4l.org/JtuOa

Main functions will need special care in general due to access to globals in error handlers. They can probably compile to FETCH_R instead if we want to drop the check from all other handlers.

Although it would be great if somebody else could run my patch, because I'm seeing weird performance behavior on my machine again. Forcefully eliding all CHECK_VAR opcodes while still removing the many paths from the VM still shows a degradation for me. My machine is useless for benchmarking. 😒

I checked out your patch, and there's missed optimization opportunities for CHECK_VAR, and a few things that will be hard to further improve. First, if you run Zend/bench.php and dump the opcodes you'll see for ary, ary2, ary3, hash1, ... a lot of CHECK_VARs, especially for ary2. For ary2 it's easy to see it's a pity because they are in a loop, and the first array access on $X would cause autovivification so the other CHECK_VARs are redundant. Similarly for hash1 we have this in a loop => bad.

Secondly, I'm not going to even run Symfony, just the built-in benchmarks already show a significant slowdown. e.g. ~0.251 after your patch vs ~0.221 before on Zend/bench.php. Also a noticeable difference on Zend/micro_bench.php, although less severe.

I didn't check bench.php yet. Can you see improvements when commenting the following line?

master...iluuu1994:php-src:undeclared-var-error#diff-85701127596aca0e597bd7961b5d59cdde4f6bb3e2a109a22be859ab7568b4d2R2896

I can verify your finding. The auto-vivification itself is not the problem, but the loop isn't guaranteed to execute, hence creating a phi-node for the undef $X and one for the $X created in the loop, which prevents elimination of CHECK_VAR. Even hard-coding the $n to 10 doesn't fix this, so loops that are guaranteed to run is something the optimizer currently doesn't seem to understand.

Tbf, I think this is not idiomatic code nowadays. Usually the variable will be before the loop and populated in the loop. Still, would be nice if we could support this.

While the question whether or not the loop executes is part of the problem, what I said is also a problem. Take a look at BB4 of ary2.

Can you see improvements when commenting the following line?

~0.212 now instead of ~0.221, so an improvement of ~4.1%.

so loops that are guaranteed to run is something the optimizer currently doesn't seem to understand.

This is often hard to prove in real code.

Testing with the emit commented out, I don't see a performance difference on Symfony demo between stock PHP and your patch.

While the question whether or not the loop executes is part of the problem, what I said is also a problem. Take a look at BB4 of ary2.

Ah, yes indeed. I have already started refactoring CHECK_VAR to create a new definition that removes MAY_BE_UNDEF so that consecutive accesses don't repeat the check. However, it isn't quite as straight-forward because the definition then breaks the DCE. I'm sure we could find a solution to that one.

For the second loop in ary2, it would also be great if CHECK_VAR for $X could be lifted out of the loop, but this is likely not a trivial task, and is only behavior-preserving if there are no side-effects before $X is accessed.

Testing with the emit commented out, I don't see a performance difference on Symfony demo between stock PHP and your patch.

That is... less promising. I would have expected at least something, given so many paths are affected, not just in terms of instructions executed but also instruction cache pressure.

I get -0.35% instruction count under valgrind with php-cgi -T1,1, and -4% wall time on this benchmark when the line is commented on a quiet system:

hyperfine --warmup 1 -L php base,undecl '/tmp/{php}/sapi/cli/php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 --repeat 10 Zend/bench.php'
Benchmark 1: /tmp/base/sapi/cli/php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 --repeat 10 Zend/bench.php
  Time (mean ± σ):      1.621 s ±  0.005 s    [User: 1.591 s, System: 0.026 s]
  Range (min … max):    1.615 s …  1.630 s    10 runs
 
Benchmark 2: /tmp/undecl/sapi/cli/php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 --repeat 10 Zend/bench.php
  Time (mean ± σ):      1.564 s ±  0.004 s    [User: 1.532 s, System: 0.028 s]
  Range (min … max):    1.558 s …  1.569 s    10 runs
 
Summary
  /tmp/undecl/sapi/cli/php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 --repeat 10 Zend/bench.php ran
    1.04 ± 0.00 times faster than /tmp/base/sapi/cli/php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 --repeat 10 Zend/bench.php

This seems promising to me :)

Edit: didn't see #18274 (comment), but -4% on bench.php is still nice and can make some difference on real code. Plus it simplifies op code handlers.

For the second loop in ary2, it would also be great if CHECK_VAR for $X could be lifted out of the loop, but this is likely not a trivial task, and is only behavior-preserving if there are no side-effects before $X is accessed.

Unroll their first iteration may be a good way to improve both loops.

Defer user error handlers raised from user-code opcodes

Navigation Menu

Repository navigation

assertion failure spl_fixedarray #18274

Description

PHP Version

Operating System

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

assertion failure spl_fixedarray #18274

Description

Description

PHP Version

Operating System

Activity

ndossche commented on Apr 8, 2025

arnaud-lb commented on Apr 9, 2025

iluuu1994 commented on Apr 9, 2025

iluuu1994 commented on Apr 10, 2025

arnaud-lb commented on Apr 10, 2025

ndossche commented on Apr 10, 2025

iluuu1994 commented on Apr 10, 2025

Girgias commented on Apr 11, 2025

iluuu1994 commented on Apr 11, 2025

1 remaining item

iluuu1994 commented on Apr 11, 2025

ndossche commented on Apr 11, 2025

iluuu1994 commented on Apr 11, 2025

ndossche commented on Apr 11, 2025

ndossche commented on Apr 11, 2025

iluuu1994 commented on Apr 11, 2025

arnaud-lb commented on Apr 12, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions