Re: Unsigned integer overflow in `toom_eval_pm2.c`

[Vincent Lefevre]

On 2023-09-04 09:52:23 +0200, marco.bodr...@tutanota.com wrote:
> Should the value ~0 be written as ~0U to be correctly assigned to an
> unsigned?

IMHO, this would be better as this would make the signedness of
the types more consistent (the goal being to have an unsigned int
for all these masks).

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Unsigned integer overflow in `toom_eval_pm2.c`

[Niels Möller]

marco.bodr...@tutanota.com writes:

> Would a comment "It returns 0 or ~0, depending on the sign of the result"
> added to all the mpn_toom_eval_ functions suffice?

Yes. It would be nice if there were some reasonable place to document
that only once, but don't know where that could be.

> From the value in {0,1} we can get mask = -value, and from the mask in
> {0, ~0} we can get value = mask & 1. Of course we can switch to a
> different convention, if someone feels it's important. I don't.

If it's actually used as a mask in several places, I'm happy to leave as
is.

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Unsigned integer overflow in `toom_eval_pm2.c`

[marco . bodrato]

Ciao,

4 set 2023, 08:02 da ni...@lysator.liu.se:

> As Vincent suggested, it would be good to document somewhere what the
> convention is (values 0 or ~0).
>

Would a comment "It returns 0 or ~0, depending on the sign of the result"
added to all the mpn_toom_eval_ functions suffice?


> And maybe change to 0 and 1 convention,
>

I see a line

mpn/generic/toom42_mul.c:  vm1_neg = mpn_toom_eval_dgr3_pm1 (as1, asm1, ap, n, 
s, a0_a2) & 1;

where we actually use 0 or 1, and many lines like

mpn/generic/toom52_mul.c:  flags = (enum toom6_flags) (toom6_vm2_neg & 
mpn_toom_eval_pm2 (as2, asm2, 4, ap, n, s, a1a3));

where we use the value as a mask: 0 or ~ 0.


>From the value in {0,1} we can get mask = -value, and from the mask in {0, ~0} 
>we can get value = mask & 1.
Of course we can switch to a different convention, if someone feels it's 
important. I don't.


> since that fits better with assigning it from the return value from
> mpn_sub_n, and it seems generally more consistent with how we handle
>

We never use mpn_sub_n to assign that value.


> boolean values elsewhere. But may need further changes, like to
> abs_sub_add_n, also noted by Vincent.
>
Yes, Vincent is right. New patch.

Should the value ~0 be written as ~0U to be correctly assigned to an unsigned?

Ĝis,
m
diff -r e3cc6f9e9753 gmp-impl.h
--- a/gmp-impl.h	Sun Aug 27 20:47:01 2023 +0200
+++ b/gmp-impl.h	Mon Sep 04 09:40:02 2023 +0200
@@ -1481,25 +1481,25 @@
 __GMP_DECLSPEC void  mpn_toom_interpolate_16pts (mp_ptr, mp_ptr, mp_ptr, mp_ptr, mp_ptr, mp_size_t, mp_size_t, int, mp_ptr);
 
 #define   mpn_toom_couple_handling __MPN(toom_couple_handling)
-__GMP_DECLSPEC void mpn_toom_couple_handling (mp_ptr, mp_size_t, mp_ptr, int, mp_size_t, int, int);
+__GMP_DECLSPEC void mpn_toom_couple_handling (mp_ptr, mp_size_t, mp_ptr, unsigned, mp_size_t, int, int);
 
 #define   mpn_toom_eval_dgr3_pm1 __MPN(toom_eval_dgr3_pm1)
-__GMP_DECLSPEC int mpn_toom_eval_dgr3_pm1 (mp_ptr, mp_ptr, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_dgr3_pm1 (mp_ptr, mp_ptr, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
 
 #define   mpn_toom_eval_dgr3_pm2 __MPN(toom_eval_dgr3_pm2)
-__GMP_DECLSPEC int mpn_toom_eval_dgr3_pm2 (mp_ptr, mp_ptr, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_dgr3_pm2 (mp_ptr, mp_ptr, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
 
 #define   mpn_toom_eval_pm1 __MPN(toom_eval_pm1)
-__GMP_DECLSPEC int mpn_toom_eval_pm1 (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_pm1 (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
 
 #define   mpn_toom_eval_pm2 __MPN(toom_eval_pm2)
-__GMP_DECLSPEC int mpn_toom_eval_pm2 (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_pm2 (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, mp_ptr);
 
 #define   mpn_toom_eval_pm2exp __MPN(toom_eval_pm2exp)
-__GMP_DECLSPEC int mpn_toom_eval_pm2exp (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, unsigned, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_pm2exp (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, unsigned, mp_ptr);
 
 #define   mpn_toom_eval_pm2rexp __MPN(toom_eval_pm2rexp)
-__GMP_DECLSPEC int mpn_toom_eval_pm2rexp (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, unsigned, mp_ptr);
+__GMP_DECLSPEC unsigned mpn_toom_eval_pm2rexp (mp_ptr, mp_ptr, unsigned, mp_srcptr, mp_size_t, mp_size_t, unsigned, mp_ptr);
 
 #define   mpn_toom22_mul __MPN(toom22_mul)
 __GMP_DECLSPEC void  mpn_toom22_mul (mp_ptr, mp_srcptr, mp_size_t, mp_srcptr, mp_size_t, mp_ptr);
diff -r e3cc6f9e9753 mpn/generic/toom54_mul.c
--- a/mpn/generic/toom54_mul.c	Sun Aug 27 20:47:01 2023 +0200
+++ b/mpn/generic/toom54_mul.c	Mon Sep 04 09:40:02 2023 +0200
@@ -61,7 +61,7 @@
 		mp_srcptr bp, mp_size_t bn, mp_ptr scratch)
 {
   mp_size_t n, s, t;
-  int sign;
+  unsigned sign;
 
   /* decomposition ***/
 #define a4  (ap + 4 * n)
diff -r e3cc6f9e9753 mpn/generic/toom63_mul.c
--- a/mpn/generic/toom63_mul.c	Sun Aug 27 20:47:01 2023 +0200
+++ b/mpn/generic/toom63_mul.c	Mon Sep 04 09:40:02 2023 +0200
@@ -37,8 +37,9 @@
 
 #include "gmp-impl.h"
 
-/* Stores |{ap,n}-{bp,n}| in {rp,n}, returns the sign. */
-static int
+/* Stores |{ap,n}-{bp,n}| in {rp,n}. */
+/* It returns 0 or ~0, depending on the sign of the result. */
+static unsigned
 abs_sub_n (mp_ptr rp, mp_srcptr ap, mp_srcptr bp, mp_size_t n)
 {
   mp_limb_t  x, y;
@@ -65,9 +66,10 @@
   return 0;
 }
 
-static int
+/* It returns 0 or ~0, depending on the sign of the result rm. */
+static unsigned
 abs_sub_add_n (mp_ptr rm, mp_ptr rp, mp_srcptr rs, mp_size_t n) {
-  int result;
+  unsigned result;
   result = abs_sub_n (rm, rp, rs, n);
   ASSERT_NOCARRY(mpn_add_n (rp, rp, rs, n));
   return result;
@@ -99,7 +101,7 @@
 {
   mp_size_t n, s, t;
   mp_limb_t cy;
-  int sign;
+  

Re: Unsigned integer overflow in `toom_eval_pm2.c`

[Andrew Teylu]

On Sun, Sep 3, 2023 at 11:40 PM  wrote:
> I attach a possible patch.
>

As an outsider, does it make sense to also change instances of "neg"
to be "sign" (if that's the interpretation) for consistency?

For someone who isn't literate in the GMP code, neg vs. sign seems to
be used slightly inconsistently, and (personally) "neg" reads as
"negated" rather than "is negative".
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs

Re: Unsigned integer overflow in `toom_eval_pm2.c`

[Niels Möller]

marco.bodr...@tutanota.com writes:

> Yes, unsigned is the best choice, it used to store a positive or
> negative number, but now it actually is a mask: 0 or ~0.
>
> I attach a possible patch.

Makes sense, I think.

As Vincent suggested, it would be good to document somewhere what the
convention is (values 0 or ~0). And maybe change to 0 and 1 convention,
since that fits better with assigning it from the return value from
mpn_sub_n, and it seems generally more consistent with how we handle
boolean values elsewhere. But may need further changes, like to
abs_sub_add_n, also noted by Vincent.

Regards,
/Niels

-- 
Niels Möller. PGP key CB4962D070D77D7FCB8BA36271D8F1FF368C6677.
Internet email is subject to wholesale government surveillance.
___
gmp-bugs mailing list
gmp-bugs@gmplib.org
https://gmplib.org/mailman/listinfo/gmp-bugs