Skip to content
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

Use built-ins for addition and subtraction on Windows #17472

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 14, 2025

For Clang, we just need to define the respective macros, since these built-ins are available in all supported Clang versions (>= 4.0.0, currently)[1].

For MSVC (and possibly other compilers) we use the respective APIs of intsafe.h[2] which are available as of Windows 7/Server 2008 R2.

This avoids the UB due to signed integer overflow that may happen with our fallback implementations.

[1] https://releases.llvm.org/4.0.0/tools/clang/docs/LanguageExtensions.html
[2] https://learn.microsoft.com/en-us/windows/win32/api/intsafe/

For Clang, we just need to define the respective macros, since these
built-ins are available in all supported Clang versions (>= 4.0.0,
currently)[1].

For MSVC (and possibly other compilers) we use the respective APIs of
intsafe.h[2] which are available as of Windows 7/Server 2008 R2.

This avoids the UB due to signed integer overflow that may happen with
our fallback implementations.

[1] <https://releases.llvm.org/4.0.0/tools/clang/docs/LanguageExtensions.html>
[2] <https://learn.microsoft.com/en-us/windows/win32/api/intsafe/>
This shouldn't be defined unconditionally, but since it is apparently
unused, we remove it altogether.
@cmb69 cmb69 requested a review from SakiTakamachi as a code owner January 14, 2025 20:07
@cmb69 cmb69 marked this pull request as draft January 14, 2025 20:43
@cmb69
Copy link
Member Author

cmb69 commented Jan 14, 2025

Need to check the test failures.

@cmb69 cmb69 marked this pull request as ready for review January 14, 2025 23:00
@cmb69
Copy link
Member Author

cmb69 commented Jan 14, 2025

I'll follow up with a similar improvement for ZEND_SIGNED_MULTIPLY_LONG() as soon as possible.

@cmb69 cmb69 requested a review from dstogov January 14, 2025 23:00
@dstogov
Copy link
Member

dstogov commented Jan 14, 2025

For MSVC (and possibly other compilers) we use the respective APIs of intsafe.h[2] which are available as of Windows 7/Server 2008 R2.

If this is not a problem, I don't see other problems.

This avoids the UB due to signed integer overflow that may happen with our fallback implementations.

@nielsdos could you please take a look. This is probably may be fixed similar to your fix for the IR.

@cmb69
Copy link
Member Author

cmb69 commented Jan 15, 2025

If this is not a problem, […]

No problem. We require Windows 7/2008 RC2 as of PHP 7.2.0 already (and Windows 8/Server 2012 as of PHP 8.3.0).

@nielsdos could you please take a look. This is probably may be fixed similar to your fix for the IR.

Background: I've noticed the UB when running the Zend/test suite with Clang UBSan on Windows. At first I was very surprised that this hasn't come up already, but when looking at the code, I saw that there is already special casing for the most important compilers/systems (using either inline ASM or _builtin*()). So it might not be that important to fix the fallbacks.

@cmb69 cmb69 merged commit 7512685 into php:master Jan 15, 2025
10 checks passed
@cmb69 cmb69 deleted the cmb/add-sub-win branch January 15, 2025 11:59
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.

2 participants