Resolve MSVC C4244 level 2 warnings #17076

cmb69 · 2024-12-07T10:23:59Z

Level 2 C4244 warnings hint at possible loss of data. So far this PR silences them by explicitly casting, what actually may be appropriate in some cases (possibly for FFI); in some other cases the warnings might be bogus, in which case we may want to add suitable assertions. There may be some cases, though, which hint at edge case bugs (in which case these should better be fixed).

It would be great if we can properly solve this. Maybe some of the code owners can have a look?

Note that the change to the CFLAGS for CI is not supposed to be committed; it's just to see whether I've missed some of these warnings when testing locally.

PS: any changes to the bundled libgd should be coordinated with upstream.

ext/mbstring/mbstring.c

ext/opcache/zend_accelerator_module.c

cmb69 · 2024-12-07T10:33:25Z

ext/random/random.c

@@ -473,7 +473,7 @@ PHPAPI zend_long php_mt_rand_common(zend_long min, zend_long max)
 	/* This is an inlined version of the RAND_RANGE_BADSCALING macro that does not invoke UB when encountering
 	 * (max - min) > ZEND_LONG_MAX.
 	 */
-	zend_ulong offset = (double) ( (double) max - min + 1.0) * (r / (PHP_MT_RAND_MAX + 1.0));
+	zend_ulong offset = (zend_ulong) (((double) max - min + 1.0) * (r / (PHP_MT_RAND_MAX + 1.0)));


The old version appears to "over-cast", so I simplified. @TimWolla, @zeriyoshi, thoughts?

looks good!

cmb69 · 2024-12-07T10:34:32Z

ext/soap/php_encoding.c

@@ -1028,7 +1028,7 @@ static zval *to_zval_double(zval *ret, encodeTypePtr type, xmlNodePtr data)
 			whiteSpace_collapse(data->children->content);
 			switch (is_numeric_string((char*)data->children->content, strlen((char*)data->children->content), &lval, &dval, 0)) {
 				case IS_LONG:
-					ZVAL_DOUBLE(ret, lval);
+					ZVAL_DOUBLE(ret, (double) lval);


Is this cast fine, @nielsdos?

alexdowad · 2024-12-07T12:09:17Z

That sounds good. This calculation is a rough heuristic which helps find the known text encoding which best matches some given byte sequence. Which way we round fractional values really does not matter much. I support @cmb69's suggestion.

…

On Sat, Dec 7, 2024, 7:49 PM Christoph M. Becker ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ext/mbstring/mbstring.c <#17076 (comment)>: > @@ -3348,7 +3348,7 @@ try_next_encoding:; } for (size_t i = 0; i < length; i++) { - array[i].demerits *= array[i].multiplier; + array[i].demerits *= (uint64_t) array[i].multiplier; Well, to make the test <https://github.com/php/php-src/actions/runs/12212106167/job/34070184398?pr=17076#step:13:136> pass, probably ⬇️ Suggested change - array[i].demerits *= (uint64_t) array[i].multiplier; + array[i].demerits = (uint64_t) (array[i].demerits * array[i].multiplier); — Reply to this email directly, view it on GitHub <#17076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIESX3RO7I7IGEOOGDENJL2ELHB3AVCNFSM6AAAAABTGCW43OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBWGQ3TONZUGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cmb69 · 2024-12-07T12:16:23Z

ext/gd/libgd/gd_webp.c

@@ -156,7 +156,7 @@ void gdImageWebpCtx (gdImagePtr im, gdIOCtx * outfile, int quality)
 	if (quality >= gdWebpLossless) {
 		out_size = WebPEncodeLosslessRGBA(argb, gdImageSX(im), gdImageSY(im), gdImageSX(im) * 4, &out);
 	} else {
-		out_size = WebPEncodeRGBA(argb, gdImageSX(im), gdImageSY(im), gdImageSX(im) * 4, quality, &out);
+		out_size = WebPEncodeRGBA(argb, gdImageSX(im), gdImageSY(im), gdImageSX(im) * 4, (float) quality, &out);


The simple cast is fine here to suppress the warning. ext/gd catches values < -1 early, and for libgd, such values cause WebPEncodeRGBA() to fail.

cmb69 · 2024-12-07T12:39:48Z

ext/gd/libgd/gd_interpolation.c

@@ -2137,8 +2137,7 @@ gdImagePtr gdImageRotateInterpolated(const gdImagePtr src, const float angle, in
 	/* round to two decimals and keep the 100x multiplication to use it in the common square angles
 	   case later. Keep the two decimal precisions so smaller rotation steps can be done, useful for
 	   slow animations. */
-	const int angle_rounded = fmod((int) floorf(angle * 100), 360 * 100);
-
+	const int angle_rounded = (int) fmod((int) floorf(angle * 100), 360 * 100);


This looks seriously overengineered. Why are there even float operations, instead of

Suggested change

const int angle_rounded = (int) fmod((int) floorf(angle * 100), 360 * 100);

const int angle_rounded = ((int) (angle * 100)) % (360 * 100);

cmb69 · 2024-12-07T12:42:18Z

ext/gd/libgd/gd.c

@@ -1134,7 +1134,7 @@ void gdImageLine (gdImagePtr im, int x1, int y1, int x2, int y2, int color)
 TBB: but watch out for /0! */
 			double ac = cos (atan2 (dy, dx));
 			if (ac != 0) {
-				wid = thick / ac;
+				wid = (int) (thick / ac);


It seems to me that rounding would make sense here, but likely introduces a subtle BC break; as such casting to int is okay.

cmb69 · 2024-12-07T12:42:37Z

ext/gd/libgd/gd.c

@@ -2827,7 +2827,7 @@ void gdImageFilledPolygon (gdImagePtr im, gdPointPtr p, int n, int c)
 			 * same footprint.
 			 */
 			if (y >= y1 && y < y2) {
-				im->polyInts[ints++] = (float) ((y - y1) * (x2 - x1)) / (float) (y2 - y1) + 0.5 + x1;
+				im->polyInts[ints++] = (int) ((float) ((y - y1) * (x2 - x1)) / (float) (y2 - y1) + 0.5 + x1);


This one is okay.

cmb69 · 2024-12-07T12:50:54Z

ext/gd/libgd/gd_avif.c

@@ -81,7 +81,7 @@ static int quality2Quantizer(int quality) {

 	float scaleFactor = (float) AVIF_QUANTIZER_WORST_QUALITY / (float) MAX_QUALITY;

-	return round(scaleFactor * (MAX_QUALITY - clampedQuality));
+	return (int) round(scaleFactor * (MAX_QUALITY - clampedQuality));


I think the float calculation is unnecessary here. The following should be good enough:

Suggested change

return (int) round(scaleFactor * (MAX_QUALITY - clampedQuality));

return clampedQuality * AVIF_QUANTIZER_WORST_QUALITY / MAX_QUALITY;

But changing this now would introduce a subtle BC break, so just casting to int appears to be sensible for now.

cmb69 · 2024-12-07T13:11:38Z

ext/gd/libgd/gd_avif.c

@@ -103,7 +103,7 @@ static avifBool setEncoderTilesAndThreads(avifEncoder *encoder, avifRGBImage *rg

 	// The number of tiles in any dimension will always be a power of 2. We can only specify log(2)tiles.

-	tilesLog2 = floor(log2(tiles));
+	tilesLog2 = (int) floor(log2(tiles));


Why do we even floor() here instead of casting to int right away:

Suggested change

tilesLog2 = (int) floor(log2(tiles));

tilesLog2 = (int) log2(tiles);

And given we're interested in the log2 of an integer, there are even options not requiring an FP instruction (especially since tiles <= 8). But okay.

dstogov

FFI changes look good to me.

These got already approval by the respective code owners in GH-17076.

* master: Resolve some MSVC C4244 level 2 warnings

C4146[1] is about unary minus applied to unsigned operands; that behavior is well defined, and apparently used deliberately in the code base. C4244[2] is about possible loss of data when converting to another arithmetic type. This is addressed by another PR[3]. Anyhow, it seems like a no brainer to elevate to `/W2` even if we have to exempt two categories of warnings, since we can catch some others. [1] <https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4146?view=msvc-170> [2] <https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4244> [3] <php#17076>

C4146[1] is about unary minus applied to unsigned operands; that behavior is well defined, and apparently used deliberately in the code base. C4244[2] is about possible loss of data when converting to another arithmetic type. This is addressed by another PR[3]. Anyhow, it seems like a no brainer to elevate to `/W2` even if we have to exempt two categories of warnings, since we can catch some others. [1] <https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4146?view=msvc-170> [2] <https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4244> [3] <#17076>

Resolve MSVC C4244 level 2 warnings

0cee428

github-actions bot added Extension: intl Extension: ffi Extension: gd Extension: mbstring Extension: opcache Extension: soap Extension: random labels Dec 7, 2024

cmb69 commented Dec 7, 2024

View reviewed changes

dstogov reviewed Dec 9, 2024

View reviewed changes

cmb69 added 3 commits December 9, 2024 12:42

fix merge-conflict

8e54ece

fix mbstring

91c6818

Merge branch 'master' into cmb/c4244

e8204d0

cmb69 added a commit that referenced this pull request Dec 10, 2024

Resolve some MSVC C4244 level 2 warnings

6e759e0

These got already approval by the respective code owners in GH-17076.

Merge branch 'master' into cmb/c4244

981c8a0

* master: Resolve some MSVC C4244 level 2 warnings

cmb69 mentioned this pull request Jan 26, 2025

Elevate Windows CI to /W2 (sans C4146/C4244) #17581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve MSVC C4244 level 2 warnings #17076

Resolve MSVC C4244 level 2 warnings #17076

cmb69 commented Dec 7, 2024 •

edited

Loading

cmb69 Dec 7, 2024

zeriyoshi Dec 24, 2024

cmb69 Dec 7, 2024

nielsdos Dec 7, 2024

alexdowad commented Dec 7, 2024 via email

cmb69 Dec 7, 2024

cmb69 Dec 7, 2024

cmb69 Dec 7, 2024

cmb69 Dec 7, 2024

cmb69 Dec 7, 2024

cmb69 Dec 7, 2024

dstogov left a comment

	const int angle_rounded = (int) fmod((int) floorf(angle * 100), 360 * 100);
	const int angle_rounded = ((int) (angle * 100)) % (360 * 100);

	return (int) round(scaleFactor * (MAX_QUALITY - clampedQuality));
	return clampedQuality * AVIF_QUANTIZER_WORST_QUALITY / MAX_QUALITY;

	tilesLog2 = (int) floor(log2(tiles));
	tilesLog2 = (int) log2(tiles);

Resolve MSVC C4244 level 2 warnings #17076

Are you sure you want to change the base?

Resolve MSVC C4244 level 2 warnings #17076

Conversation

cmb69 commented Dec 7, 2024 • edited Loading

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

alexdowad commented Dec 7, 2024 via email

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

dstogov left a comment

Choose a reason for hiding this comment

cmb69 commented Dec 7, 2024 •

edited

Loading