php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #78270 Usage of __vectorcall convention with FFI
Submitted: 2019-07-10 13:37 UTC Modified: 2019-10-29 08:01 UTC
From: lisachenko dot it at gmail dot com Assigned: cmb (profile)
Status: Closed Package: *Extensibility Functions
PHP Version: 7.4.0alpha2 OS: Windows x64
Private report: No CVE-ID: None
 [2019-07-10 13:37 UTC] lisachenko dot it at gmail dot com
Description:
------------
I'm trying to import AST internal PHP functions with FFI, but for Windows platform this brings a lot of pain, because __vectorcall convention is used. It was introduced in https://github.com/php/php-src/commit/3b6cd5fe4ce81d3d219887339f3915140214708c

Unfortunately, libffi doesn't support vectorcall convention, thus symbol could not be imported properly with FFI.

Is it possible to remove such specific calling convention, used for ZEND_FASTCALL macros for 7.4.0 and just switch to traditional one: __fastcall for x32. For x64 platform 

Expected result:
----------------
All exported PHP functions don't use vectorcall convention anymore.

Actual result:
--------------
Some PHP functions are exported as vectorcall-functions (have double @@ at the end with number of additional bytes), for example, "zend_ast_evaluate@@24" and could not be imported with FFI under Windows platform.


Patches

override-zend-fastcall (last revision 2019-07-22 10:56 UTC by cmb@php.net)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2019-07-11 01:35 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2019-07-11 01:35 UTC] ab@php.net
Thanks for the report. The vectorcall calling convention for 64-bit is the analogue of fastcall on 32-bit. 64-bit supports a very few calling conventions, vectorcall is the most advanced one compared to both default 64-bit and to fastcall.

The latest libffi has been released quite a long ago, it might make sense to check whether the latest dev supports vectorcall. It would be also the preferable way, as vectorcall is per se a far better option than the default x64 calling convention. Performance improvements was the reason switching to it back then, and removing it just out of hand without a good QA would be IMO a no go. On a case-by-case base, one could check where such a switch wouldn't impact, but for sure not just a blanket removal.

Also not sure the note about x32, did you mean x86? Also, could you please put a piece of code one could start with? Perhaps a list of functions where the issue was met. 

Thanks.
 [2019-07-11 14:53 UTC] lisachenko dot it at gmail dot com
I've checked the source code of libffi. Zero lines about vectorcall, so unlikely that this convention is supported right now.

x32 was typo, there should be "x86" of course. You can start debug this with simple attempt to import zend_atoi function under Windows platform which is declared as following:

ZEND_API int ZEND_FASTCALL zend_atoi(const char *str, size_t str_len);

Minimal reproduced case:
<?php

$vectorcallBug = FFI::cdef("extern int zend_atoi(const char *str, size_t str_len);", 'php7.dll');
$vectorcallBug->zend_atoi(); // Good if we can see an exception about invalid argument number

But now we can get only this: Fatal error: Uncaught FFI\Exception: Failed resolving C function 'zend_atoi'
 [2019-07-21 04:22 UTC] php-bugs at lists dot php dot net
No feedback was provided. The bug is being suspended because
we assume that you are no longer experiencing the problem.
If this is not the case and you are able to provide the
information that was requested earlier, please do so and
change the status of the bug back to "Re-Opened". Thank you.
 [2019-07-21 08:05 UTC] requinix@php.net
-Status: No Feedback +Status: Open
 [2019-07-22 10:56 UTC] cmb@php.net
The following patch has been added/updated:

Patch Name: override-zend-fastcall
Revision:   1563792985
URL:        https://bugs.php.net/patch-display.php?bug=78270&patch=override-zend-fastcall&revision=1563792985
 [2019-07-22 10:56 UTC] cmb@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: cmb
 [2019-07-22 10:56 UTC] cmb@php.net
I don't see a good *general* solution here.  Assuming that
`#define ZEND_FASTCALL __vectorcall` yields a more performant
binary than `#define ZEND_FASTCALL`, we would have to sacrifice
performance for the supposedly rare case when someone wants to use
such functions via FFI.

A compromise might be to let the configuration override the
definition in zend_portability.h, e.g. something like the attached
patch, so it would be possible to do

  set CFLAGS=/D ZEND_FASTCALL= & configure …

Would that be sufficent for you, Alexander, or would you want the
official binaries to be built without __vectorcall?
 [2019-07-22 12:29 UTC] lisachenko dot it at gmail dot com
Unfortunately, recompiling PHP with custom ZEND_FASTCALL doesn't change anything. Nobody will do this for general purposes, so in my opinion, we shouldn't affect overall performance by dropping this call convention, as it is a step back.

However I have an interesting idea. What if we could solve this in another way, by defining all internal API functions as ZEND_FASTCALL and removing ZEND_API macros for them, thus they won't be exposed outside. And select some useful methods to work with internal structures with ZEND_API, but without declaring them as ZEND_FASTCALL convention.

I can see some conflicts in presence of both ZEND_API and ZEND_FASTCALL. Because in some places the macros ZENA_API is present and in another it is absent, like with ZEND_FASTCALL.
 [2019-07-22 13:45 UTC] cmb@php.net
-Status: Feedback +Status: Open
 [2019-07-25 10:42 UTC] cmb@php.net
Well, the best solution would be to support __vectorcall, since
there may be other libraries which use this calling convention.

I tried to assess the required effort for this, and found that
apparently no name mangling is yet supported by ext/ffi, so that
even __fastcall wouldn't work on x86.  This has to be resolved
anyway.
 [2019-10-14 21:19 UTC] cmb@php.net
There is now a dev version available[1], which adds minimal
__vectorcall support (should be sufficient for PHP's APIs). It
would be great if you could test it.

[1] <https://windows.php.net/downloads/snaps/ostc/ffi-vectorcall/>
 [2019-10-22 09:54 UTC] lisachenko dot it at gmail dot com
Current patch helps a lot, at least most of __vectorcall functions can be resolved now.

Surprisingly,
  extern zval __vectorcall *zend_hash_add_or_update(HashTable *ht, zend_string *key, zval *pData, uint32_t flag); 
could not be resolved, could you please check this?
 [2019-10-22 14:51 UTC] cmb@php.net
Failing to resolve zend_hash_add_or_update() has been tracked as
bug #78716; since that bug has been fixed, new builds are
available at
<https://windows.php.net/downloads/snaps/ostc/ffi-vectorcall/>.
 [2019-10-29 08:01 UTC] cmb@php.net
-Status: Assigned +Status: Closed -Package: Unknown/Other Function +Package: *Extensibility Functions
 [2019-10-29 08:01 UTC] cmb@php.net
Basic __vectorcall support is implemented as
<http://git.php.net/?p=php-src.git;a=commit;h=bedbecf56d94353a2bcebc835d14896fd95ce6d7>.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Dec 21 16:01:28 2024 UTC