Remove usage of CHECK from non-test file

tcharding commented at 3:33 am on November 4, 2022: contributor

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

tcharding cross-referenced this on Nov 4, 2022 from issue Conditionally include `stdio.h` and `stdlib.h` by tcharding

real-or-random approved

real-or-random commented at 11:21 am on November 4, 2022: contributor

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.

apoelstra commented at 2:37 pm on November 4, 2022: contributor

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).

sipa commented at 2:53 pm on November 4, 2022: contributor

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.

real-or-random commented at 5:48 pm on November 4, 2022: contributor

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 :) ).

sipa commented at 5:51 pm on November 4, 2022: contributor

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.

real-or-random commented at 6:33 pm on November 4, 2022: contributor

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.

tcharding commented at 8:05 pm on November 4, 2022: contributor

Awesome, thanks lads, will re-spin.

real-or-random commented at 0:09 am on November 5, 2022: contributor

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

0#ifdef VERIFY 
1{
2    int verify_bit = bit;
3    while (verify_bit < 256) {
4        VERIFY_CHECK(...);
5        veirfy_bit++;
6    }
7}
8#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.

tcharding force-pushed on Nov 7, 2022

tcharding commented at 2:06 am on November 7, 2022: contributor

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

tcharding commented at 2:08 am on November 7, 2022: contributor

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

tcharding force-pushed on Nov 7, 2022

tcharding commented at 2:15 am on November 7, 2022: contributor

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:

0
1#ifdef VERIFY
2    int verify_bit;
3#endif

Remove usage of CHECK from non-test file

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 in preparation for moving the definition of `CHECK` as part of an
effort to allow secp256k1 to be compiled to WASM as part of
`rust-secp256k1`.

6a965b6b98

in src/ecmult_impl.h:208 in 0a894ee0d0 outdated

206+    VERIFY_CHECK(carry == 0);
207+
208+    int verify_bit = bit;
209+    while (verify_bit < 256) {
210+        VERIFY_CHECK(secp256k1_scalar_get_bits(&s, verify_bit, 1) == 0);
211+        verify_bit++;

real-or-random commented at 3:34 pm on November 7, 2022:

You need to wrap that piece of code in a new block , i.e., { and }. Then the compiler will accept it. (The brackets in my suggestion weren’t a mistake ;)).

C90 has this strange rule that variable declarations must always come first in a block.

tcharding commented at 7:29 pm on November 7, 2022:

Face palm! Thanks bro

tcharding force-pushed on Nov 7, 2022

tcharding commented at 8:31 pm on November 7, 2022: contributor

Added braces as suggested and cleared compiler warning.

real-or-random approved

real-or-random commented at 2:39 am on November 8, 2022: contributor

utACK 6a965b6b98bc08646c87bcfc826181e317079a9e

sipa commented at 4:54 pm on November 16, 2022: contributor

utACK 6a965b6b98bc08646c87bcfc826181e317079a9e

real-or-random merged this on Nov 16, 2022

real-or-random closed this on Nov 16, 2022

sipa referenced this in commit 9d47e7b71b on Dec 13, 2022

dhruv referenced this in commit 55ffd47cc6 on Dec 14, 2022

dhruv referenced this in commit 967c65b158 on Dec 14, 2022

tcharding cross-referenced this on Jan 9, 2023 from issue Verification by tcharding

dhruv referenced this in commit 78b5ddf28b on Jan 11, 2023

dhruv referenced this in commit 215394a1d5 on Jan 11, 2023

div72 referenced this in commit 945b094575 on Mar 14, 2023

str4d referenced this in commit 0df7b459f6 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1147, 1149, 1000, 1155, 1156 by jonasnick

Remove usage of CHECK from non-test file #1149

Note to reviewers