Automatically check that VERIFY_CHECKs are side effect free

sipa commented at 10:27 pm on March 12, 2021: contributor

Alternative to #902 (but contains part of it).

Permit a configure-time option to use a trick to let the compiler prove our VERIFY_CHECK statements are side-effect free. See details on https://stackoverflow.com/a/35294344, and was suggested on #902 (comment).

This is default off in order to not break builds on untested platforms (which may have different sensitivity for this kind of optimization). It can be set to auto as well, to let the configure script figure out if the compiler (and current compilation flag) permit usage of this.

sipa cross-referenced this on Mar 12, 2021 from issue Add VERIFY_ONLY_CHECK that only evaluates in VERIFY mode by sipa

apoelstra approved

apoelstra commented at 0:40 am on March 13, 2021: contributor

ack 07d1a6e189cad3a411424b52e9ba333d33585596

Nice trick using autotools to detect whether the shouldn’t_survive variable would actually survive or not!

real-or-random commented at 8:44 am on March 13, 2021: contributor

I like this approach because it’s safe and we let the compiler prove things. I have a few minor suggestions (will post these later).

I’m somewhat concerned that there may be compiler (flags) in the wild that make the autoconf check pass but still fail on the real examples in our code base. It’s somewhat hard to predict. Of course this wouldn’t be terrible. We could try and see if we get those reports.

Maybe a more conservative approach would be to restrict it to gcc and clang, or to have a --enable/--with flag to force it off. Or approach it as “compiler-as-a-static-verifier” and enable this only in a special test build (that we also add to CI).

sipa force-pushed on Mar 15, 2021

sipa commented at 5:51 am on March 15, 2021: contributor

A few changes:

Added a configure flag to enable the checks (can be set to yes, no, or auto).
By default it is off, to prevent it from biting people unexpectedly that try to compile on slightly less than platforms where the check may fail.
Made the autodetection test code stricter, as it was causing issues at -O1 (where the autodetection succeeded, but compilation would fail). With the new check, will not detect the compiler as sufficiently powerful at -O1.
Added now-needed CI configuration to enable it in most cases.

real-or-random commented at 9:33 am on March 15, 2021: contributor

I still seems to fail with ubsan. I guess ubsan disables some optimizations (there could be UB in the eliminated code).

Is there a specific reason to disable this on clang?

sipa force-pushed on Mar 15, 2021

sipa commented at 7:58 pm on March 15, 2021: contributor

I still seems to fail with ubsan. I guess ubsan disables some optimizations (there could be UB in the eliminated code).

Pushed a change to make it ‘auto’ for those (which for the time being probably means ’no’).

Is there a specific reason to disable this on clang?

Yes, CI failed. Apparently clang’s optimizer isn’t powerful enough for the auto-detect test I have now.

real-or-random commented at 10:10 pm on March 15, 2021: contributor

(I reported the MacOS failures at https://github.com/cirruslabs/osx-images/issues/33#issuecomment-799787772 .)

real-or-random commented at 2:05 pm on March 16, 2021: contributor

Is there a specific reason to disable this on clang?

Yes, CI failed. Apparently clang’s optimizer isn’t powerful enough for the auto-detect test I have now.

Are you sure? I don’t see any failure here in the previous CI runs that seems specific to clang https://cirrus-ci.com/github/bitcoin-core/secp256k1/pull/904. My local clang 11.1 seems clever enough to optimize it (ok that does not say much – the Debian image on CI has clang 7…)

sipa force-pushed on Mar 16, 2021

sipa commented at 6:55 pm on March 16, 2021: contributor

@real-or-random Trying without CHECK_SIDE_EFFECT_FREE=auto for clang to see where it fails.

EDIT: seems it’s fine.

Make fe_is_zero side-effect free 81951ac69b

Introduce VERIFY_CHECK_ONLY 545453470b

Check that VERIFY_CHECK is side-effect free 18805930ce

Remove unnecessary #ifdef VERIFY guards a3828d72f1

Enable side-effect free check in CI 75234ab0eb

sipa force-pushed on Mar 18, 2021

sipa commented at 5:30 am on March 18, 2021: contributor

Rebased after #693 merge. Had to re-introduce #902’s VERIFY_CHECK_ONLY to give checks that only apply in VERIFY-mode (and/or can’t be proven side-effect free). Given how many #ifdef VERIFY; VERIFY_CHECK(...); #endif patterns we already had that can be subsumed by that, I think this isn’t too bad.

elichai commented at 12:37 pm on March 20, 2021: contributor

If it works properly on all mainstream compilers then it’s pretty cool :) I am a little worried that some compiler at some version will be able to prove the example on autoconf but won’t be able to prove a specific condition in our code, which will cause it to not compile.

But this worry isn’t substantiated by anything.

real-or-random commented at 4:02 pm on March 20, 2021: contributor

By default it is off, to prevent it from biting people unexpectedly that try to compile on slightly less than platforms where the check may fail. @elichai

elichai commented at 5:07 pm on March 20, 2021: contributor

@real-or-random Oopps should’ve read the whole thread and not just the OP 😅 Code review ACK 75234ab0eb1c3f5e58c70711a2c0d7ba0689029a

real-or-random commented at 9:17 am on June 14, 2021: contributor

Here’s a slightly improved definition that (on GCC) gives a diagnostic which is readable and is given at compile-time instead of link time.

 0#if SECP256K1_GNUC_PREREQ(4, 4)
 1    extern int secp256k1_not_supposed_to_survive()
 2        __attribute__((pure))
 3        __attribute__((error("VERIFY_CHECK cannot be proven to have no side effects")));
 4    #define ASSERT_NO_SIDE_EFFECT(cond) do { (void)(secp256k1_not_supposed_to_survive() || (cond)); } while(0)
 5#else
 6    extern int VERIFY_CHECK_cannot_be_proven_to_have_no_side_effects;
 7    #define ASSERT_NO_SIDE_EFFECT(cond) do { (void)(VERIFY_CHECK_cannot_be_proven_to_have_no_side_effects || (cond)); } while(0)
 8#endif
 9
10int main(void) {
11    int a = 1;
12    int b = 17;
13    ASSERT_NO_SIDE_EFFECT(b + a);
14    ASSERT_NO_SIDE_EFFECT(a = 0);
15    return a;
16}

(On compiler explorer to play around with this: https://gcc.godbolt.org/z/hhhxsb9T9)

It’s a little bit strange because GCC has this error attribute only for functions, and not for variables, so we need to use a function but tell GCC that it’s a pure function. I’ve verified that this works.

As a nit, I’d prefer VERIFY_ONLY_CHECK over VERIFY_CHECK_ONLY but if you agree, feel free to add a renaming commit on top, otherwise this could be too much work…)

sipa commented at 5:43 pm on May 9, 2023: contributor

I believe this isn’t the way to go. It’s a nice idea, but in practice, our code has been accumulating more and more VERIFY_CHECKs that are surrounded by #ifdef VERIFY, just because they’re statements that we’re not sure the compiler can optimize out. With this PR, that’d mean many VERIFY_CHECK_ONLYs, which need extra reviewer scrutiny for side-effect-freeness anyway, leaving the benefit of this PR for just the tiny, very simple, checks - ones that ought to be obvious anyway to reviewers.

sipa closed this on May 9, 2023

real-or-random cross-referenced this on Jul 24, 2023 from issue Policy for VERIFY_CHECK and #ifdef VERIFY by real-or-random

Automatically check that VERIFY_CHECKs are side effect free #904