Skip to content

Fix GH-16890: array_sum() with GMP can loose precision (LLP64) - #16891

Closed
cmb69 wants to merge 2 commits into
php:PHP-8.3from
cmb69:cmb/gh16890
Closed

cmb69 wants to merge 2 commits into
php:PHP-8.3from
cmb69:cmb/gh16890

Conversation

@cmb69

@cmb69 cmb69 commented Nov 21, 2024

Copy link
Copy Markdown
Member

We must use mpz_fits_si_p() instead of mpz_fits_slong_p() since the latter is not suitable for LLP64 data models.


Note that I've targeted PHP-8.3 since I haven't been able to find a suitable test case. Still, I think the actual fix should go into PHP-8.2.

We must use `mpz_fits_si_p()` instead of `mpz_fits_slong_p()` since the
latter is not suitable for LLP64 data models.
@cmb69
cmb69 requested a review from Girgias as a code owner November 21, 2024 16:37
@cmb69 cmb69 linked an issue Nov 21, 2024 that may be closed by this pull request
@cmb69

cmb69 commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

@cmb69
cmb69 marked this pull request as draft November 21, 2024 16:46
libgmp does not define `mpz_fits_si_p()`, so we use `mpz_fits_slong_p()`
there.
@devnexen

Copy link
Copy Markdown
Member

Will we end up in another libreadline/libedit situation ? :)

@cmb69

cmb69 commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

Will we end up in another libreadline/libedit situation ? :)

Not really. While libreadline and libedit have been developed independently, mpir is a fork of libgmp (5?), and as such has the same license. Either can be used on POSIX systems, but only mpir made fixes for LLP64 models; unfortunately, mpir is no longer maintained. Still using libgmp on Windows (if you can even built it with MSVC) doesn't make much sense, although in the long run we may need to stop using mpir on Windows. Also note that on Windows we're not using libedit, but rather WinEditLine, which is yet again somewhat different.

@cmb69
cmb69 marked this pull request as ready for review November 21, 2024 17:45

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, a lot of this code does seem a bit jank.... hopefully after I get round to merging the custom ZPP change for master we can improve it.

@cmb69 cmb69 closed this in cfcf5cf Nov 25, 2024
@cmb69
cmb69 deleted the cmb/gh16890 branch November 25, 2024 11:57
@cmb69

cmb69 commented Nov 25, 2024

Copy link
Copy Markdown
Member Author

Given that the fix required an ugly work-around for libgmp, that the reported issue was about array_sum(), that it has been self-reported, and PHP-8.2 is in security stage in a month, I've only applied to PHP-8.3 (and merged up).

devnexen added a commit that referenced this pull request Dec 9, 2024
boundaries should be INT_MIN <= val < INT_MAX in fact.

close GH-16891
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.

array_sum() with GMP can loose precision (LLP64)

3 participants