Skip to content

Resolve MSVC C4146 Warning in php_random_uint128.h #19342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KaseyJenkins
Copy link
Contributor

The following code in php_random_uint128.h gives out MSVC's C4146 warning (unary minus operator applied to unsigned type, result still unsigned):

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));
}

This PR resolves it with substituting (-s) with (~s + 1)

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment on issue

@KaseyJenkins
Copy link
Contributor Author

Thank you, @TimWolla!

Have I understood it correctly that the decision has been made and cannot be reversed? :)

Quoting @cmb69

Fixing this in only public headers doesn't make sense to me

The reasoning behind this particular change in this public header is that building a PHP extension with MSVC gives out this warning and usually requires additional measures to circumvent it (e.g. patching this particular file with #pragma warning(disable:4146)).

@cmb69
Copy link
Member

cmb69 commented Aug 1, 2025

Have I understood it correctly that the decision has been made and cannot be reversed? :)

No. It's certainly not up to me (or any other individual) to make final decisions regarding php-src. So this is up for debate.

However, AFAIK this is well-defined behavior, so I don't understand why MSVC raises a warning (level 2, with /sdl even level 1) for the code. And if the behavior is well-defined, I feel uncomfortable changing the code, or just adding a pragma to silence the warning.

@KaseyJenkins
Copy link
Contributor Author

KaseyJenkins commented Aug 1, 2025

Thank you @cmb69 for your reply!

In terms of compiler optimization issues it's rather interesting that gcc seems to recognize (~x + 1) as (-x) (where x is uint), but MSVC might be 'struggling' a bit (with optimizations turned off).

    uint32_t s = 1;
    uint32_t t = -s;
    uint32_t u = ~s + 1;

GCC:

main:
        pushq   %rbp
        movq    %rsp, %rbp
        movl    $1, -4(%rbp)
       
        #  uint32_t t = -s;
        movl    -4(%rbp), %eax
        negl    %eax
        movl    %eax, -8(%rbp)
        
        # uint32_t u = ~s + 1;
        movl    -4(%rbp), %eax
        negl    %eax
        movl    %eax, -12(%rbp)
      
        movl    $0, %eax
        popq    %rbp
        ret

MSVC:

s$ = 0
t$ = 4
u$ = 8
main    PROC
$LN3:
        sub     rsp, 24
        mov     DWORD PTR s$[rsp], 1
        
        #  uint32_t t = -s;
        mov     eax, DWORD PTR s$[rsp]
        neg     eax
        mov     DWORD PTR t$[rsp], eax
        
        # uint32_t u = ~s + 1;
        mov     eax, DWORD PTR s$[rsp]
        not     eax
        inc     eax
        mov     DWORD PTR u$[rsp], eax
        
        xor     eax, eax
        add     rsp, 24
        ret     0
main    ENDP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants