do not expose mp_balance_mul, add MP_ENABLED feature detection #213

minad · 2019-04-08T11:26:08Z

sjaeckel

TBH I preferred the way with separate if-statements with comments instead of one big if-statement with in-line comments

the rest seems fine

sjaeckel · 2019-04-08T11:42:10Z

demo/test.c

@@ -1,5 +1,8 @@
 #include "shared.h"

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


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

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.

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

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.

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?

minad · 2019-04-08T12:53:18Z

TBH I preferred the way with separate if-statements with comments instead of one big if-statement with in-line comments

Hmm written like this it is more aligned with how the conditions are checked for the other special cases karatsuba and toom. I think it is was more clear like this, but astyle makes a bit of a mess :(

minad · 2019-04-08T12:59:38Z

@sjaeckel Better like this?

sjaeckel

TBH I prefer having 5 if statements after each other and some (simple forward) goto's than such a construct

But I'm not sure if I'm alone with that opinion.

bn_mp_mul.c

sjaeckel · 2019-04-08T13:40:54Z

bn_mp_mul.c

+#ifdef BN_MP_BALANCE_MUL_C
+   else if ((a->used != b->used) &&
+
+            /* Check sizes. The smaller one needs to be larger than the Karatsuba cut-off.


it's about the entire readability and such complex if statements suck

Please compare the old code with the new code side by side, don't look at the diff only.
Then I think this PR improves things.

which old code? pre-206-merged?

Compare
https://github.com/libtom/libtommath/blob/develop/bn_mp_mul.c
with
https://github.com/libtom/libtommath/blob/41d3d73b8e19531fd476cc57355bdb1a015c5eb3/bn_mp_mul.c
and then tell me seriously if you defend the goto version.

Btw. I've the impression that this is going in the same direction as the rebase discussion.

Linearity of things, where linearity in my opinion leads to easier readability for a human.

and then tell me seriously if you defend the goto version.

BTW yes, overall it improved

For example we disagreed about what should be tested or not, this is quite basic. Then there was the discussion about the rebasing, then the discussion about about the public ABI.

I would prefer if we wouldn't over discuss too many things to be honest. If there is something to be learned ok but otherwise it just costs time. It happened a few times now - I created a (from my pov) quick and uncontroversial patch and then a longish discussion emerges. But I am not saying I am innocent in this respect ;)

The length of the patch should be proportional to the length of the discussion :)

BTW yes, overall it improved

good to hear 😅

The length of the patch should be proportional to the length of the discussion :)

True, if there are no fundamentals being discussed as e.g. the testing.

then the discussion about about the public ABI

You mean #172 ?

I created a (from my pov) quick and uncontroversial patch

I was fine with the original patch, besides the points I commented on. Then you made a v2 which implemented mp_mul entirely different and also changed parts that weren't commented on... yes this leads to re-discussion of things and new discussions...

czurnieden

if (0) {
Really?
Nothing wrong with getting rid of the gotos but it is also not completely wrong to keep them so a rewrite needs to add something more substantial than getting rid of the gotos.
So bite that sour grape and use a goto to get out of the balancing branch. That gets rid of the first two elses and, of course if (0) {
I don't think (a->used != b->used) at line 18 will save enough to be worth the ops.
And either use a variable for both MIN/MAX or for none. Please don't mix them. Many OCDers will thank you for that.
I also wouldn't use so many comparisons in a single if (it's bad enough in the preprocessor directives) but that's probably just me.

I can live with all of the above, no problem, with one exception: if (0) {
Pleeeease!

minad · 2019-04-08T20:07:27Z

* I don't think `(a->used != b->used)` at line 18 will save enough to be worth the ops.

Fixed.

* And either use a variable for both MIN/MAX or for none. Please don't mix them. Many OCDers will thank you for that.

Ok, ok :)

I can live with all of the above, no problem, with one exception: if (0) {

I will not change this one. I like it since it looks like a nice trick.

minad · 2019-04-08T20:13:15Z

Btw, we could introduce an ENABLED macro to handle the ifdefs then we could use something much nicer like this:

if (MP_ENABLED(MP_KARATSUBA_MUL) && condition) {
    ....
}

However we would then rely on the compilers dead code elimination. I usually prefer that style. If you want, I prepare a patch, but this will change quite a few lines all over tommath I guess.

Edit: @czurnieden @sjaeckel See my last commit concerning the MP_ENABLED macro.

czurnieden · 2019-04-08T20:43:33Z

. I like it since it looks like a nice trick.

I don't like it since it looks like an abomination.
An alien element.
A leftover you would automatically delete and then wonder why the compiler complains.

Please: either use a goto, or, since there is nothing to cleanup, set the sign and return from the balance branch directly.

Oh, and also: there is more to come. The TC4,5(6) can go below the balancing, no special treatment needed but FFT/NFT need a different kind of massage, must be treated before the balancing and have an immediate out, too. FFT/NFT will need some time (FFT is ready to go but NFT is not even started) but it would be nice if you don't make it too complicated to add them.

minad · 2019-04-08T20:45:10Z

bn_mp_mul.c

+#define MP_ENABLED(x)           _MP_ENABLED1(x)
+#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


@czurnieden @sjaeckel How do you like something like this?

The macro will evaluate to 1 if the macro is either defined as empty or defined as 1.

#define ONE 1 #define ZERO 0 #define EMPTY MP_ENABLED(ONE) 1 MP_ENABLED(ZERO) 0 MP_ENABLED(EMPTY) 1 MP_ENABLED(UNDEF) 0

The original macros were meant to exclude code from compiling, from going into the binary in the first place. This patch keeps the branches in and hopes for the compiler to optimize them away.
Is it the case? Have you checked the dumps of GCC and clang? What is left if compiled with -Os?

This will always work. Every compiler optimizes if (0) away. For example the Linux kernel does that too. It is actually a quite common practice. The advantage of this is that you always get the typechecking.

The only situation where you have to go back to #ifdefs is if the definition is missing or changes depending on macros.

Trying a different approach now.

minad · 2019-04-08T21:50:31Z

superseded by #214

minad force-pushed the mul-api branch from ffd7e85 to 82e5254 Compare April 8, 2019 11:27

sjaeckel reviewed Apr 8, 2019

View reviewed changes

minad force-pushed the mul-api branch from eb26cb8 to 41d3d73 Compare April 8, 2019 13:10

sjaeckel reviewed Apr 8, 2019

View reviewed changes

sjaeckel requested a review from czurnieden April 8, 2019 14:24

czurnieden previously requested changes Apr 8, 2019

View reviewed changes

do not expose mp_balance_mul, reorganization in mp_mul

cb73a57

minad requested a review from czurnieden April 8, 2019 20:08

minad force-pushed the mul-api branch from 41d3d73 to b7ac64c Compare April 8, 2019 20:23

introduce MP_ENABLED

f4f50be

minad force-pushed the mul-api branch from b7ac64c to f4f50be Compare April 8, 2019 20:44

minad commented Apr 8, 2019

View reviewed changes

minad added 3 commits April 8, 2019 23:03

apply MP_ENABLED at other places

620c76f

apply to bn_mp_invmod

e7fdc96

restrict the MP_ENABLED macro to only work with the BN_*_C defines

9421a48

minad requested a review from sjaeckel April 8, 2019 21:13

minad changed the title ~~do not expose mp_balance_mul, minor reorganization in mp_mul~~ do not expose mp_balance_mul, add MP_ENABLED feature detection Apr 8, 2019

minad added 4 commits April 8, 2019 23:17

formatting again

8ae19ce

apply to bn_mp_sqr

2752429

apply to mp_div_d

7024133

apply to exptmod

0354365

minad closed this Apr 8, 2019

minad deleted the mul-api branch November 14, 2019 14:35

do not expose mp_balance_mul, add MP_ENABLED feature detection #213

do not expose mp_balance_mul, add MP_ENABLED feature detection #213

Uh oh!

Conversation

minad commented Apr 8, 2019

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad commented Apr 8, 2019

Uh oh!

minad commented Apr 8, 2019

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjaeckel Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czurnieden left a comment

Choose a reason for hiding this comment

Uh oh!

minad commented Apr 8, 2019

Uh oh!

minad commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czurnieden commented Apr 8, 2019

Uh oh!

minad Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad commented Apr 8, 2019

Uh oh!

Uh oh!

minad Apr 8, 2019 •

edited

Loading

sjaeckel Apr 8, 2019 •

edited

Loading

minad commented Apr 8, 2019 •

edited

Loading

minad Apr 8, 2019 •

edited

Loading