Skip to content

Introduce zend_vm_opcode_handler_t / zend_vm_opcode_handler_func_t #19006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Jul 2, 2025

This reduces the chances of confusion between opcode handlers used by the VM, and opcode handler functions used for tracing or debugging. Depending on the VM, zend_vm_opcode_handler_t may not be a function. For instance in the HYBRID VM this is a label pointer.

Extracted from #18720.

This reduces confusion between opcode handlers used by the VM, and opcode
handler functions used for tracing. Depending on the VM,
zend_vm_opcode_handler_t may not be a real function. For instance in the HYBRID
VM this is a label pointer.
@arnaud-lb arnaud-lb force-pushed the opcode-handler-type branch from bdaf126 to ed49acd Compare July 2, 2025 08:36
@arnaud-lb arnaud-lb marked this pull request as ready for review July 2, 2025 13:47
@arnaud-lb arnaud-lb requested a review from dstogov as a code owner July 2, 2025 13:47
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch seems simple, but I don't see all the possible consequences.
I don't object against it, but please check the comments.

Comment on lines +2372 to +2374
$str .= "#else\n";
$str .= "typedef const void* zend_vm_opcode_handler_t;\n";
$str .= "typedef const void* zend_vm_opcode_handler_func_t;\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose #else means GOTO and SWITCH VM.
Most probably GOTO uses only zend_vm_opcode_handler_t and SWITCH doesn't use both (or zend_vm_opcode_handler_t is just int).

Comment on lines +2367 to +2368
$str .= "typedef const void* zend_vm_opcode_handler_t;\n";
$str .= "typedef void (ZEND_FASTCALL *zend_vm_opcode_handler_func_t)(void);\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments that explain the conceptual difference for all VM types would be helpful.

@@ -298,6 +298,7 @@ void ZEND_FASTCALL zend_jit_undefined_string_key(EXECUTE_DATA_D)
ZVAL_NULL(result);
}

#if ZEND_VM_KIND != ZEND_VM_KIND_HYBRID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously opcache and JIT didn't have compile-time dependencies on VM type. The same opcache build was able to work with HYBRID and CALL VM.

It's not necessary to keep this, but if we decide to remove this possibility we also may remove zend_jit_vm_kind run-time checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants