tests fail on CCL+GMP+Windows

[Qian Yun]

On Windows, build fricas with CCL (Clozure CL) and gmp support,
the build is successfully, but when running tests, there are a
few places (for example first test in bugs2018.input) where
FRICASsys enters into infinite loop and I can observe memory
is leaking. (to 10GB.)

This does not happen if I disable GMP support.

Luckily this problem is easy to reproduce, and backtrace
easily points to this minimal reproducible example:

(1) -> gcd(4068020004323239869867049777123762790865,
238106358327114453439438371357)


(1) - 4510077639178402975

- Qian

[Qian Yun]

The correct answer is 13936666434531148641,
and 13936666434531148641+4510077639178402975=2^64

So looks like a sign error.

- Qian

[Qian Yun]

Solved.

diff --git a/src/lib/gmp_wrap.c b/src/lib/gmp_wrap.c
index a1065104..45e3fa7b 100644
--- a/src/lib/gmp_wrap.c
+++ b/src/lib/gmp_wrap.c
@@ -165,7 +165,7 @@ gmp_wrap_gcd(mp_limb_t * rp, mp_limb_t *s1p,
rp[res] = rc;
res++;
}
- if (rp[res - 1] & (1ul << (BIT_CNT - 1))) {
+ if (rp[res - 1] & (1ull << (BIT_CNT - 1))) {
rp[res] = 0;
res++;
}

On Windows, 1ul (unsigned long) is 4 bytes instead of 8.
So changing to 1ull fixes this.

Actually there is compiler warning on this line on Windows:

../../../fricas/src/lib/gmp_wrap.c: In function 'gmp_wrap_gcd':
../../../fricas/src/lib/gmp_wrap.c:168:28: warning: left shift count >=
width of type [-Wshift-count-overflow]
168 | if (rp[res - 1] & (1ul << (BIT_CNT - 1))) {
| ^~

- Qian

[Qian Yun]

BTW, why use addition for "rlb"? The result of gcd
will always be smaller than the arguments.

The following will save a few bytes on stack allocation.

- Qian

diff --git a/src/lisp/num_gmp.lisp b/src/lisp/num_gmp.lisp
index f9015a2b..debcb796 100644
--- a/src/lisp/num_gmp.lisp
+++ b/src/lisp/num_gmp.lisp
@@ -462,7 +462,7 @@
(rl (if (< xl yl) xl yl))
(xlb (words_to_bytes xl))
(ylb (words_to_bytes yl))
- (rlb (+ xlb ylb))
+ (rlb (if (< xlb ylb) xlb ylb))
)
(declare (type fixnum xl yl rl xlb ylb rlb))

[Grégory Vanuxem]

For information I can reproduce this issue and, effectively, the patch
fixes it on my Windows 11.

> --
> You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/7018f201-d163-4fff-8dff-00f513672b2c%40gmail.com.

[Waldek Hebisch]

On Thu, Oct 17, 2024 at 08:06:02PM +0800, Qian Yun wrote:
> Solved.
>
> diff --git a/src/lib/gmp_wrap.c b/src/lib/gmp_wrap.c
> index a1065104..45e3fa7b 100644
> --- a/src/lib/gmp_wrap.c
> +++ b/src/lib/gmp_wrap.c
> @@ -165,7 +165,7 @@ gmp_wrap_gcd(mp_limb_t * rp, mp_limb_t *s1p,
> rp[res] = rc;
> res++;
> }
> - if (rp[res - 1] & (1ul << (BIT_CNT - 1))) {
> + if (rp[res - 1] & (1ull << (BIT_CNT - 1))) {
> rp[res] = 0;
> res++;
> }
>
> On Windows, 1ul (unsigned long) is 4 bytes instead of 8.
> So changing to 1ull fixes this.

OK, please commit.

--
Waldek Hebisch

[Waldek Hebisch]

On Thu, Oct 17, 2024 at 08:42:21PM +0800, Qian Yun wrote:
> BTW, why use addition for "rlb"? The result of gcd
> will always be smaller than the arguments.
>
> The following will save a few bytes on stack allocation.
>
> - Qian
>
> diff --git a/src/lisp/num_gmp.lisp b/src/lisp/num_gmp.lisp
> index f9015a2b..debcb796 100644
> --- a/src/lisp/num_gmp.lisp
> +++ b/src/lisp/num_gmp.lisp
> @@ -462,7 +462,7 @@
> (rl (if (< xl yl) xl yl))
> (xlb (words_to_bytes xl))
> (ylb (words_to_bytes yl))
> - (rlb (+ xlb ylb))
> + (rlb (if (< xlb ylb) xlb ylb))
> )
> (declare (type fixnum xl yl rl xlb ylb rlb))

I do not remember why. But gmp have its requirements, so before
going with such a change one should check what gmp requires.
Note that violating gmp requirements may work for one gmp
version and fail for different version.

--
Waldek Hebisch

[Qian Yun]

https://gmplib.org/manual/Low_002dlevel-Functions

Says the result can be up to the smaller operand.
There's no compatibility notes and the interface seems stable
for at least 10 years.

Also in the following SBCL version, we have:

(rl (if (< xl yl) xl yl))

(res (sb-bignum::%allocate-bignum rl)))

- Qian

[Waldek Hebisch]

On Fri, Oct 18, 2024 at 08:34:58AM +0800, Qian Yun wrote:
> https://gmplib.org/manual/Low_002dlevel-Functions
>
> Says the result can be up to the smaller operand.
> There's no compatibility notes and the interface seems stable
> for at least 10 years.

If gmp documentaion says that smaller operaand is OK, then
I would believe this. We had a problem in the past, when
we passed smaller buffer than the documented size. It
worked with two gmp versions, but failed with a third (middle
one).

> Also in the following SBCL version, we have:
> (rl (if (< xl yl) xl yl))
> (res (sb-bignum::%allocate-bignum rl)))

So it looks that I overlooked this.

--
Waldek Hebisch

[Qian Yun]

How to proceed then?

- Qian

[Waldek Hebisch]

On Fri, Oct 18, 2024 at 07:07:02PM +0800, Qian Yun wrote:
> How to proceed then?

Go on with the patch.

> --
> You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/3eb1048e-a4a9-47a9-8146-aac7b98c3788%40gmail.com.

--
Waldek Hebisch