[cscott]

The VM checks for interrupts after setting opline to the next op, such as a jump target (and, in the CALL VM, the op after a call to a user function); the JIT checks at loop headers and in side-exit stubs. If the interrupt function throws (async pcntl signal handler, Excimer timeout, ...), the exception is attributed to that op, which has not run. Live ranges end at the op that consumes a var, so ZEND_HANDLE_EXCEPTION does not free vars that this op was about to consume:

function gen() { yield 1; release(); } // interrupt pending as release() returns
foreach (gen() as $v) {}               // FE_FETCH jumps to FE_FREE: generator leaks

The same happens with switch on a string, and with ternary/match results assigned after the jump.

The interrupt helpers already undefine the result of the interrupted op. This PR moves that to zend_cleanup_interrupted_op(), which also frees vars whose live range ends at the interrupted op, and the operand defined by the op just before it (which has no live range). Interrupt timing is unchanged.

Tests use zend_test.observer.set_vm_interrupt_on_end (also added by the fix for GH-24480), which makes an interrupt pending and throwing as an observed function returns.

Tested on debug builds of the hybrid and CALL VMs, without opcache and with opcache with JIT off, function JIT and tracing JIT.

[devnexen]

Hi @cscott would it be possible to try this test case ?

--TEST--
VM interrupt handled while Generator::throw() resumes the generator
--EXTENSIONS--
zend_test
--INI--
zend_test.observer.enabled=1
zend_test.observer.show_output=0
zend_test.observer.observe_function_names=gen
zend_test.observer.set_vm_interrupt_on_begin=1
--FILE--
<?php
class Dtor {
    public function __destruct() {
        echo "destroyed\n";
    }
}

function gen() {
    try {
        yield new Dtor();
    } catch (Exception $e) {
        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
    }
    echo "resumed\n";
    yield 2;
}

$g = gen();
var_dump($g->current() instanceof Dtor);
var_dump($g->throw(new Exception('thrown')));
unset($g);
echo "done\n";
?>
--EXPECT--
bool(true)
Exception: thrown
resumed
destroyed
int(2)
done

[cscott]

There are test cases in the PR -- what do you want me to "try" exactly? That it fails, that's it's an improvement in some way on my test case? I'm not entirely sure what you're asking for.
I think I've figured out that you're saying that your test case should pass, but that with my patch it doesn't pass, indicating a problem with my patch. I'm working on it.

[cscott]

Ok, added a new patch on the branch which fixes @devnexen's test case (and adds it to Zend/tests). Can be squashed with the parent but I figured it would be better for review to keep them separate for now.

Generator::throw() resumes the generator at ZEND_HANDLE_EXCEPTION, with opline_before_exception pointing at the YIELD, and the executor checks for interrupts on entry. zend_cleanup_interrupted_op() decided whether the interrupt function had thrown by checking EX(opline)->opcode == ZEND_HANDLE_EXCEPTION, which is also true here, so it freed the new Dtor() temporary whose live range ends at the YIELD. But the YIELD had already moved it into the generator, so it was freed twice. (oops)

In the new commit the VM helper and the JIT stub now record whether an exception was pending before calling the interrupt function, and the new cleanup only runs if it wasn't.