Fix GH-13119: Changed to convert float and double values into strings using `H` format #13125

SakiTakamachi · 2024-01-11T16:10:03Z

At first, I made the base branch master by mistake, so the label was...

…s using zend_gcvt

SakiTakamachi · 2024-01-14T02:33:08Z

I think there's probably a lot of conflict with master, so when merging this into master, it might be a good idea to prioritize master and create a separate PR for master later.

cc:
@nielsdos @Girgias

Girgias · 2024-01-14T11:49:45Z

ext/pdo_firebird/firebird_statement.c

-					ZVAL_STR(result, zend_strpprintf(0, "%F", *(float*)var->sqldata));
+					{
+						char f_buf[256];
+						zend_gcvt((double) get_float_from_sqldata(var->sqldata), 8, '.', 'E', f_buf);


Can't our custom %H format specifier with zend_spprintf_unchecked/[zend_strpprintf_unchecked](https://heap.space/s?defs=zend_strpprintf_unchecked&project=php-src) do the trick?

Thanks, that makes sense! I fixed it.

Girgias · 2024-01-14T11:49:50Z

ext/pdo_firebird/firebird_statement.c

-					ZVAL_STR(result, zend_strpprintf(0, "%F", get_double_from_sqldata(var->sqldata)));
+					{
+						char d_buf[256];
+						zend_gcvt(get_double_from_sqldata(var->sqldata), 16, '.', 'E', d_buf);


nielsdos · 2024-01-14T14:02:02Z

ext/pdo_firebird/firebird_statement.c

@@ -459,11 +464,11 @@ static int firebird_stmt_get_col(
 					break;
 				case SQL_FLOAT:
 					/* TODO: Why is this not returned as the native type? */
-					ZVAL_STR(result, zend_strpprintf(0, "%F", *(float*)var->sqldata));
+					ZVAL_STR(result, zend_strpprintf_unchecked(0, "%.*H", get_float_from_sqldata(var->sqldata), 8));


I believe you can use "%.8H" instead of "%.*H" (and get rid of the argument 8).

Yes, but in that case, it seemed to use a macro that converted a string to a number, so I thought it would be more efficient to pass a number as an argument from the beginning.

TBH, I don't know how much the efficiency will change. Personally, I think either way of writing is fine, but do you think there is much difference in terms of efficiency?

That's fair, then we can leave this as-is. I don't think the efficiency between the two differs noticeably though.

I don't really understand, but after correcting this, the test that was failing on Windows_ZTS passed.

nielsdos · 2024-01-14T14:02:06Z

ext/pdo_firebird/firebird_statement.c

 					break;
 				case SQL_DOUBLE:
 					/* TODO: Why is this not returned as the native type? */
-					ZVAL_STR(result, zend_strpprintf(0, "%F", get_double_from_sqldata(var->sqldata)));
+					ZVAL_STR(result, zend_strpprintf_unchecked(0, "%.*H", get_double_from_sqldata(var->sqldata), 16));


Ditto, but with 16 instead of 8.

nielsdos · 2024-01-14T14:25:46Z

ext/pdo_firebird/tests/gh13119.phpt

+  [1]=>
+  array(2) {
+    ["F_VAL"]=>
+    string(7) "1.0E-16"


I wonder whether the use of scientific notation breaks BC somehow though, although in arithmetic they might not as they're numeric strings.

We can do the calculations, so I think it's okay.

https://3v4l.org/DL0bk

SakiTakamachi · 2024-01-14T14:48:14Z

Hmm. Windows....

SakiTakamachi · 2024-01-14T15:35:08Z

Maybe it's due to ZTS rather than Windows?

nielsdos · 2024-01-14T17:42:42Z

Maybe it's due to ZTS rather than Windows?

I don't expect ZTS to make a difference here, there might be a subtle bug in the sprintf implementation from Zend, but I haven't investigated that yet.

SakiTakamachi · 2024-01-14T19:25:42Z

I haven't looked into it properly yet either.

In this case, there are at least two places with conditional compilation related to ZTS. I thought it might not be an issue with ZTS itself, but rather an impact from these conditional compilations. (I won't know until I look into it properly...)

php-src/main/spprintf.c

Line 181 in ad9ec26

    
           static void xbuf_format_converter(void *xbuf, bool is_char, const char *fmt, va_list ap) /* {{{ */

Girgias · 2024-01-15T09:44:51Z

ext/pdo_firebird/firebird_statement.c

 					break;
 				case SQL_DOUBLE:
 					/* TODO: Why is this not returned as the native type? */
-					ZVAL_STR(result, zend_strpprintf(0, "%F", get_double_from_sqldata(var->sqldata)));
+					ZVAL_STR(result, zend_strpprintf_unchecked(0, "%.16H", get_double_from_sqldata(var->sqldata)));


Not sure if we need the unchecked variant now that we only pass one parameter. As I thought, the reason for the unchecked is to be able to pass the precision without the compiler freaking out.

Thank you!
Do you think it's better to use zend_strpprintf?

If the compiler doesn't complain, yes :)

As you would expect, the compiler complained to me :)

Then the current PR is fine :)

Girgias

This looks fine to me if CI is happy and stable

* PHP-8.2: Fix GH-13119 (#13125)

* PHP-8.3: Fix GH-13119 (#13125)

SakiTakamachi marked this pull request as draft January 11, 2024 16:10

github-actions bot added Extension: pdo_firebird and removed Extension: mysqli Extension: mysqlnd Extension: oci8 Extension: odbc Extension: opcache Category: Optimizer labels Jan 11, 2024

SakiTakamachi force-pushed the fix/gh-13119 branch 2 times, most recently from e97a292 to ffdd21f Compare January 13, 2024 14:52

Fix GH13119: Changed to convert float and double values into string…

ecd9839

…s using zend_gcvt

SakiTakamachi force-pushed the fix/gh-13119 branch from ffdd21f to ecd9839 Compare January 13, 2024 15:30

SakiTakamachi marked this pull request as ready for review January 14, 2024 02:30

Girgias reviewed Jan 14, 2024

View reviewed changes

change to use zend_strpprintf_unchecked

8740151

nielsdos reviewed Jan 14, 2024

View reviewed changes

Specify numbers instead of wildcards

e60d7be

Girgias reviewed Jan 15, 2024

View reviewed changes

Girgias approved these changes Jan 15, 2024

View reviewed changes

SakiTakamachi changed the title ~~Fix GH-13119: Changed to convert float and double values into strings using zend_gcvt~~ Fix GH-13119: Changed to convert float and double values into strings using H format Jan 15, 2024

SakiTakamachi added a commit that referenced this pull request Jan 16, 2024

Merge branch 'PHP-8.2' into PHP-8.3

f234104

* PHP-8.2: Fix GH-13119 (#13125)

SakiTakamachi closed this in fa751c7 Jan 16, 2024

SakiTakamachi added a commit that referenced this pull request Jan 16, 2024

Merge branch 'PHP-8.3'

bdd7711

* PHP-8.3: Fix GH-13119 (#13125)

SakiTakamachi deleted the fix/gh-13119 branch March 15, 2024 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-13119: Changed to convert float and double values into strings using `H` format #13125

Fix GH-13119: Changed to convert float and double values into strings using `H` format #13125

Fix GH-13119: Changed to convert float and double values ​​into strings using H format #13125

Fix GH-13119: Changed to convert float and double values ​​into strings using H format #13125

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix GH-13119: Changed to convert float and double values into strings using `H` format #13125

Fix GH-13119: Changed to convert float and double values into strings using `H` format #13125