Fix error handling during opcache compilation

[arnaud-lb]

Possible fix for GH-17422
Based on #17627

Current behavior

Currently, Opcache disables the user-defined error handler during compilation. When opcache.record_errors is enabled, errors are replayed when loading the script from cache later, this time without disabling the user-defined error handler.

Changes

This PR makes the following changes:

Errors emitted during compilation are delayed and handled after compilation, when it's safe to execute user-defined error handlers. As before, when opcache.record_errors is enabled, errors are also replayed when loading the script from cache and user-defined error handler are called, if any.

This is done by changing the EG(record_errors) mechanism so that errors are not only recorded, but also delayed.

Class linking:

Since class linking uses the same mechanism, errors emitted during class linking are also delayed (with Opcache), and replayed after linking. Exceptions thrown by user-defined error handlers are not promoted to fatal errors anymore (when opcache is enabled).

Fatal errors:

Fatal errors are not safe to delay, so they are always processed immediately. Any delayed errors are also processed, but unfortunately it's not safe to execute user-defined error handlers at this point, so these errors still bypass them.

Notes

This targets master because of the behavior changes.

We could apply the same behavior when opcache is disabled. (Edit: done)

UPGRADING draft:

Core:
 * Errors emitted during compilation and class linking are now handled
   after compilation or class linking. However, fatal errors emitted during
   compilation or class linking cause any delayed error to be handled
   immediately, without calling user-defined error handlers.
 * Exceptions thrown by user-defined error handlers during class linking
   are not promoted to fatal errors anymore and do not prevent linking.

[TimWolla]

Some first remarks regarding the tests. Did not yet work through the C code.

[TimWolla]

Should we also have a test that tries to catch the exception or are the new tests in ext/opcache/tests/gh17422/ sufficient?

[TimWolla]

This one fails when OPcache is not loaded. It still says the “During inheritance of C” bit. Thus:

We could apply the same behavior when opcache is disabled.

This would make sense to me.

[TimWolla]

The changes all make sense to me as far as I'm qualified to tell 😄

[iluuu1994]

A thought before reviewing this in detail: Should this behavior be unified for opcache / no opcache? Consider this example:

// a.php
set_error_handler(function () {
    require __DIR__ . '/c.php';
    Foo::test();
});
require __DIR__ . '/b.php';

// b.php
class Foo  {
    public static function test() {
        switch (true) {
            default:
                continue;
        }
        echo "b.php\n";
    }
}

// c.php
class Foo  {
    public static function test() {
        echo "c.php\n";
    }
}

This behavior will diverge currently, printing b.php with opcache, but c.php without (edit: actually it would error on inclusion in the error handler), before eventually erroring with "Cannot redeclare class Foo" on both. It's not a big issue, but if it is simple I'd prefer to have the behavior match for both.

[TimWolla]

We could apply the same behavior when opcache is disabled.

@iluuu1994 From the initial PR description. And I agree that unifying the behavior makes sense. The issue only existed in the first place, because the behavior was not consistent between OPcache and non-OPcache. To me any observable behavioral differences introduced by OPcache are a bug (unless they are the main purpose of OPcache, e.g. autoloading seeing outdates files when revalidation is disabled).

[iluuu1994]

Ah thank you, I missed this last sentence. 🙂 And Arnaud is fast at implementing than me commenting. 😄

To me any observable behavioral differences introduced by OPcache are a bug (unless they are the main purpose of OPcache, e.g. autoloading seeing outdates files when revalidation is disabled).

I think diverging behavior is ok if the engine benefits from it. As an example, cache slot merging is observable, but it's a worthwhile trade-off.