Skip to content

do not expose mp_balance_mul, add MP_ENABLED feature detection #213

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

Closed
wants to merge 9 commits into from
Closed

do not expose mp_balance_mul, add MP_ENABLED feature detection #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb73a57
do not expose mp_balance_mul, reorganization in mp_mul
minad Apr 8, 2019
f4f50be
introduce MP_ENABLED
minad Apr 8, 2019
620c76f
apply MP_ENABLED at other places
minad Apr 8, 2019
e7fdc96
apply to bn_mp_invmod
minad Apr 8, 2019
9421a48
restrict the MP_ENABLED macro to only work with the BN_*_C defines
minad Apr 8, 2019
8ae19ce
formatting again
minad Apr 8, 2019
2752429
apply to bn_mp_sqr
minad Apr 8, 2019
7024133
apply to mp_div_d
minad Apr 8, 2019
0354365
apply to exptmod
minad Apr 8, 2019
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
4 changes: 1 addition & 3 deletions bn_mp_div_d.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,10 @@ int mp_div_d(const mp_int *a, mp_digit b, mp_int *c, mp_digit *d)
return MP_OKAY;
}

#ifdef BN_MP_DIV_3_C
/* three? */
if (b == 3u) {
if (MP_ENABLED(MP_DIV_3) && b == 3u) {
return mp_div_3(a, c, d);
}
#endif

/* no easy answer [c'est la vie]. Just division */
if ((res = mp_init_size(&q, a->used)) != MP_OKAY) {
Expand Down
36 changes: 10 additions & 26 deletions bn_mp_exptmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ int mp_exptmod(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y)
}

/* if exponent X is negative we have to recurse */
if (X->sign == MP_NEG) {
#ifdef BN_MP_INVMOD_C
if (MP_ENABLED(MP_INVMOD) && X->sign == MP_NEG) {
mp_int tmpG, tmpX;
int err;

Expand All @@ -46,50 +45,35 @@ int mp_exptmod(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y)
err = mp_exptmod(&tmpG, &tmpX, P, Y);
mp_clear_multi(&tmpG, &tmpX, NULL);
return err;
#else
} else {
/* no invmod */
return MP_VAL;
#endif
}

/* modified diminished radix reduction */
#if defined(BN_MP_REDUCE_IS_2K_L_C) && defined(BN_MP_REDUCE_2K_L_C) && defined(BN_S_MP_EXPTMOD_C)
if (mp_reduce_is_2k_l(P) == MP_YES) {
if (MP_ENABLED(MP_REDUCE_IS_2K_L) && MP_ENABLED(MP_REDUCE_2K_L) && MP_ENABLED(S_MP_EXPTMOD) &&
mp_reduce_is_2k_l(P) == MP_YES) {
return s_mp_exptmod(G, X, P, Y, 1);
}
#endif

#ifdef BN_MP_DR_IS_MODULUS_C
/* is it a DR modulus? */
dr = mp_dr_is_modulus(P);
#else
/* default to no */
dr = 0;
#endif
/* is it a DR modulus? default to no */
dr = MP_ENABLED(MP_DR_IS_MODULUS) ? mp_dr_is_modulus(P) : 0;

#ifdef BN_MP_REDUCE_IS_2K_C
/* if not, is it a unrestricted DR modulus? */
if (dr == 0) {
if (MP_ENABLED(MP_REDUCE_IS_2K) && dr == 0) {
dr = mp_reduce_is_2k(P) << 1;
}
#endif

/* if the modulus is odd or dr != 0 use the montgomery method */
#ifdef BN_MP_EXPTMOD_FAST_C
if (IS_ODD(P) || (dr != 0)) {
if (MP_ENABLED(MP_EXPTMOD_FAST) && (IS_ODD(P) || (dr != 0))) {
return mp_exptmod_fast(G, X, P, Y, dr);
} else {
#endif
#ifdef BN_S_MP_EXPTMOD_C
} else if (MP_ENABLED(S_MP_EXPTMOD)) {
/* otherwise use the generic Barrett reduction technique */
return s_mp_exptmod(G, X, P, Y, 0);
#else
} else {
/* no exptmod for evens */
return MP_VAL;
#endif
#ifdef BN_MP_EXPTMOD_FAST_C
}
#endif
}

#endif
12 changes: 4 additions & 8 deletions bn_mp_invmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,13 @@ int mp_invmod(const mp_int *a, const mp_int *b, mp_int *c)
return MP_VAL;
}

#ifdef BN_FAST_MP_INVMOD_C
/* if the modulus is odd we can use a faster routine instead */
if (IS_ODD(b)) {
if (MP_ENABLED(FAST_MP_INVMOD) && IS_ODD(b)) {
return fast_mp_invmod(a, b, c);
}
#endif

#ifdef BN_MP_INVMOD_SLOW_C
return mp_invmod_slow(a, b, c);
#else
return MP_VAL;
#endif
return MP_ENABLED(MP_INVMOD_SLOW)
? mp_invmod_slow(a, b, c)
: MP_VAL;
}
#endif
110 changes: 38 additions & 72 deletions bn_mp_mul.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,82 +6,48 @@
/* high level multiplication (handles sign) */
int mp_mul(const mp_int *a, const mp_int *b, mp_int *c)
{
int res, neg;
#ifdef BN_MP_BALANCE_MUL_C
int len_b, len_a;
#endif
int res, neg, min_len, max_len, digs;
min_len = MIN(a->used, b->used);
max_len = MAX(a->used, b->used);
digs = a->used + b->used + 1;
neg = (a->sign == b->sign) ? MP_ZPOS : MP_NEG;
#ifdef BN_MP_BALANCE_MUL_C
len_a = a->used;
len_b = b->used;

if (len_a == len_b) {
goto GO_ON;
}
/*
* Check sizes. The smaller one needs to be larger than the Karatsuba cut-off.
* The bigger one needs to be at least about one KARATSUBA_MUL_CUTOFF bigger
* to make some sense, but it depends on architecture, OS, position of the
* stars... so YMMV.
* Using it to cut the input into slices small enough for fast_s_mp_mul_digs
* was actually slower on the author's machine, but YMMV.
*/
if ((MIN(len_a, len_b) < KARATSUBA_MUL_CUTOFF)
|| ((MAX(len_a, len_b)) / 2 < KARATSUBA_MUL_CUTOFF)) {
goto GO_ON;
}
/*
* Not much effect was observed below a ratio of 1:2, but again: YMMV.
*/
if ((MAX(len_a, len_b) / MIN(len_a, len_b)) < 2) {
goto GO_ON;
}

res = mp_balance_mul(a,b,c);
goto END;

GO_ON:
#endif

/* use Toom-Cook? */
#ifdef BN_MP_TOOM_MUL_C
if (MIN(a->used, b->used) >= TOOM_MUL_CUTOFF) {
if (MP_ENABLED(MP_BALANCE_MUL) &&
/* Check sizes. The smaller one needs to be larger than the Karatsuba cut-off.
* The bigger one needs to be at least about one KARATSUBA_MUL_CUTOFF bigger
* to make some sense, but it depends on architecture, OS, position of the
* stars... so YMMV.
* Using it to cut the input into slices small enough for fast_s_mp_mul_digs
* was actually slower on the author's machine, but YMMV.
*/
(min_len >= KARATSUBA_MUL_CUTOFF) &&
(max_len / 2 >= KARATSUBA_MUL_CUTOFF) &&
/* Not much effect was observed below a ratio of 1:2, but again: YMMV. */
(max_len >= (2 * min_len))) {
res = mp_balance_mul(a,b,c);
} else if (MP_ENABLED(MP_TOOM_MUL) &&
min_len >= TOOM_MUL_CUTOFF) {
res = mp_toom_mul(a, b, c);
} else
#endif
#ifdef BN_MP_KARATSUBA_MUL_C
/* use Karatsuba? */
if (MIN(a->used, b->used) >= KARATSUBA_MUL_CUTOFF) {
res = mp_karatsuba_mul(a, b, c);
} else
#endif
{
/* can we use the fast multiplier?
*
* The fast multiplier can be used if the output will
* have less than MP_WARRAY digits and the number of
* digits won't affect carry propagation
*/
int digs = a->used + b->used + 1;

#ifdef BN_FAST_S_MP_MUL_DIGS_C
if ((digs < (int)MP_WARRAY) &&
(MIN(a->used, b->used) <=
(int)(1u << ((CHAR_BIT * sizeof(mp_word)) - (2u * (size_t)DIGIT_BIT))))) {
res = fast_s_mp_mul_digs(a, b, c, digs);
} else
#endif
{
#ifdef BN_S_MP_MUL_DIGS_C
res = s_mp_mul(a, b, c); /* uses s_mp_mul_digs */
#else
res = MP_VAL;
#endif
}
}
END:
} else if (MP_ENABLED(MP_KARATSUBA_MUL) &&
min_len >= KARATSUBA_MUL_CUTOFF) {
res = mp_karatsuba_mul(a, b, c);
} else if (MP_ENABLED(FAST_S_MP_MUL_DIGS) &&
/* can we use the fast multiplier?
*
* The fast multiplier can be used if the output will
* have less than MP_WARRAY digits and the number of
* digits won't affect carry propagation
*/
(digs < (int)MP_WARRAY) &&
(min_len <=
(int)(1u << ((CHAR_BIT * sizeof(mp_word)) - (2u * (size_t)DIGIT_BIT))))) {
res = fast_s_mp_mul_digs(a, b, c, digs);
} else if (MP_ENABLED(S_MP_MUL_DIGS)) {
res = s_mp_mul(a, b, c); /* uses s_mp_mul_digs */
} else {
res = MP_VAL;
}
c->sign = (c->used > 0) ? neg : MP_ZPOS;
return res;
}
#endif

22 changes: 8 additions & 14 deletions bn_mp_reduce.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,15 @@ int mp_reduce(mp_int *x, const mp_int *m, const mp_int *mu)
if ((res = mp_mul(&q, mu, &q)) != MP_OKAY) {
goto CLEANUP;
}
} else if (MP_ENABLED(S_MP_MUL_HIGH_DIGS) &&
(res = s_mp_mul_high_digs(&q, mu, &q, um)) != MP_OKAY) {
goto CLEANUP;
} else if (MP_ENABLED(FAST_S_MP_MUL_HIGH_DIGS) &&
(res = fast_s_mp_mul_high_digs(&q, mu, &q, um)) != MP_OKAY) {
goto CLEANUP;
} else {
#ifdef BN_S_MP_MUL_HIGH_DIGS_C
if ((res = s_mp_mul_high_digs(&q, mu, &q, um)) != MP_OKAY) {
goto CLEANUP;
}
#elif defined(BN_FAST_S_MP_MUL_HIGH_DIGS_C)
if ((res = fast_s_mp_mul_high_digs(&q, mu, &q, um)) != MP_OKAY) {
goto CLEANUP;
}
#else
{
res = MP_VAL;
goto CLEANUP;
}
#endif
res = MP_VAL;
goto CLEANUP;
}

/* q3 = q2 / b**(k+1) */
Expand Down
43 changes: 15 additions & 28 deletions bn_mp_sqr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,22 @@ int mp_sqr(const mp_int *a, mp_int *b)
{
int res;

#ifdef BN_MP_TOOM_SQR_C
/* use Toom-Cook? */
if (a->used >= TOOM_SQR_CUTOFF) {
if (MP_ENABLED(MP_TOOM_SQR) && /* use Toom-Cook? */
a->used >= TOOM_SQR_CUTOFF) {
res = mp_toom_sqr(a, b);
/* Karatsuba? */
} else
#endif
#ifdef BN_MP_KARATSUBA_SQR_C
if (a->used >= KARATSUBA_SQR_CUTOFF) {
res = mp_karatsuba_sqr(a, b);
} else
#endif
{
#ifdef BN_FAST_S_MP_SQR_C
/* can we use the fast comba multiplier? */
if ((((a->used * 2) + 1) < (int)MP_WARRAY) &&
(a->used <
(int)(1u << (((CHAR_BIT * sizeof(mp_word)) - (2u * (size_t)DIGIT_BIT)) - 1u)))) {
res = fast_s_mp_sqr(a, b);
} else
#endif
{
#ifdef BN_S_MP_SQR_C
res = s_mp_sqr(a, b);
#else
res = MP_VAL;
#endif
}
}
} else if (MP_ENABLED(MP_KARATSUBA_SQR) && /* Karatsuba? */
a->used >= KARATSUBA_SQR_CUTOFF) {
res = mp_karatsuba_sqr(a, b);
} else if (MP_ENABLED(FAST_S_MP_SQR) && /* can we use the fast comba multiplier? */
(((a->used * 2) + 1) < (int)MP_WARRAY) &&
(a->used <
(int)(1u << (((CHAR_BIT * sizeof(mp_word)) - (2u * (size_t)DIGIT_BIT)) - 1u)))) {
res = fast_s_mp_sqr(a, b);
} else if (MP_ENABLED(S_MP_SQR)) {
res = s_mp_sqr(a, b);
} else {
res = MP_VAL;
}
b->sign = MP_ZPOS;
return res;
}
Expand Down
5 changes: 4 additions & 1 deletion demo/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#include "shared.h"

/* We can also test the private API here */
#include "tommath_private.h"
Copy link
Member

@sjaeckel sjaeckel Apr 8, 2019

Choose a reason for hiding this comment

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

we can but should we?

IMO as soon as the library symbols are properly set-up you won't be able to build these tests then against the shared library

Copy link
Member Author

@minad minad Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Then we need a special test build with exposed s_* symbols. I think it is better if internal functions are tested separately. For example in the mul case we could also test the different code paths by passing the right values. But this is too fragile and I would also like to test edge cases, which would otherwise not be hit.

This would also solve the issue with exposed CUTOFF variables. We should just make those constant and test karatsuba and toom cook separately.

Copy link
Member

@sjaeckel sjaeckel Apr 8, 2019

Choose a reason for hiding this comment

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

We should just make those constant and test karatsuba and toom cook separately

IIUC they can't be made constant as they vary from platform to platform. Sure they're mostly constant now as nobody's really tweaking them but I guess that's caused by the fact that there's no proper way yet to determine them per platform. (besides running etc/tune)

Then we need a special test build with exposed s_* symbols. I think it is better if internal functions are tested separately.

I'm no big fan of explicit testing of internal functionality.

For example in the mul case we could also test the different code paths by passing the right values.

I prefer that way to make sure the entire chain works and then determine that everything is called through code coverage.

But this is too fragile

why is this too fragile?

I would also like to test edge cases

why would you want to test cases which can't be hit?

that all sounds a bit over the top to me

Copy link
Member Author

@minad minad Apr 8, 2019

Choose a reason for hiding this comment

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

I'm no big fan of explicit testing of internal functionality.

I am not saying to test arbitrary internal things. However internal functions with a well defined API should be tested, e.g., mp_balance_mul. @czurnieden also added the balance mul test to the test suite, but he did not call it directly which he should have. If only the external api should be tested, there should not even be a function test_balance_mul.

I prefer that way to make sure the entire chain works and then determine that everything is called through code coverage.

If you want that - then the test suite must be split such that internal function tests and api function tests can be compiled separately. No big deal. #if TEST_INTERNAL .... #endif.
Ensure that stuff is hit via code coverage is good practice, but not enough and it depends on external tooling. Do we have coverage set up?

Besides, if we would just start renaming internal functions to s_*, we wouldn't have to tweak the visibility settings in the first step. The s_* functions would just be internal and not belong to the public ABI by our own convention. In the next step we could make the symbols hidden.

why is this too fragile?

The test of the internal function mp_balance_mul should not depend on the criterion at which this function is selected in mp_mul.

why would you want to test cases which can't be hit?

Robustness of the functions. I would not extend functions to accept values which cannot be handled fundamentally, but for example if karatsuba also works for small numbers and these should be tested too.

that all sounds a bit over the top to me

No it is not. If you want to see something which is over the top look at the sqlite test suite. What I am proposing here is good practice and modest. It is also too much work hopefully. Furthermore there are people willing to improve stuff, like me getting this test suite refactoring started.

Copy link
Member

@sjaeckel sjaeckel Apr 8, 2019

Choose a reason for hiding this comment

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

If only the external api should be tested, there should not even be a function test_balance_mul.

why not if this test is specifically crafted to test mp_balance_mul within mp_mul?

Do we have coverage set up?

you can do make coverage :)

Besides, if we would just start renaming internal functions to s_*, we wouldn't have to tweak the visibility settings in the first step. The s_* functions would just be internal and not belong to the public ABI by our own convention. In the next step we could make the symbols hidden.

fine by me, keep the discussion points of #172 in mind

The test of the internal function mp_balance_mul should not depend on the criterion at which this function is selected in mp_mul.

what sense does a specific test even make if its counterpart isn't available? as it is now the test's can't even be built w/o mp_balance_mul

if karatsuba also works for small numbers and these should be tested too.

then set the cutoff in the testcase appropriately?


static int test_trivial_stuff(void)
{
mp_int a, b, c, d;
Expand Down Expand Up @@ -1624,7 +1627,7 @@ static int test_mp_balance_mul(void)
goto LTM_ERR;
}

if ((e = mp_mul(&a, &b, &c)) != MP_OKAY) {
if ((e = mp_balance_mul(&a, &b, &c)) != MP_OKAY) {
goto LTM_ERR;
}

Expand Down
1 change: 0 additions & 1 deletion tommath.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@ int mp_sub(const mp_int *a, const mp_int *b, mp_int *c);

/* c = a * b */
int mp_mul(const mp_int *a, const mp_int *b, mp_int *c);
int mp_balance_mul(const mp_int *a, const mp_int *b, mp_int *c);

/* b = a*a */
int mp_sqr(const mp_int *a, mp_int *b);
Expand Down
8 changes: 8 additions & 0 deletions tommath_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ extern void *XCALLOC(size_t nmemb, size_t size);
extern void XFREE(void *mem, size_t size);
#endif

/* feature detection macro */
#define _MP_ENABLED_TEST ,
#define MP_ENABLED(x) _MP_ENABLED1(BN_##x##_C)
#define _MP_ENABLED1(x) _MP_ENABLED2(_MP_ENABLED_TEST##x)
#define _MP_ENABLED2(x) _MP_ENABLED3(x 1, 0)
#define _MP_ENABLED3(x, y, ...) y

/* ---> Basic Manipulations <--- */
#define IS_ZERO(a) ((a)->used == 0)
#define IS_EVEN(a) (((a)->used == 0) || (((a)->dp[0] & 1u) == 0u))
Expand All @@ -48,6 +55,7 @@ int fast_s_mp_mul_high_digs(const mp_int *a, const mp_int *b, mp_int *c, int dig
int s_mp_mul_high_digs(const mp_int *a, const mp_int *b, mp_int *c, int digs);
int fast_s_mp_sqr(const mp_int *a, mp_int *b);
int s_mp_sqr(const mp_int *a, mp_int *b);
int mp_balance_mul(const mp_int *a, const mp_int *b, mp_int *c);
int mp_karatsuba_mul(const mp_int *a, const mp_int *b, mp_int *c);
int mp_toom_mul(const mp_int *a, const mp_int *b, mp_int *c);
int mp_karatsuba_sqr(const mp_int *a, mp_int *b);
Expand Down