Hi Matt, Thanks for the comments.
> -----Original Message----- > From: Matt Wilmas [mailto:php_li...@realplain.com] > Sent: Tuesday, August 25, 2015 12:06 PM > To: Anatol Belski <anatol....@belski.net>; 'Dmitry Stogov' <dmi...@php.net>; > 'Xinchen Hui' <xinche...@zend.com>; 'Nikita Popov' <nikita....@gmail.com>; > 'Pierre Joye' <pierre....@gmail.com>; 'Bob Weinand' > <bobw...@hotmail.com>; 'Jakub Zelenka' <bu...@php.net> > Cc: internals@lists.php.net > Subject: Re: [PHP-DEV] Overflow checks and integral vars comparison > > Hi Anatol, Dmitry, all, > > ----- Original Message ----- > From: "Anatol Belski" > Sent: Friday, August 21, 2015 > > > Hi, > > > > Resending this as missed internals at the start. > > > > I was lately rethinking some part of the 64-bit RFC, and also seeing > > now Jakub's work on catching overflows in ext/openssl and Matt > > Williams suggestions on it (which was going a bit more global over > > it). So I came up with these macros with two goals > > Dang, people be gettin' my name wrong, in the same way, even "in writing." > ;-P > Ohh, no intention on that, I will save it in my head, Matt Wilmas :) > > - standardize the overflow checks > > - do actualy checks only on architectures where it's needed > > The compiler will already do that, as I've said of course, and more importantly > (for most cases), it will never get it wrong. > > > - simplify the checks where external libs (openssl, libxml, etc.) use > > firm datatypes like int > > > > #if SIZEOF_INT == SIZEOF_ZEND_LONG > > # define ZEND_LONG_INT_OVFL(zl) (0) > > # define ZEND_LONG_INT_UDFL(zl) (0) > > #else > > # define ZEND_LONG_INT_OVFL(zlong) ((zlong) > (zend_long)INT_MAX) # > > define > > ZEND_LONG_INT_UDFL(zlong) ((zlong) < (zend_long)INT_MIN) #endif > > Really no point in all that... > > > #define ZEND_SIZE_T_INT_OVFL(size) ((size) > (size_t)INT_MAX) > > What's wrong with just doing the check, "as is," instead of hiding it behind a > [longer] macro? (Also, none of the casts are necessary...?) > > > So having it like > > > > If (ZEND_LONG_INT_OVFL(x)) { > > return; > > } > > > > Compiler would eliminate the branch automatically on 32-bit and ILP64. > > Already will. :^) > > > Some other macros to do signed/unsigned comparison, these can be > extended. > > > > #define ZEND_SIZE_T_GT_ZEND_LONG(size, zlong) ((zlong) < 0 || (size) > > > (size_t)(zlong)) #define ZEND_SIZE_T_GTE_ZEND_LONG(size, zlong) > > ((zlong) < > > 0 > > || (size) >= (size_t)(zlong)) #define ZEND_SIZE_T_LT_ZEND_LONG(size, > > zlong) > > ((zlong) >= 0 && (size) < (size_t)(zlong)) #define > > ZEND_SIZE_T_LTE_ZEND_LONG(size, zlong) ((zlong) >= 0 && (size) <= > > (size_t)(zlong)) > > Yes, the only case where we might want to do something special is comparing > an unsigned type, like size_t (string lengths), with a signed max, since that's not > impossible to the compiler. Of course, you have to KNOW that's what you want > and/or it's safe to ignore the check, e.g. not dealing with strings > 2 GB on 32- > bit, etc. > > And again, a generic way to do that for a "max" of integer type or greater (since > literals are smallest type >= int that will hold the value), that should work > anywhere, is: > > #define ZEND_SIGNED_OVFL_etc(val, max) (sizeof(val) > sizeof(max) && (val) > > (max)) > > Simple and optimizes easily... > In general it's better to not to be optimistic about what compilers will do. Also it is probably a question of taste. SIZEOF_INT == SIZEOF_ZEND_LONG is something with zero calculation and guaranteed to exclude any possible side effect, while sizeof(int) == sizeof(zend_long) depends on the optimization choosen. Not disputing that your suggestion will do nearly the same job in the usual case, ofc. With the casts - probably yes, many cases will go by the standard type promotion rules, but having the casts only ensures it does what one actually intended to do and has no effect in the generated code. For example one would need a cast when comparing with INT_MAX + 1. Worse is with unsigned, because the result depends on the platform and concrete types and values. Hiding behind the macros makes full sense for two reasons at least - there are people with various skills that can just rely on what the API provides without going too deep into the detail - doing an internal change won't affect anything in the existing usages > > IMHO these and maybe more are missing after the 64-bit RFC. Do you > > think they would make sense? Or would make sense now, or later in master? > > Also, why, exactly, has the zend_string length been defined as size_t (just > because it's "supposed to" be?), which is incompatible with most everything in > PHP, instead of zend_long (since we had int before)? > > e.g. Just look at strlen() -- oops, size_t no worky! :-/ Dmitry? We need range > checking there, right? Conversion to double? No? Then there's *no > point* in using size_t, and would let the compiler auto-optimize the above- > referenced comparisons with size_t... > Not sure what you mean here, 32-bit? memory_limit is unsigned 32-bit there, but even if not - it is hardly possible to have a string exceeding it there anyway. Otherwise, there is a plenty of libraries working with size_t and it makes full sense to exhaust that on 64-bit. And the most of PHP7 is just fine with size_t. Before the 64-bit RFC my idea was actually to port the dependency libs for the full 64-bit support, but it's insane. So better to fix the side effects, as those are almost the same as in PHP5 on most of 64-bit. Regards Anatol -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
