Zend: Add `zend_try_get_double` by LamentXU123 · Pull Request #23398 · php/php-src

LamentXU123 · 2026-08-21T10:55:10Z

See php/php-tasks#32. Given now we have corresponding try functions to almost every zval_get_TYPE functions except double, it is reasonable to add zend_try_get_double. A quick search shows that there are 62 occasions of zend_get_double in the code base.

A real-world bug example is in #23384 (comment)_

cc @Girgias

I almost copy-paste the implementation of zend_try_get_long for this function.

devnexen · 2026-08-21T12:12:23Z

that s a nice addition !

Girgias · 2026-08-21T12:37:40Z

+				ZEND_ASSERT(Z_TYPE(dst) == IS_DOUBLE);
+				return Z_DVAL(dst);
+			}
+		case IS_UNDEF:


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.

I manually add this case :| But this makes debug easier so sure.

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)

I will also add a comment in code to make it clear.

We shouldn't be reading an uninitialized typed property, that's a bug in the calling code.

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.

iluuu1994 · 2026-09-04T09:28:27Z

 }
 /* }}} */

+static zend_never_inline double ZEND_FASTCALL zval_try_get_double_func(const zval *op, bool *failed) /* {{{ */


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.

iluuu1994 · 2026-09-04T09:45:20Z

+		*failed = false;
+		return Z_DVAL_P(op);
+	}
+	return zval_try_get_double_func(op, failed);


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.

Good point indeed :)

iluuu1994 · 2026-09-04T11:58:30Z

I'd also ask why this is tested in zend_test instead of on actual extensions.

LamentXU123 · 2026-09-04T12:09:22Z

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.

iluuu1994 · 2026-09-04T13:01:02Z

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.

LamentXU123 · 2026-09-04T13:37:46Z

Okay I add an actual fix in GD to use this internal function. So we might need reviews from David.

devnexen · 2026-09-04T17:24:11Z

@@ -0,0 +1,99 @@
+--TEST--


I am not convinced gd is necessarily the best experiment terrain for this. e.g. standard pack() seems a better candidate.

LamentXU123 · 2026-09-04T17:43:03Z

pack() now throws a TypeError when a value for the f, g, G, d, e, or E format code cannot be converted to float, instead of silently coercing it.

I guess this is a better way to use zend_try_get_double?

devnexen · 2026-09-04T18:40:27Z

seems better but @iluuu1994 is right please address his comment.

devnexen · 2026-09-04T18:41:16Z

@@ -0,0 +1,113 @@
+--TEST--


it surely deserves better coverage.

LamentXU123 · 2026-09-04T18:52:48Z

I've already Ilija's comment. Added more tests, I even test it's behavior in gmp :)

devnexen · 2026-09-04T18:54:56Z

The declaration is still never inlined (zend_operators.h).

devnexen

except the nit, looking good but please wait another approval.

iluuu1994 · 2026-09-04T18:57:04Z

I would agree to remove that. It's meaningless because zval_try_get_double() (or zval_try_get_double_func()) isn't called from Zend/zend_operators.c and so can't be inlined, and after LTO/PGO the hint is gone anyway.

iluuu1994 · 2026-09-04T19:03:58Z

+		default: ZEND_UNREACHABLE();
+	}
+}
+/* }}} */


Please also avoid vim folding on new functions

devnexen · 2026-09-04T19:06:48Z

I may have approved a bit fast after all..

devnexen · 2026-09-04T19:07:11Z

+					double d;
+					float v;
+					if (!php_pack_try_get_double(&argv[currentarg], currentarg + 2, &d)) {
+						zend_string_release(output);


seems this cleanup is repetitive

devnexen · 2026-09-04T19:08:24Z

+
+	*result = zval_try_get_double(value, &failed);
+	if (UNEXPECTED(failed)) {
+		if (!EG(exception)) {


not needed I think

you may need to update php_pack too ?

LamentXU123 · 2026-09-04T19:19:18Z

Done. I also add more tests on error handling.

LamentXU123 · 2026-09-04T20:21:24Z

Okay. I am not familiar with the standard pack function and it is difficult for me to write bug fix for it. But I done it, so I am extremely not sure about this. This PR is now rather a bug fix to pack instead of barely adding a helper function to Zend.

devnexen · 2026-09-05T06:28:56Z

+						return 0.0;
+					}
+				}
+				return type == IS_LONG ? (double) lval : dval;


if (type == IS_DOUBLE) { return dval; } if (UNEXPECTED(lval == 0)) { return zend_strtod(Z_STRVAL_P(op), NULL); } return (double) lval;

Makes sense to preserve "-0" here ig.

devnexen · 2026-09-05T20:43:20Z

+						return 0.0;
+					}
+				}
+				if (type == IS_DOUBLE) {


I realise ... this ... from 1100 to 1106 should be above the trailing data check above wdyt ?

devnexen · 2026-09-05T20:43:37Z

  . Added zend_string_ends_with() and related variants.
  . Added trait support for internal classes.
  . Added do_php_cli().
+  . Added zval_try_get_double(), which converts a defined zval to a double and


in the end, it should probably be reviewed.

Girgias

Can the changes to pack be split out from the PR? I'd rather have the API available for 8.6+ then the pack changes.

LamentXU123 · 2026-09-06T12:27:51Z

Okay I split the changes to pack out to #23590

Girgias

This looks good to me :)

LamentXU123 · 2026-09-06T16:03:38Z

Thank you all! Very much appreciated.

LamentXU123 requested a review from kocsismate as a code owner August 21, 2026 10:55

github-actions Bot added Category: Engine Extension: zend_test ABI break labels Aug 21, 2026

LamentXU123 marked this pull request as draft August 21, 2026 11:31

LamentXU123 marked this pull request as ready for review August 21, 2026 12:15

LamentXU123 requested a review from Girgias August 21, 2026 12:22

Girgias reviewed Aug 21, 2026

View reviewed changes

Comment thread Zend/zend_operators.c Outdated

NickSdot reviewed Aug 21, 2026

View reviewed changes

Comment thread ext/zend_test/tests/zval_try_get_double.phpt Outdated

Comment thread ext/zend_test/tests/zval_try_get_double.phpt Outdated

LamentXU123 requested review from Girgias and arnaud-lb August 21, 2026 18:23

iluuu1994 reviewed Sep 4, 2026

View reviewed changes

LamentXU123 requested a review from devnexen as a code owner September 4, 2026 13:35

github-actions Bot added the Extension: gd label Sep 4, 2026

LamentXU123 force-pushed the trygetdouble branch from 2f9013f to cbf5e37 Compare September 4, 2026 13:42

devnexen reviewed Sep 4, 2026

View reviewed changes

LamentXU123 force-pushed the trygetdouble branch 2 times, most recently from 0d5e68e to c2ac548 Compare September 4, 2026 17:41

LamentXU123 requested a review from bukka as a code owner September 4, 2026 17:41

github-actions Bot added the Extension: standard label Sep 4, 2026

devnexen requested changes Sep 4, 2026

View reviewed changes

github-actions Bot added the Extension: gmp label Sep 4, 2026

devnexen approved these changes Sep 4, 2026

View reviewed changes

iluuu1994 reviewed Sep 4, 2026

View reviewed changes

devnexen reviewed Sep 4, 2026

View reviewed changes

devnexen reviewed Sep 5, 2026

View reviewed changes

LamentXU123 removed Extension: gd Extension: gmp Extension: zend_test labels Sep 6, 2026

github-actions Bot added the Extension: gmp label Sep 6, 2026

Girgias reviewed Sep 6, 2026

View reviewed changes

Zend: Add zval_try_get_double()

278a069

LamentXU123 force-pushed the trygetdouble branch from 552b1ee to 278a069 Compare September 6, 2026 12:24

LamentXU123 mentioned this pull request Sep 6, 2026

ext/standard: Validate numeric values passed to pack() #23590

Open

Girgias approved these changes Sep 6, 2026

View reviewed changes

devnexen approved these changes Sep 6, 2026

View reviewed changes

LamentXU123 merged commit 21034a8 into php:master Sep 6, 2026
18 checks passed

LamentXU123 deleted the trygetdouble branch September 6, 2026 16:03

Uh oh!

Conversation

LamentXU123 commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devnexen commented Aug 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Sep 4, 2026

Uh oh!

LamentXU123 commented Sep 4, 2026

Uh oh!

iluuu1994 commented Sep 4, 2026

Uh oh!

LamentXU123 commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 commented Sep 4, 2026

Uh oh!

devnexen commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 commented Sep 4, 2026

Uh oh!

devnexen commented Sep 4, 2026

Uh oh!

devnexen left a comment

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen commented Sep 4, 2026

Uh oh!

devnexen Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 commented Sep 4, 2026

Uh oh!

LamentXU123 commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

LamentXU123 commented Aug 21, 2026 •

edited

Loading

LamentXU123 Aug 21, 2026 •

edited

Loading

LamentXU123 commented Sep 4, 2026 •

edited

Loading

iluuu1994 commented Sep 4, 2026 •

edited

Loading

devnexen Sep 4, 2026 •

edited

Loading