Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72202
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain hristov.com from 91.196.124.214 cause and error)
Message-ID: <52F10D11.1090703@hristov.com>
Date: Tue, 04 Feb 2014 17:53:53 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
MIME-Version: 1.0
To: Anatol Belski <anatol.php@belski.net>
CC: internals@lists.php.net
References: <e077612e6dd6e128cc0b8042db8d6b1c.squirrel@webmail.klapt.com>    <52F0E8CF.7090501@hristov.com> <0681aaa573a34b3e07e246e7f88efbab.squirrel@webmail.klapt.com>
In-Reply-To: <0681aaa573a34b3e07e246e7f88efbab.squirrel@webmail.klapt.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] 64 bit RFC, #2 and #3 yes
From: php@hristov.com (Andrey Hristov)

  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