-
Notifications
You must be signed in to change notification settings - Fork 118
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
PHP7 Support #48
PHP7 Support #48
Conversation
…sing in a char/int to mutate
…hp_session functions also expect smart_string instead of char* + int
… zend_string instead of char* + len. IS_BOOL is not a distinct type anymore IS_TRUE/IS_FALSE instead
…ss of zval indirection
…e, and zend_string instead of char+int for zend_lookup_class
…nd msgpack_unserialize_raw to have one level less of dereference
…macro removal PHP 5 -> 7
…sgpack_unpacker_fetch_object and msgpack_base_fetch_object
…nd_parse_parameters, which returns a zend_string instead of char+int
…was being fired when zend_symtable_update succeeded instead of failed
…equals IS_INDIRECT
…val's are still used by non_dtor members
Hey @laruence, first off good news! We are at 85% with test fails, holy crap I never thought I would see the day.... https://gist.github.com/Sean-Der/cd94c0b6fb71a81e089b However, I have ran into another issue with translating. In PHP 5 you could call zend_symtable_update and zend_hash_index_update on an object, allowing you to create invalid properties. In PHP 7 when you use those functions on the HASH_OF(object) it will create strange state (object becomes immutable, you will get a segfault in other places) In PHP 7 converting these functions to the appropriate zend_object functions also breaks references. zend_std_write_property I am going to be taking a break from working on this extension till I know the answer to the issues I have talked about so far. The template related functions have memory leaks (that are caught by the PHP memory manager) thanks! |
@@ -492,61 +364,41 @@ static ZEND_METHOD(msgpack_unpacker, execute) | |||
{ | |||
char *str = NULL, *data; | |||
long str_len = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str_len should be size_t
@zxcvdavid Thank you so much for looking this PR over! Going through and fixing your comments now. I am not able to compile with -C89 , I am getting errors in Zend headers? What compiler/flags are recommended? Would you mind looking over my comments/questions above about the zend API. thanks! |
@Sean-Der about C89 issues, you can see : php/php-src@76a290d Variable definitions need at the top. thanks. |
…invalid objects (non-string and 0 length keys)
Hey, just FYI, I am going to work on this in this week , I am going to create a new PHP7 branch.. thanks |
@laruence that is awesome to hear! Is there anything I can do to make things easier on you? Also currently php-src has some headers with C++ style comments (can't compile anything with -ansi) I am going to open a PR soon to fix that. There was some discussion in the PHP 7 extension channel about it. |
@Sean-Der nothing now, you have done a great work :), thanks |
just for the record, I spent whole day on this. there is a problem that is, we should pop values on stack. thus in case of err, we could release these garbages on the stack to fix the memleaks, which should reduce the peak memory usage as well. however, it is not easy for the current implementation.. need more time.. my current work could be found at : https://github.com/msgpack/msgpack-php/tree/php7 |
@laruence Just looking at all the things you fixed up, I made SO many mistakes thank you for cleaning up after me :/ doesn't look like I saved you that much time in the end urgh, better luck next time hopefully! sorry I couldn't figure out a better way to keep the stack an array of zval* , I am excited to see what the proper thing to do was! thank you so much for putting so much time into this. |
nah, you did a great progress. just maybe because I am more familiar with PHP7 internal. :) |
@Sean-Der I fixed the memleaks, now I am going to close this PR.. thanks for all the thing you did, cheers! |
@laruence working with you has been the best experience I have had with Open Source in a very long time. I can't thank you enough for how supporting/nice you have been. You have been awesome, if I run into you a conference I owe you a soda/beer etc... I would love to continue helping with the extension! (Is that the most helpful thing I could be doing for PHP right now?) If so I will work on getting bin/str support in 5+7 and take a look at all existing bugs and triage them. Otherwise, just tell me where I can be most useful! I am currently porting pecl-yaml and in contact with the maintainer, and things are going MUCH faster. |
@Sean-Der great, I am going to invite you into this project :) thanks |
Hey @laruence !
I have finally gotten this to the point that I feel confident about the architecture.
There are still many test fails (pass rate is at around 30%) However, most of these test fails have to do with not properly handling references and other issues that can be fixed easily (I just don't understand the zend engine API well enough)
I am opening this branch so I can mark some questions for you, and start a light review.
Would it be possible to start running some CI against this using the current service?
I am removing all preprocessor stuff to handle different API versions, I am only supporting PHP 7.