Skip to content

Drop fastcall attribute on variadic function #2975

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 3 commits into from

Conversation

lwhsu
Copy link
Contributor

@lwhsu lwhsu commented Dec 13, 2017

Eliminate "fastcall calling convention ignored on variadic function"
warning when compiling on 32-bits system with clang

Eliminate "fastcall calling convention ignored on variadic function"
warning when compiling on 32-bits system with clang
@nikic
Copy link
Member

nikic commented Dec 13, 2017

I believe we should just drop the FASTCALL for this function entirely. Fastcall (as a callee-clean calling convention) just doesn't make sense for variadics. Other compilers are probably silently falling back to cdecl as well. E.g. GCC says "If the number of arguments is variable all arguments are pushed on the stack" in the fastcall documentation.

@lwhsu
Copy link
Contributor Author

lwhsu commented Dec 13, 2017

@nikic: That makes sense. I updated the patch. Do you want me to update the PR title and squash the commits?

@@ -154,7 +154,7 @@ ZEND_API void ZEND_FASTCALL zend_hash_graceful_destroy(HashTable *ht);
ZEND_API void ZEND_FASTCALL zend_hash_graceful_reverse_destroy(HashTable *ht);
ZEND_API void ZEND_FASTCALL zend_hash_apply(HashTable *ht, apply_func_t apply_func);
ZEND_API void ZEND_FASTCALL zend_hash_apply_with_argument(HashTable *ht, apply_func_arg_t apply_func, void *);
ZEND_API void ZEND_FASTCALL zend_hash_apply_with_arguments(HashTable *ht, apply_func_args_t apply_func, int, ...);
ZEND_API void zend_hash_apply_with_arguments(HashTable *ht, apply_func_args_t apply_func, int, ...);
Copy link
Member

Choose a reason for hiding this comment

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

This also has to be changed in the zend_hash.c file, which still has the fastcall declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@lwhsu lwhsu changed the title Only add fastcall attribute on variadic function when not using clang Drop fastcall attribute on variadic function Dec 14, 2017
@lwhsu
Copy link
Contributor Author

lwhsu commented Dec 15, 2017

@nikic
Copy link
Member

nikic commented Dec 15, 2017

Merged as 72ec23d, thanks!

@nikic nikic closed this Dec 15, 2017
@lwhsu lwhsu deleted the clang-fastcall-variadic branch January 5, 2018 06:48
cmb69 added a commit to cmb69/php-src that referenced this pull request Aug 13, 2024
This is unlikely to be properly supported by compilers anyway, see
<php#2975>.
cmb69 added a commit that referenced this pull request Aug 13, 2024
This is unlikely to be properly supported by compilers anyway, see
<#2975>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants