Re: [RFC] 64 bit platform improvements for string length and integer

Hi Jakub, On Sat, January 11, 2014 17:08, Jakub Zelenka wrote: > On Sat, Jan 11, 2014 at 3:29 PM, Anatol Belski <ab@php.net> wrote: > > >> >> yep, that's exactly how it performs till now in the mainstream, just >> replace ZEND_INT_MAX with LONG_MAX. Adding a warning to every such case >> would cause a warning flood in many PHP apps, guaranteed :) Whereby it >> might be ok for debug mode maybe, I wouldn't do it as I can't remember >> any WTFs about the behavior. >> >> >> > I meant adding warning only for overflow cases. Values in existing apps > can't be bigger than LONG_MAX so I don't see how it could cause warning > flood in the current PHP apps. I don't even think that users will be > often using such a big values after the 64bit changes. > > The thing is that if I use value bigger than LONG_MAX after 64bit changes > and I pass it to the function defined in extension that does not support > it (use "l" in zpp), then I rather see warning than unexpected rounding to > LONG_MAX... There is no way how to find out (except looking to the ext > sources) that the big values are not supported. If I get warning, I can > fix it in the code straight away... > But isn't it the exact situation now on ILP32 vs LP64 systems? Like 32 vs 64 bit Linux? I haven't suggested 'l' to handle the param the old way, just it to be an alias for 'i'. In the new code that would be as dynamic as php_int_t is, however 'l' as alias would retain zpp compatibility to the older PHP. For instance how much sense had such a centralized warding for the snippet is_int(PHP_INT_MAX+1) ? There is a lot of functions accepting an arbitrary numeric value and not depending on any library, why they should throw a warning by default? Why should we throw a warning about 32 bit in the 64 bit build? Furtehrmore, LONG_MAX is 64 bit on LP64 systems, that would be the same as PHP_INT_MAX. > The another thing is that this checking needs to be done by other > libraries wrappers after getting parameters because the most libraries > using smaller types. That was exactly what I did when I worked on openssl > ext for 64bit and I will need to do it in my fann and crypto extension. > There are bunch > of other extensions that will need to do it too (for example imagick). The > difference with openssl changes is that the BC will need to be kept. I > am sure that if warnings were already in zend_parse_arg_impl, then the > work for supporting 64bit branch would much easier for me and other PECL > extension maintainers... > Yes, that's the case - the range checks have to be done locally according to what the wrapped library needs. If you haven't done them till now, you might have an issue on 64 bit linux anyway. Like say the lib uses int, but 64 bit long is passed. If that checks are already in place in your ext, then it's fine. The range checks you've done for openssl are the obligatory part, that's what I was doing as well for all the other extensions. Some are still pending as on the progress wiki page. And of course this is to mention in the porting guide. The libraries are different, for instance iconv uses size_t for string length, libxml uses int for string length. zpp cannot know, which library an extension uses. On the other hand - an extension without any library deps but with some math functions should be kept in mind, too. Regards anatol