Skip to content

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

Closed
adsr wants to merge 1 commit into
php:masterfrom
adsr:gh9090_ffi_func_ptr
Closed

Fix GH-9090: Support assigning function pointers in FFI#9107
adsr wants to merge 1 commit into
php:masterfrom
adsr:gh9090_ffi_func_ptr

Conversation

@adsr

@adsr adsr commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@adsr adsr mentioned this pull request Jul 22, 2022
Closed
@devnexen

devnexen commented Jul 23, 2022

Copy link
Copy Markdown
Member

Looks fine by me but cc @dstogov

@cmb69

cmb69 commented Jul 23, 2022

Copy link
Copy Markdown
Member

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".

iluuu1994
iluuu1994 reviewed Jul 23, 2022
View reviewed changes
Comment thread ext/ffi/ffi.c Outdated

@iluuu1994 iluuu1994 Jul 23, 2022

Copy link
Copy Markdown
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.

@adsr adsr Jul 25, 2022

Copy link
Copy Markdown
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.

@dstogov dstogov Jul 26, 2022

Copy link
Copy Markdown
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 Jul 27, 2022

Copy link
Copy Markdown
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
dstogov
dstogov requested changes Jul 26, 2022
View reviewed changes

@dstogov dstogov left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ext/ffi/ffi.c Outdated

@dstogov dstogov Jul 26, 2022

Copy link
Copy Markdown
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

cmb69 commented Jul 27, 2022

Copy link
Copy Markdown
Member

Thank you!

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

Reviewers

@iluuu1994 iluuu1994 iluuu1994 left review comments
@dstogov dstogov dstogov approved these changes

Assignees

No one assigned

Labels

Extension: ffi Extension: zend_test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adsr @devnexen @cmb69 @iluuu1994 @dstogov