Skip to content

Compiling php_random_pcgoneseq128xslrr64_rotr64() triggers C4146 #15106

Closed as not planned
Closed as not planned
Compiling php_random_pcgoneseq128xslrr64_rotr64() triggers C4146#15106
Labels
BugExtension: randomStatus: Won't Fix

Description

@cmb69
cmb69
opened on Jul 25, 2024

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

Activity

  1. cmb69
    added
    Bug
    Status: Needs Triage
    Extension: random
    on Jul 25, 2024

  2. TimWolla commented on Jul 25, 2024

    @TimWolla
    TimWolla
    on Jul 25, 2024
    Member

    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.

  3. TimWolla commented on Jul 25, 2024

    @TimWolla
    TimWolla
    on Jul 25, 2024
    Member

    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.

  4. TimWolla commented on Jul 25, 2024

    @TimWolla
    TimWolla
    on Jul 25, 2024
    Member

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

  5. cmb69 commented on Jul 25, 2024

    @cmb69
    cmb69
    on Jul 25, 2024
    MemberAuthor

    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.

  6. zeriyoshi commented on Jul 26, 2024

    @zeriyoshi
    zeriyoshi
    on Jul 26, 2024
    Contributor

    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.

  7. cmb69 commented on Jul 31, 2024

    @cmb69
    cmb69
    on Jul 31, 2024
    MemberAuthor

    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.

  8. cmb69 commented on Jul 31, 2024

    @cmb69
    cmb69
    on Jul 31, 2024
    MemberAuthor

    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.

  9. TimWolla commented on Jul 31, 2024

    @TimWolla
    TimWolla
    on Jul 31, 2024
    Member

    So, should we close this as Wont Fix then?

  10. cmb69 commented on Jul 31, 2024

    @cmb69
    cmb69
    on Jul 31, 2024
    MemberAuthor

    Yes.

  11. cmb69
    added
    Status: Won't Fix
    and removed
    Status: Needs Triage
    on Jul 31, 2024
  12. cmb69
    closed this as completedon Jul 31, 2024
  13. cmb69
    closed this as not plannedon Jul 31, 2024
  14. TimWolla
    marked MSVC Compiler warning (level 2) C4146 in php_random_uint128.h #19343 as a duplicate of this issue on Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugExtension: randomStatus: Won't Fix

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions