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

 	 * (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

 			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

 		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

 	   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

 			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

 			 */
 			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

 	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

 	// 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

	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

Uh oh!

Conversation

cmb69 commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

alexdowad commented Dec 7, 2024 via email

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!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cmb69 commented Dec 7, 2024 •

edited

Loading