Compiling php_random_pcgoneseq128xslrr64_rotr64() triggers C4146

[cmb69]

Description

I don't actually think there is a bug, but I'm looking for clarification. The issue is with the implementation for platforms where __uint128_t is not available (but I presume that it would be the same for the alternative implementation):

php-src/ext/random/php_random_uint128.h

Lines 78 to 85 in da72ac1

	static inline uint64_t php_random_pcgoneseq128xslrr64_rotr64(php_random_uint128_t num)
	{
	const uint64_t
	v = (num.hi ^ num.lo),
	s = num.hi >> 58U;
	return (v >> s) | (v << ((-s) & 63));
	}

The -s triggers C4146, which is a level 2 diagnostic, but is elevated to an error if the /sdl compile directive is active. The issue came up when I've tried to build sqlsrv and pdo_sqlsrv, and I found microsoft/msphpsql#1496, where it has been suggested to deactivate /sdl (which, in my opinion, should be done for production builds anyway).

Now, if I'm not mistaken, applying unary minus to an unsigned operand is well-defined behavior (at least in C99), so either I'm mistaken, C++ behaves differently, or the warning is bogus.

Maybe @zeriyoshi or @TimWolla can clarify.

PHP Version

PHP 8.3

Operating System

Windows

[TimWolla]

The reference implementation is here:

https://github.com/imneme/pcg-c/blob/83252d9c23df9c82ecb42210afed61a7b42402d7/include/pcg_variants.h#L96-L106

https://github.com/imneme/pcg-c/blob/83252d9c23df9c82ecb42210afed61a7b42402d7/include/pcg_variants.h#L243-L247

The code as implemented in PHP matches the signedness of the reference implementation.

[TimWolla]

This could just be rewritten to

return (v >> s) | (v << (64 - s)); 

But I would be somewhat unhappy to diverge from the reference implementation just to satisfy a compiler warning about perfectly legal code.

Now, if I'm not mistaken, applying unary minus to an unsigned operand is well-defined behavior (at least in C99), so either I'm mistaken, C++ behaves differently, or the warning is bogus.

My understanding is that the warning is a warning about possibly unexpected behavior for less-experienced C developers, just like clang’s -Wstring-plus-int:

#include<stdio.h>

int
main(void) {
	printf("%s\n", "foo" + 1);
}
$ clang test.c
test.c:5:23: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
        printf("%s\n", "foo" + 1);
                       ~~~~~~^~~
test.c:5:23: note: use array indexing to silence this warning
        printf("%s\n", "foo" + 1);
                             ^
                       &     [  ]
1 warning generated.

[TimWolla]

The following (untested) patch might leave the code in a cleaner state than before, though, because php_random_pcgoneseq128xslrr64_rotr64 is not technically specific to u128 anyways:

diff --git i/ext/random/engine_pcgoneseq128xslrr64.c w/ext/random/engine_pcgoneseq128xslrr64.c
index 9ff4f91192..5c6b311577 100644
--- i/ext/random/engine_pcgoneseq128xslrr64.c
+++ w/ext/random/engine_pcgoneseq128xslrr64.c
@@ -43,6 +43,11 @@ PHPAPI inline void php_random_pcgoneseq128xslrr64_seed128(php_random_status_stat
 	step(s);
 }
 
+ZEND_ATTRIBUTE_CONST static inline uint64_t rotr(const uint64_t x, int k)
+{
+	return (x << k) | (x >> (64 - k));
+}
+
 static php_random_result generate(void *state)
 {
 	php_random_status_state_pcgoneseq128xslrr64 *s = state;
@@ -51,7 +56,10 @@ static php_random_result generate(void *state)
 
 	return (php_random_result){
 		.size = sizeof(uint64_t),
-		.result = php_random_pcgoneseq128xslrr64_rotr64(s->state),
+		.result = rotr(
+			(php_random_uint128_hi(s->state) ^ php_random_uint128_lo(s->state)),
+			php_random_uint128_hi(s->state) >> 58U
+		),
 	};
 }
 
diff --git i/ext/random/php_random_uint128.h w/ext/random/php_random_uint128.h
index 8c484ce5ba..c94660140e 100644
--- i/ext/random/php_random_uint128.h
+++ w/ext/random/php_random_uint128.h
@@ -28,17 +28,17 @@ typedef struct _php_random_uint128_t {
 	uint64_t lo;
 } php_random_uint128_t;
 
-static inline uint64_t php_random_uint128_hi(php_random_uint128_t num)
+ZEND_ATTRIBUTE_CONST static inline uint64_t php_random_uint128_hi(php_random_uint128_t num)
 {
 	return num.hi;
 }
 
-static inline uint64_t php_random_uint128_lo(php_random_uint128_t num)
+ZEND_ATTRIBUTE_CONST static inline uint64_t php_random_uint128_lo(php_random_uint128_t num)
 {
 	return num.lo;
 }
 
-static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint64_t lo)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint64_t lo)
 {
 	php_random_uint128_t r;
 
@@ -48,7 +48,7 @@ static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint
 	return r;
 }
 
-static inline php_random_uint128_t php_random_uint128_add(php_random_uint128_t num1, php_random_uint128_t num2)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_add(php_random_uint128_t num1, php_random_uint128_t num2)
 {
 	php_random_uint128_t r;
 
@@ -58,7 +58,7 @@ static inline php_random_uint128_t php_random_uint128_add(php_random_uint128_t n
 	return r;
 }
 
-static inline php_random_uint128_t php_random_uint128_multiply(php_random_uint128_t num1, php_random_uint128_t num2)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_multiply(php_random_uint128_t num1, php_random_uint128_t num2)
 {
 	php_random_uint128_t r;
 	const uint64_t
@@ -74,29 +74,20 @@ static inline php_random_uint128_t php_random_uint128_multiply(php_random_uint12
 
 	return r;
 }
-
-static inline uint64_t php_random_pcgoneseq128xslrr64_rotr64(php_random_uint128_t num)
-{
-	const uint64_t
-		v = (num.hi ^ num.lo),
-		s = num.hi >> 58U;
-
-	return (v >> s) | (v << ((-s) & 63));
-}
 # else
 typedef __uint128_t php_random_uint128_t;
 
-static inline uint64_t php_random_uint128_hi(php_random_uint128_t num)
+ZEND_ATTRIBUTE_CONST static inline uint64_t php_random_uint128_hi(php_random_uint128_t num)
 {
 	return (uint64_t) (num >> 64);
 }
 
-static inline uint64_t php_random_uint128_lo(php_random_uint128_t num)
+ZEND_ATTRIBUTE_CONST static inline uint64_t php_random_uint128_lo(php_random_uint128_t num)
 {
 	return (uint64_t) num;
 }
 
-static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint64_t lo)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint64_t lo)
 {
 	php_random_uint128_t r;
 
@@ -105,24 +96,15 @@ static inline php_random_uint128_t php_random_uint128_constant(uint64_t hi, uint
 	return r;
 }
 
-static inline php_random_uint128_t php_random_uint128_add(php_random_uint128_t num1, php_random_uint128_t num2)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_add(php_random_uint128_t num1, php_random_uint128_t num2)
 {
 	return num1 + num2;
 }
 
-static inline php_random_uint128_t php_random_uint128_multiply(php_random_uint128_t num1, php_random_uint128_t num2)
+ZEND_ATTRIBUTE_CONST static inline php_random_uint128_t php_random_uint128_multiply(php_random_uint128_t num1, php_random_uint128_t num2)
 {
 	return num1 * num2;
 }
-
-static inline uint64_t php_random_pcgoneseq128xslrr64_rotr64(php_random_uint128_t num)
-{
-	const uint64_t
-		v = ((uint64_t) (num >> 64U)) ^ (uint64_t) num,
-		s = num >> 122U;
-
-	return (v >> s) | (v << ((-s) & 63));
-}
 # endif
 
 #endif	/* PHP_RANDOM_UINT128_H */

[cmb69]

But I would be somewhat unhappy to diverge from the reference implementation just to satisfy a compiler warning about perfectly legal code.

That is my point: if the warning is indeed bogus, we can just ignore it (or could disable it for that particular code fragment, if someone wants us to). And I'm still quite confident that the code is perfectly valid for C99; I'm still not sure about C++, though.

If you want me to check that patch, I'll gladly do it, but I'm not opposed to leave the code as is, particularly if it matches the reference implementation.

[zeriyoshi]

I am rather in favor of changing the reference implementation to suppress warnings. I don't think there is much benefit to be gained by following the reference implementation.

However, my C / C++ (especially C++) skills are not high and I would respect the views of the experts either way.

[cmb69]

Hmm, I did a minimal PHP build with /w14146 (i.e. treat C4146 as level 1 warning), and got warnings raised all over the place. Fixing this in only public headers doesn't make sense to me (besides diverging from the reference implementation can make future updates harder).

Of course, we could generally suppress the warning, or for every single line (#pragma warning(suppress: 4146)), but neither solution appears sensible to me (at least when applying unary minus to literals, the warning makes sense, basically the same issue as https://3v4l.org/NsksW, and having #pragmas all over the place, probably guarded by #ifdef _MSC is ugly).

So either we replace all -u with (~u + 1), or we just ignore the issue, unless somebody complains. The former would be a small step to cleaner builds with MSVC (currently, we are only free of W1 warnings on x64), but might not as optimized by compilers.

[cmb69]

Well, blindly replacing all -u with (~u + 1) doesn't look like a good idea, due to several macro definitions would be affected, and if these macros are applied to signed values, that might be bad.

So I ran a build with /sdl /wd4146 (i.e. disable C4146, but check for all other SDL related issues), and got a lot of C4703, e.g. for previous_filename in

php-src/Zend/zend_ast.c

Lines 551 to 565 in a578c27

	zend_string *previous_filename;
	zend_long previous_lineno;
	if (scope) {
	previous_filename = EG(filename_override);
	previous_lineno = EG(lineno_override);
	EG(filename_override) = scope->info.user.filename;
	EG(lineno_override) = zend_ast_get_lineno(ast);
	}
	zend_result r = zend_ast_evaluate_inner(result, ast, scope, short_circuited_ptr, ctx);
	if (scope) {
	EG(filename_override) = previous_filename;
	EG(lineno_override) = previous_lineno;
	}
	return r;
	}

That code looks perfectly fine, since the use is guarded by scope (which does not appear to change). I don't think it makes sense to try to fix all these probably bogus warnings.

[TimWolla]

So, should we close this as Wont Fix then?

[cmb69]

Yes.