Tailcall VM interrupt crash#21922
Conversation
| out($f," if (UNEXPECTED(((uintptr_t)opline & ZEND_VM_ENTER_BIT))) { \\\n"); | ||
| out($f," return opline; \\\n"); | ||
| out($f," } \\\n"); | ||
| out($f," ZEND_VM_TAIL_CALL(opline->handler(ZEND_OPCODE_HANDLER_ARGS_PASSTHRU)); \\\n"); |
There was a problem hiding this comment.
the alternative would be returning instead of tail-calling here. But that might be slower.
There was a problem hiding this comment.
Yes this would work. Possibly this may result in worse branch prediction as we revert to the main loop's central dispatch point.
Another alternative would be to make ZEND_VM_INTERRUPT() return an opline whose handler is zend_interrupt_helper, like we do for halt_op in the HYBRID and TAILCALL VMs: https://github.com/php/php-src/compare/arnaud-lb:vm-interrupt-tailcall-repro-3. This probably requires special handling in zend_jit_trace_execute(), I haven't investigated.
I've benchmarked the 3 approaches:
Results:
Symfony:
base: mean: 0.4459; stddev: 0.0004; diff: -0.00%
1 : mean: 0.4439; stddev: 0.0004; diff: -0.45%; p-value: 0.001000 (strong)
2 : mean: 0.4471; stddev: 0.0003; diff: +0.25%; p-value: 0.001000 (strong)
3 : mean: 0.4436; stddev: 0.0003; diff: -0.52%; p-value: 0.001000 (strong)
Symfony (valgrind):
base: diff: +0.00%
1 : diff: +0.12%
2 : diff: +0.01%
3 : diff: -0.00%
bench.php:
base: mean: 0.8044; stddev: 0.0004; diff: -0.00%
1 : mean: 0.8161; stddev: 0.0005; diff: +1.45%; p-value: 0.001000 (strong)
2 : mean: 0.8142; stddev: 0.0016; diff: +1.22%; p-value: 0.001000 (strong)
3 : mean: 0.8043; stddev: 0.0006; diff: -0.01%; p-value: 0.690382 (weak)
bench.php (valgrind):
base: diff: +0.00%
1 : diff: +1.03%
2 : diff: -0.50%
3 : diff: -0.00%
The Symfony results do not make sense to me, but these are stable.
Approach 3 seems better overall, speed-wise, assuming that we don't find issues with it.
There was a problem hiding this comment.
Uh, that's a nice approach! Love it.
I don't see any issues, that approach will never have &call_interrupt_op show up in EX(opline) or the VM IP register so that's perfect.
I have no idea about the JIT code, so, I'll leave that to you :-D
There was a problem hiding this comment.
I've pulled in your changes in the 3rd approach to let CI run while I review it in more dept (first glance looks good).
The call_interrupt_op sentinel approach only applies to TAILCALL VM,
where musttail constraints prevent checking ZEND_VM_ENTER_BIT in
ZEND_VM_DISPATCH_TO_HELPER. For CALL VM (non-global-reg), zend_interrupt
now forwards directly to zend_interrupt_helper_SPEC instead of returning
&call_interrupt_op.
Guard zend_interrupt (function and forward declaration) with:
#if !(defined(ZEND_VM_FP_GLOBAL_REG) && defined(ZEND_VM_IP_GLOBAL_REG))
&& ZEND_VM_KIND != ZEND_VM_KIND_TAILCALL
This fixes three failure modes:
- Global-reg builds (HYBRID/CALL with FP+IP regs): ZEND_OPCODE_HANDLER_RET
is void, making return &call_interrupt_op a constraint violation
- CALL non-global-reg builds (x32, Windows): call_interrupt_op was
undeclared for non-TAILCALL VM kinds
- TAILCALL builds: zend_interrupt (CALL variant) compiled but unused,
triggering -Werror=unused-function
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In TAILCALL delayed helpers (EX-signature helpers that can't musttail), ZEND_VM_INTERRUPT() was routing through zend_interrupt_helper_SPEC, which can return a tagged pointer (ZEND_VM_ENTER_BIT set) via ZEND_VM_ENTER(). That tagged pointer propagated back to ZEND_VM_DISPATCH_TO_HELPER, which blindly dispatched to it, causing a misaligned access crash. The fix: TAILCALL delayed helpers use SAVE_OPLINE() + return &call_interrupt_op, the sentinel pattern already used by TAILCALL VM. ZEND_VM_DISPATCH_TO_HELPER then dispatches to call_interrupt_op->handler (zend_interrupt_helper_SPEC_TAILCALL), which loads the real opline from EX(opline) and handles the interrupt cleanly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
In the new tailcall VM, if a call like
ZEND_VM_DISPATCH_TO_HELPER(...)uses a helper likezend_is_smaller_helper_SPEC_TAILCALL(), and during that helperEG(vm_interrupt)gets set, then it will return the current opline tagged withZEND_VM_ENTER_BIT. But theZEND_VM_DISPATCH_TO_HELPERmacro directly dereferenced the tagged pointer.This PR changes the macro to check for a tagged pointer, and returns instead of doing the tailcall as the outer executor will handle the
ZEND_VM_ENTER_BIT.To reproduce this, I piggy-backed on the vm_interrupt machinery in zend_test I just added in #21910. It uses a compare handler on
class VmInterruptComparableto deterministically set theEG(vm_interrupt).