Skip to content
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

Allow loading FFI bindings through ffi.preload directive #4842

Closed
wants to merge 5 commits into from

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Oct 22, 2019

No description provided.

@remicollet
Copy link
Member

@dstogov thanks for your work

Can you please clarify how to use it ?
ffi.preload is for the headers, right ?
And about the class which use it ?

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

$ php -dffi.preload=ext/ffi/tests/300.h -r 'FFI::scope("TEST_300")->printf("Hello World from %s!\n", "PHP");'

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

Just remove FFI::load() and add "ffi.preload" directive.

@remicollet
Copy link
Member

OK, indeed it works this way.

Could be nice to disallow opcache.preload... when FFI is used....
Going to be confusing for users

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

You probably mean FFI::load()?
It's not going to work if PHP started as "root" and uses "opcache.preload_user" to change credentials.
I'm going to make FFI::load() to fail (with corresponding error message) in this case.
@remicollet is this the same what you suggest.

@remicollet
Copy link
Member

remicollet commented Oct 22, 2019

Hmm... no trying to understand an d avoid segfault ;)

Looks like a simple call with a static property is enough to raise a segfault

# php -derror_reporting=-1 -dopcache.log_verbosity_level=4 -d zend_extension=opcache.so  -d opcache.enable=1 -d opcache.enable_cli=1  -d opcache.preload_user=remi -d opcache.preload=preload-foo.inc foo.php
Tue Oct 22 15:03:25 2019 (26901): Message Cached script '$PRELOAD$'
Tue Oct 22 15:03:25 2019 (26901): Message Cached script '/home/rpmbuild/SPECS/remirepo/tools/ffi-examples/preload-foo.inc'
Erreur de segmentation (core dumped)

So seems another issue (not FFI related)
preload-foo.inc is a simple

<?php
namespace Remi;

class Foo {
	static private $single = NULL;
}

So perhaps the reason for this segfault reported in #78713 (the static property is used as singleton)

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

@remicollet the crash should be fixed by previous commit. Could you please recheck.

@remicollet
Copy link
Member

remicollet commented Oct 22, 2019

@dstogov indeed, segfault with singleton is fixed.

@remicollet
Copy link
Member

remicollet commented Oct 22, 2019

Looks like it works as expected

  • FFI::load in the preloaded class constructor (singleton)
  • ffi.preload and FFI::scope in the preloaded class constructor

Without this PR, and only with the "Allocate" fix, I don't encounter any segfault, but "FFI::scope" fails (so FFI::load is always used)

So the warning is indeed needed.

Great work!

@cmb69
Copy link
Member

cmb69 commented Oct 22, 2019

Fix compilation on Windows:

 ext/opcache/ZendAccelerator.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c
index 54e6fdf6cf..704e1c38d9 100644
--- a/ext/opcache/ZendAccelerator.c
+++ b/ext/opcache/ZendAccelerator.c
@@ -4590,9 +4590,11 @@ static int accel_finish_startup(void)
 		sapi_module.ub_write = preload_ub_write;
 		sapi_module.flush = preload_flush;
 
+#ifndef ZEND_WIN32
 		if (in_child) {
 			CG(compiler_options) |= ZEND_COMPILE_PRELOAD_IN_CHILD;
 		}
+#endif
 		CG(compiler_options) |= ZEND_COMPILE_PRELOAD;
 		CG(compiler_options) |= ZEND_COMPILE_HANDLE_OP_ARRAY;
 		CG(compiler_options) |= ZEND_COMPILE_IGNORE_INTERNAL_CLASSES;

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

@cmb69 thanks

@cmb69
Copy link
Member

cmb69 commented Oct 22, 2019

@dstogov, you're welcome!

@dstogov
Copy link
Member Author

dstogov commented Oct 22, 2019

Merged into PHP-7.4 as 1417352

@dstogov dstogov closed this Oct 22, 2019
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.

3 participants