-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Fix GH-16890: array_sum() with GMP can loose precision (LLP64) #16891
Conversation
We must use `mpz_fits_si_p()` instead of `mpz_fits_slong_p()` since the latter is not suitable for LLP64 data models.
https://github.com/php/php-src/actions/runs/11957635359/job/33335259816?pr=16891 😢 |
libgmp does not define `mpz_fits_si_p()`, so we use `mpz_fits_slong_p()` there.
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. |
There was a problem hiding this comment.
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.
Given that the fix required an ugly work-around for libgmp, that the reported issue was about |
boundaries should be INT_MIN <= val < INT_MAX in fact. close GH-16891
We must use
mpz_fits_si_p()
instead ofmpz_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.