Skip to content

Implement zend_safe_address() for MSVC 64bit #17679

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Implement zend_safe_address() for MSVC 64bit #17679

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Zend/zend_multiply.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,19 @@ static zend_always_inline size_t zend_safe_address(size_t nmemb, size_t size, si
return (size_t) res;
}

#elif defined(_MSC_VER)

static zend_always_inline size_t zend_safe_address(size_t nmemb, size_t size, size_t offset, bool *overflow)
{
size_t res;
if (UNEXPECTED(FAILED(ULongLongMult(nmemb, size, &res)) || FAILED(ULongLongAdd(res, offset, &res)))) {
Copy link
Member

Choose a reason for hiding this comment

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

LGTM for 64 bits, but a bit less sure it's appropriate for 32 bits with size_t though, ie UIntMult ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The 32bit case is already handled above:

php-src/Zend/zend_multiply.h

Lines 279 to 292 in 9040e79

#elif SIZEOF_SIZE_T == 4
static zend_always_inline size_t zend_safe_address(size_t nmemb, size_t size, size_t offset, bool *overflow)
{
uint64_t res = (uint64_t) nmemb * (uint64_t) size + (uint64_t) offset;
if (UNEXPECTED(res > UINT64_C(0xFFFFFFFF))) {
*overflow = 1;
return 0;
}
*overflow = 0;
return (size_t) res;
}

We could use ULongMult()/ULongAdd() on Windows, but I don't think that would be a relevant improvement (also, 32bit is probably quite uncommon on Windows nowadays).

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes saw it but I thought the api you mentioned above made a real difference, thanks for the pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

See https://godbolt.org/z/K43dPhavE for the comparision; the intrinsic versions saves an addc at the cost of a couple more instructions.

*overflow = 1;
return 0;
}
*overflow = 0;
return res;
}

#else

static zend_always_inline size_t zend_safe_address(size_t nmemb, size_t size, size_t offset, bool *overflow)
Expand Down