Re: 64 bit RFC, #2 and #3 yes

From: Date: Tue, 04 Feb 2014 15:53:53 +0000
Subject: Re: 64 bit RFC, #2 and #3 yes
References: 1 2 3  Groups: php.internals 
Request: Send a blank email to [email protected] to get a copy of this message 
 Anatol,

On  4.02.2014 16:39, Anatol Belski wrote:

Hi Andrey,

On Tue, February 4, 2014 14:19, Andrey Hristov wrote:

On  1.02.2014 14:30, Anatol Belski wrote:


Hi,


as the concerns on the BC breach by zpp and macros changes are huge,
we've invented the below to make the essential change only visible.

This branches have zpp and macros change reverted (like #2 and #3 had
resulted yes), only the necessary 64 improvement is in place.

PHP core, zpp and macros reverted to the state of mainstream
https://github.com/weltling/php-src/tree/str_size_and_int64_old_names


Diff of the branch with old names to master
http://belski.net/phpz/str_size_and_int64_old_names.diff



An extension ported and fully worky with 5.3/4/5 and str_size_and_int64
  branch, diff
https://github.com/weltling/phurple/compare/str_size_and_int64


The same ext how it looks now (not using the new zpp placeholders)
https://github.com/weltling/phurple



A sample extension generated with ext_skel from str_size_and_int64
branch with several usage examples, worky also wit h5.3/4/5
https://github.com/weltling/str_size_and_int64_example/blob/master/str_
size_and_int64.c#L47

The sample ext diff to the current mainstream base
https://github.com/weltling/str_size_and_int64_example/compare/str_size_
and_int64_revert


Were this an acceptable tradeoff for this RFC to make it, one could
still decide it in favor of 5.6. The worries and wishes are not
reasonless, which is clear. However I think to strike a compromise is
important to keep balance.

Regards


Anatol







I saw 2 problems with the patch, in mysqlnd. After 2 instances I stopped
reading but a parameter of type uint is being declared as php_int_t, a flag
variable.


I appreciate you could take a look at this. Could you point to the exact
place where you see an issue? With the replacement of uint there are
multiple reasons (and we was talking about it while being on the
discussion about the mysqli_poll), two main of those

- php_size_t should be used for string lengths
- even flags coming from userland may cause overflow in 64 bit mode

So uint to php_uint_t might be because of that, depending on what you're
talking about.

Thanks

Anatol

-typedef enum_func_status (*func_mysqlnd_conn_data__tx_commit_or_rollback)(MYSQLND_CONN_DATA * conn, const zend_bool commit, const unsigned int flags, const char * const name TSRMLS_DC);


+typedef enum_func_status (*func_mysqlnd_conn_data__tx_commit_or_rollback)(MYSQLND_CONN_DATA * conn, const zend_bool commit, const php_int_t flags, const char * const name TSRMLS_DC);

The API needs unsigned integer, not signed one. mysql, mysqli and pdo should take care when they get signed integers and pass them appropriately.

Here is more:

-	unsigned int	char_minlen;
-	unsigned int	char_maxlen;
+	php_size_t	char_minlen;
+	php_size_t	char_maxlen;

even unsigned int is too much for this data but still, going size_t just is completely unnecessary, as the value will be between 1 and 4. However, as you don't know the inner workings it is excuseable. Still my standpoint that we inflate memory usage where it is not necessary at all.


-			  Check bug #52891 : Wrong data inserted with mysqli/mysqlnd when using bind_param, value > LONG_MAX
+			  Check bug #52891 : Wrong data inserted with mysqli/mysqlnd when using bind_param, value > PHP_INT_MAX

Here this is a line in a comment. The bug has specific title and it hasn't changed, however the constant mentioned is changed. A bit annoying.

There might be more, I gave a glance look at it. I'm curious to see how much memory real scripts will use with the branch.

Best,
Andrey

Thread (7 messages)

« previous php.internals (#72202) next »