[BUG] Property hooks + enabled JIT causes unexpected results (segfault for JIT function)

[mrVrAlex]

Description

Hello, I think I found some bugs here.

• take official docker image
• install opcache
• set opcache.ini config as:

[opcache]
opcache.enable=1
opcache.enable_cli=1

opcache.jit_buffer_size=100M
opcache.jit=1205

The following code:

<?php

declare(strict_types=1);

interface EntityManagerInterface {
    public function persist(object $entity);
}
class EntityManager implements EntityManagerInterface {

    public function persist(object $entity)
    {
        echo "Persisting entity: " . get_class($entity) . "\n";
    }
}
abstract class AbstractDecorator implements EntityManagerInterface {
    protected $wrapped;
    public function persist(object $entity)
    {
        $this->wrapped->persist($entity);
    }
}

class EntityManagerDecorator extends AbstractDecorator {
    protected object $proxy;
    protected $wrapped {
        get {
            if (!isset($this->proxy)) {
                $this->proxy = ($this->factory)();
            }
            return $this->proxy;
        }
    }

    public function __construct(private Closure $factory)
    {
    }
}

$em = new EntityManagerDecorator(static fn() => new EntityManager());
$a = new stdClass();
$em->persist($a);
$a1 = new stdClass();
$em->persist($a1);

Case 1: SEGMENTATION FAULT

If run this code, then it will be crash on
$em->persist($a1);

Case 2: Fatal error (unexpected behaviour)
If remove in this code interface EntityManagerInterface and implements statements then it will be give fatal error:

Fatal error: Uncaught Error: Call to a member function persist() on null in /app/test.php:20
Stack trace:
#0 /app/test.php(42): AbstractDecorator->persist(Object(stdClass))
#1 {main}
  thrown in /app/test.php on line 20

If disable opcache or use tracing mode it will be work as expected:

Persisting entity: stdClass
Persisting entity: stdClass

P.S. I tried test PHP 8.4.3RC1 - bugs also exists there.

PHP Version

PHP 8.4.2

Operating System

Alpine 3.21

[nielsdos]

Simplified:

<?php

abstract class AbstractDecorator {
    protected $wrapped;
    public function persist()
    {
        var_dump($this->wrapped);
    }
}

class EntityManagerDecorator extends AbstractDecorator {
    protected $wrapped {
        get {
            return $this->proxy;
        }
    }

    public function __construct(private int $proxy)
    {
    }
}

$em = new EntityManagerDecorator(1234);
$em->persist();

[nielsdos]

Probably, zend_get_known_property_info is wrong in zend_jit.c.

[nielsdos]

We're returning property info for the property but not taking into account that the subclasses may make the property a hook. This causes the property to be read from the property table instead of executing the hook, giving the wrong result.
We already return NULL in zend_get_known_property_info because function JIT doesn't support hooks.
It's not possible I think to extend the zend_get_known_property_info to also return NULL when the property may be a hook in the subclass because we don't know what classes may get loaded at runtime.
Ideally this gets fixed by adding real property hook support in the function JIT. This doesn't seem super easy.

[mrVrAlex]

It seems with tracing JIT opcache.jit=on also have bug:
Just wrap code to the cycle:

foreach (range(1, 100) as $i) {
    call_user_func($fn);
}

I think after some iterations will trigger JIT tracing and hook for $wrapped will be return null

[nielsdos]

That makes sense, conceptually it's still the same issue I described earlier: there is no real hook codegen support for the JIT. We detect hooks, but only on the parent classes, not the child classes, and then take the slow path. So when a child class uses a hook and the parent class doesn't, the code generated for the parent class mistakingly assumes there is no hook and that breaks things.

[iluuu1994]

This is indeed unfortunate. I don't see a way to fix this without introducing hook support, but hook support in the JIT is also questionable if hooks are rare, and we generate lots of code that will never run... Optimally, PHP would switch to opt-in inheritance, i.e. final by default, open when needed. But that's wishful thinking.

class A {
    public $prop = 1;
}

class B extends A {
    public $prop {
        get => 2;
    }
}

function test(A $a) {
    var_dump($a->prop);
}

test(new A);
test(new B);

Slightly simpler example.

[mrVrAlex]

hooks are rare

It's only now, but what will be after some years?) Even if disallow extending property with hooks then more people will be use hooks directly in code and someday without JIT it will bottleneck in performance... Should put some warning to documentation about do not use CPU bound calculation in property hooks)

In my case I just decide try hooks for abstract ready decorator in Doctrine https://github.com/doctrine/orm/blob/3.3.x/src/Decorator/EntityManagerDecorator.php (instead of full implement from scratch) In big projects using EM - common case and JIT give boost here...)

[iluuu1994]

I added the "obvious" fix here, I'm not sure if there are other places where similar assumptions are made. Let's wait until Dmitry is back from vacation next week.

[nielsdos]

I added the "obvious" fix here, I'm not sure if there are other places where similar assumptions are made. Let's wait until Dmitry is back from vacation next week.

That's very conservative right, even if you don't have a hook the check can trigger on overridden properties and take the slow path?

[iluuu1994]

@nielsdos It's conservative, but not very (imo). Overriding properties is something that rarely has uses. Excluding hooks, they are:

• Changing the visibility
• Changing the default value
• Adding attributes

I'm not sure if the additional load would be offset by the additional properties dodging the slow path.

[nielsdos]

Okay right. I was asking because this crossed my mind at one point too but thought it was perhaps too much.

[iluuu1994]

Yeah, tbf this is all based on assumptions, I did not do any measurements yet.

[iluuu1994]

Thanks for the patience, we finally found a reasonable solution to this issue.