Zend: Add zend_try_get_double - #23398
Conversation
|
that s a nice addition ! |
| ZEND_ASSERT(Z_TYPE(dst) == IS_DOUBLE); | ||
| return Z_DVAL(dst); | ||
| } | ||
| case IS_UNDEF: |
There was a problem hiding this comment.
We don't check this for zval_get_double, and I don't think we should do this here either. If you hand an UNDEF zval there are bigger issues at hand that need to be investigated.
There was a problem hiding this comment.
I manually add this case :| But this makes debug easier so sure.
There was a problem hiding this comment.
It seems the reason we do this in the other ones is for VM optimizations (which probably would be good to add a comment using the commit description as to why IS_UNDEF is checked there)
There was a problem hiding this comment.
I will also add a comment in code to make it clear.
There was a problem hiding this comment.
We shouldn't be reading an uninitialized typed property, that's a bug in the calling code.
There was a problem hiding this comment.
Okay I take some hour to learn this stuff. AFAIK it is reasonable to remove IS_UNDEF. I don't know anything about VM optimizations, but I can tell that this:
case IS_UNDEF:
*failed = true;
return 0.0;Is completely the wrong semantic. IS_UNDEF should not be considered as 0.0. This
a. makes debugging harder cuz this is hiding the bug and makes the code base even more unpredictable.
b. makes no sense because IS_UNDEF isn't any value and shouldn't be considered as 0 anyways.
Instead if we remove this logic, this falls into
default:
ZEND_UNREACHABLE();That immediately catch the bug. And is more nicer semantically because trying to turn it into a double value is indeed unreachable behavior.
I know we have *failed = true. But IMO this is more like indicating a error when turning the value. I think if you are trying to get a double from an UNDEF type, the problem is way more serious than that and errors need to be thrown here.
Also the bug you've mentioned make sense too. This isn't correct logic anyways.
| } | ||
| /* }}} */ | ||
|
|
||
| static zend_never_inline double ZEND_FASTCALL zval_try_get_double_func(const zval *op, bool *failed) /* {{{ */ |
There was a problem hiding this comment.
Why never inline? Given this is called only from zval_try_get_double(), which cannot be inlined in other compilation units, this should very much be inlined.
| *failed = false; | ||
| return Z_DVAL_P(op); | ||
| } | ||
| return zval_try_get_double_func(op, failed); |
There was a problem hiding this comment.
This pattern doesn't really make sense unless you move this function to the .h file so it can be inlined.
In that case, the zend_never_inline on zval_try_get_double_func() would make a bit more sense.
|
I'd also ask why this is tested in zend_test instead of on actual extensions. |
Ah. Because I have yet to write a actual bug fix that use this function. So I only write test in zend_test as a new feature. |
|
IMO we should not introduce functions that aren't made use of. If this is merged and branched into 8.6, we'll fix on an implementation we have no real proof is correct, and after that point changing it becomes a BC break for extensions. |
|
Okay I add an actual fix in GD to use this internal function. So we might need reviews from David. |
Add a failure-reporting double conversion API analogous to zval_try_get_long(), and use it to validate GD affine translate and scale options.
2f9013f to
cbf5e37
Compare
5 participants
See php/php-tasks#32. Given now we have corresponding
tryfunctions to almost everyzval_get_TYPEfunctions except double, it is reasonable to addzend_try_get_double. A quick search shows that there are 62 occasions ofzend_get_doublein the code base.A real-world bug example is in #23384 (comment)_
cc @Girgias
I almost copy-paste the implementation of
zend_try_get_longfor this function.