Marking it ready for review for GitHub to send notifications to reviewers, who I need feedback from, even though it's not ready for merge.

If the EG(vm_interrupt) flag is set, the goal is for zend_interrupt_function to be called while the internal function call frame is still on the stack.

No. VM interrupts were designed to allow safe exit (or execution transfer) after some asynchronous event without repeatable checks after each VM instruction. Before the transfer, it's possible to capture the current execution state (save execut_data and opline) and then transfer execution back. This way we implement PHP callbacks for posix signal.

• The proposed additional check doesn't allow transfer back (into the middle of the ICALL handler). Signal callbacks won't work properly.
• Even without transfer back and force you'll miss arguments cleanup code
• Your patch starts to perform two checks for interrupt after each ICALL. This should make a slowdown.
• Behavior of DO_ICALL becomes inconsistent with internal functions/methods called through DO_FCALL

Observer callback already may be called through ZEND_OBSERVER_FCALL_END. Why don't you check interrupt there.

@dstogov Thank you for your feedback. However, I don't think you understood my point about zend_call_function, which does an interrupt check and calls the zend_interrupt_function handler and has done so for ~4 years.

If we have a function like:

ZEND_FUNCTION(example) {
    // ...
   zend_call_function(/* ... */);
    // ...
}

And the interrupt handler is called during this function, which is during a ZEND_DO_ICALL, and the vm interrupt handler switches away, then the rest of the function will never be called. That is, unless it does a fiber switch and backs up and restores C call stack and such as well. But if it can do that, then it's also not a problem for ZEND_DO_ICALL, right?

Your patch starts to perform two checks for interrupt after each ICALL. This should make a slowdown.

I don't think so. It removes the ZEND_VM_SET_OPCODE(opline + 1); and replaces it with:

	CHECK_SYMBOL_TABLES()
	OPLINE = opline + 1;

Where is the other interrupt check?

Behavior of DO_ICALL becomes inconsistent with internal functions/methods called through DO_FCALL

I would make the same changes to ZEND_DO_FCALL as well. I just wanted feedback on this fcall handler, as the other handlers would likely just mimic the changes here.

Observer callback already may be called through ZEND_OBSERVER_FCALL_END. Why don't you check interrupt there.

Yes, but installing an observer for every internal function fcall is expensive. My whole goal is to enable performance optimizations in the engine by avoiding zend_execute_internal, but still being able to see the leaf frame in an interrupt handler.

The proposed additional check doesn't allow transfer back (into the middle of the ICALL handler). Signal callbacks won't work properly.

In reality it's not "middle of ICALL handler" but end of function handler. The VM stack is in a safe state here. The signals now would simply be on top of the internal function, without any state being actually invalid.

Signal callbacks are not "capture the current execution state (save execut_data and opline) and then transfer execution back", but simply a frame pushed on top of the current VM stack. This wouldn't be safe anyway, unless you also back up the C stack, which fibers do, which includes the cleanup of the ICALL handler.

In fact, I also consider this sort-of helpful for stacktraces in signal handlers - these never include the internal function where the signal occurred, but only the line after. This would make it much more correct, especially for internal functions taking a bit of time.

@morrisonlevi you are right about the double check. (I was wrong).
@bwoebi you right about the posix signal handles. (I forgot the last implementation details).

Anyway, the actual call to zend_interrupt_function() is moved from the end into the middle of ICALL handler. and this is the problem.

zend_interrupt_function() is allowed to change EG(current_execute_data). This is why it's called though zend_interrupt_helper and followed by ZEND_VM_ENTER().

With this patch, the change of EG(current_execute_data) won't have proper effect.

@morrisonlevi I didn't understand what fiber related problem you are trying to solve.
If this is not just for observer, please demonstrate the problem with a complete test (PHPT+zend_test).

@dstogov Thanks for your reply. Consider code with this pattern:

ZEND_FUNCTION(/* ... */) {
    // ...
   zend_call_function(/* ... */);
    // ...
}

Remember that for ~4 years zend_call_function will check the interrupt flag and call zend_interrupt_function if it's set. There are a many places where this happens during internal functions, at the very least:

• Many array functions.
• PDOStatement::fetch
• Closure::call
• zend_closure_call_magic
• Exception::__toString
• mb_ereg_replace, mb_ereg_replace_callback, mb_eregi_replace
• preg_replace_callback
• Phar::webPhar (and more functions in Phar, I think)
• iterator_apply

If the zend_interrupt_function sets the EG(current_execute_data) during these zend_call_function calls, it's not going to work as expected because the opcode hasn't been moved to the next place yet (nor cleaned up args, etc). It's not the clean VM state you were expecting before 4 years ago when zend_call_function was changed.

So what I am saying, is that my patch doesn't change anything meaningful for extensions which want to jump to other places in the VM. They would need to do more work than just saving and swapping VM state, they also need to save C stack and registers, and that's already the case because of zend_call_function. Such extensions need to do something more like a "fiber switch" rather than a simpler VM jump.

I cannot write a test for you, because I don't have an extension which does what you are saying. But if you were to run it while calling such functions listed above, you should be able to see what I'm talking about. For instance, make a large array and do array_filter with a callback, and even better, make that callback an internal function too.

I think I started to understand you (sorry, this took a while).

I see, commit 555489d added interrupt check into zend_call_function().
This code doesn't look right to me, but you point to it, just as an argument, that the desired behavior is broken for a long time anyway.

Your PR doesn't fix anything, but just allows to set an "observing interrupt handler" that may see the interrupted internal function and its arguments. Right?

Your PR [...] just allows to set an "observing interrupt handler" that may see the interrupted internal function and its arguments. Right?

Yes, that's correct. This would be a nice improvement for my work. I will work on adding support to the other fcall handlers and adding an entry to UPGRADING.INTERNALS.

Your PR [...] just allows to set an "observing interrupt handler" that may see the interrupted internal function and its arguments. Right?

Yes, that's correct. This would be a nice improvement for my work. I will work on adding support to the other fcall handlers and adding an entry to UPGRADING.INTERNALS.

Do your profiler generates VM interrupts itself? (I mean these are not regular POSIX signal or timeout).

If this is the case, I would try not to use VM interrupts, but read the PHP backtrace info at first place.
On one hand this would require some protection from inconsistent leaf frame (I remember php-fpm made something similar), but on the other hand this may provide much more precise profiling info. This way doesn't work for you?

Accepting this PR (when complete) is not a big problem. It should be mentioned that interrupt handlers are not allowed to switch EG(current_execute_data), if the leaf frame corresponds to an internal function call.

Do your profiler generates VM interrupts itself? (I mean these are not regular POSIX signal or timeout).

We generate VM interrupts from another thread, and atomically set the VM interrupt flags of active PHP threads.

If this is the case, I would try not to use VM interrupts, but read the PHP backtrace info at first place. On one hand this would require some protection from inconsistent leaf frame (I remember php-fpm made something similar), but on the other hand this may provide much more precise profiling info. This way doesn't work for you?

There's not a way to do this safely from the other thread.

If we switch to a signal which is delivered to the PHP thread and walk during the signal handler (rather than a VM interrupt), we still have to deal with those consequences:

1. Signal handlers are extremely limited in what they can do. They cannot allocate memory or call many other useful things because all function calls in the signal handler must be async-signal-safe.
2. A signal will interrupt system calls. Even with SA_RESTART, some calls do not restart (eg sleep, select) which can affect user observable behavior.
3. A signal may interrupt at any point of the opcode handlers. It's really hard to keep the leaf frame in a safe state. This is partly why the interrupt handler mechanism was added in the first place, isn't it? Because timeouts and other interrupt handlers would periodically crash?

It seems best to only do this in VM interrupt handlers.

Accepting this PR (when complete) is not a big problem. It should be mentioned that interrupt handlers are not allowed to switch EG(current_execute_data), if the leaf frame corresponds to an internal function call.

I'll have you review the note, thank you.

We generate VM interrupts from another thread, and atomically set the VM interrupt flags of active PHP threads.

This way you may handle one event for a loop iteration or internal function call, but this is up to you...

A signal may interrupt at any point of the opcode handlers. It's really hard to keep the leaf frame in a safe state. This is partly why the interrupt handler mechanism was added in the first place, isn't it? Because timeouts and other interrupt handlers would periodically crash?

Previously, on POSIX we bailed out right from an async timeout signal and on Windows we set flag and checked it after VM instruction. VM interrupts unified implementation and minimized overhead of checks by doing them only at jumps and calls.

I accidentally removed some VM checks in my latest change in jump opcodes. I'll fix it soon.

As per @dstogov's feedback, the interrupt checks on returns are removed. This moves the interrupt checks for internal functions for DO_ICALL, DO_FCALL, and DO_FCALL_BY_NAME.

Interrupt checks have not been added for cases like zend_execute_ex being set, which had interrupts before. Dmitry, let me know if you think this is a problem. If it's not a problem, I think the patch is ready for final review (some tests are still running at this time). If it is a problem, let me know why so I can add them back. The opcode handlers are likely to get ugly if I do this.

Edit: I still need to update JIT. Forgot about that momentarily. But could you please review everything other than that?

The VM part of the PR looks fine for me now.
Now the VM changes should be reflected in JIT.
I think @bwoebi can do this.

@dstogov I updated the JIT with Bob's help. Does it look okay to you?

I'd like to merge this. I'm not sure how Dmitry was getting asm of the JIT, and I'm on aarch64 anyway, so I can't check that I've actually fixed the asm when no interrupt handler is set. Is this acceptable to everyone? Anyone want to check anything else?

@dstogov Thanks to Arnaud's help, the PR is improved. Could you please review this again?

Merging it after talking to Arnauld given that Dmitry stayed silent and RC1 is nearing.