Remove usage of CHECK from non-test file #1149

Currently CHECK is used only in test and bench mark files except for one usage in ecmult_impl.h.

We would like to move the definition of CHECK out of util.h so that util.h no longer has a hard dependency on stdio.h.

Done as part of an effort to allow secp256k1 to be compiled to WASM as part of rust-secp256k1.

Note to reviewers

Please review carefully, I don't actually know if this patch is correct. Done while working on #1095. I'm happy to make any changes both in concept and execution - I'm super rusty at C programming.

cc real-or-random

utACK 0a3ebd7a364da68ce760e773955b0ac8e772a41f

I wasn't even aware that there's a CHECK left in the main code, good to remove it.

I was about to say leave the #ifdef VERIFY in but the code is safer without it due to the side-effect on bit. This is a side-effect that we can afford -- any sane compiler will remove this code anyway.

I also utACK this approach -- though I worry that we'll get "value of bit is never read" errors, which I haven't checked.

But @tcharding why does this affect your wasm build at all? This code should be completely elided if you don't have VERIFY set, so it shouldn't matter what's in there (as long as it can be parsed by cpp).

Why does this remove the #ifdef CHECK around this whole loop? That looks like it could result in meaningfully worse performance if the compiler isn't smart enough the loop is free of side-effects.

But @tcharding why does this affect your wasm build at all? This code should be completely elided if you don't have VERIFY set, so it shouldn't matter what's in there (as long as it can be parsed by cpp).

I think it doesn't affect the wasm build. It's just that I suggested this as an additional change in #1148.

Why does this remove the #ifdef CHECK around this whole loop? That looks like it could result in meaningfully worse performance if the compiler isn't smart enough the loop is free of side-effects.

You mean #ifdef VERIFY, I guess. Yes, we could leave it it but it's literally a dead store, except for the loop condition. Even the oldest available gcc on godbolt manages to remove this: https://gcc.godbolt.org/z/5arb8EaMn

though I worry that we'll get "value of bit is never read" errors, which I haven't checked.

No, it is read (in the loop condition :) ).

You mean #ifdef VERIFY, I guess. Yes, we could leave it it but it's literally a dead store, except for the loop condition. Even the oldest available gcc on godbolt manages to remove this: https://gcc.godbolt.org/z/5arb8EaMn

VERIFY_CHECK doesn't macro-expand to nothing; it expands to do { (void)(cond); } while(0);, so that equivalence requires the compiler to reason through the fact that secp256k1_scalar_get_bits is side-effect free.

VERIFY_CHECK doesn't macro-expand to nothing; it expands to do { (void)(cond); } while(0);, so that equivalence requires the compiler to reason through the fact that secp256k1_scalar_get_bits is side-effect free.

Oh, sure. Ok, then I guess it's better to keep the #ifdef VERIFY.

Awesome, thanks lads, will re-spin.

Sorry for spending too much thoughts on this piece of test code. ;) But I think a very clean way would be to have a new variable in a new scope, like

#ifdef VERIFY 
{
    int verify_bit = bit;
    while (verify_bit < 256) {
        VERIFY_CHECK(...);
        veirfy_bit++;
    }
}
#endif

This pattern avoids any side effects on the non-VERIFY code... Of course, this particular piece of code is simple enough so that it doesn't really matter. But this seems to be a clean approach whenever we have some extra code in a #ifdef VERIFY block.

Thanks @real-or-random, your code snippet wasn't quite complete but I think I've distilled out the essence of it :)

Changes in force push:

• Better worded the commit log to mention the intent of moving CHECK.
• Kept the ifdef on VERIFY.
• Used a new local variable instead of mutating bit as suggested.

thanks

I'm getting a compiler warning for "mixed declarations and code", what is the project style guide for such a thing please?

src/ecmult_impl.h: In function ‘secp256k1_ecmult_wnaf’: src/ecmult_impl.h:205:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] 205 | int verify_bit;

Should I use an ugly declaration at the top of the function:

#ifdef VERIFY
    int verify_bit;
#endif    

Added braces as suggested and cleared compiler warning.

utACK 6a965b6b98bc08646c87bcfc826181e317079a9e

utACK 6a965b6b98bc08646c87bcfc826181e317079a9e