Skip to content

Fix GH-9090: Support assigning function pointers in FFI #9107

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

Closed
wants to merge 1 commit into from

Conversation

adsr
Copy link
Contributor

@adsr adsr commented Jul 22, 2022

No description provided.

@devnexen
Copy link
Member

Looks fine by me but cc @dstogov

@cmb69
Copy link
Member

cmb69 commented Jul 23, 2022

Thank you! On a quick glimpse, this looks good to me as well, but we should also check the calling convention compatibility (zend_ffi_type.func.abi), and maybe some other "details".

}

/* Ensure same arg count */
dst_argc = dst_type->func.args ? zend_hash_num_elements(dst_type->func.args) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a check for C varags? Either way, a test for that would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added a variadic to the test.

Copy link
Member

Choose a reason for hiding this comment

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

You should check

(src_type->attr & ZEND_FFI_ATTR_VARIADIC) == (dst_type->attr & ZEND_FFI_ATTR_VARIADIC)

Also it's make sense to add check for calling convention

src_type->func.abi == dst_type->func.abi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added those checks. Thank you.

@adsr adsr force-pushed the gh9090_ffi_func_ptr branch from 105ce4b to 44c3558 Compare July 25, 2022 22:56
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.

Everything else except abi and variadic looks fine.

}

/* Ensure same arg count */
dst_argc = dst_type->func.args ? zend_hash_num_elements(dst_type->func.args) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

You should check

(src_type->attr & ZEND_FFI_ATTR_VARIADIC) == (dst_type->attr & ZEND_FFI_ATTR_VARIADIC)

Also it's make sense to add check for calling convention

src_type->func.abi == dst_type->func.abi

@adsr adsr force-pushed the gh9090_ffi_func_ptr branch from 44c3558 to 3f70649 Compare July 27, 2022 00:33
@cmb69 cmb69 closed this in 8cf9c2f Jul 27, 2022
@cmb69
Copy link
Member

cmb69 commented Jul 27, 2022

Thank you!

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.

5 participants