Automatically check that VERIFY_CHECKs are side effect free

[sipa]

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.

[apoelstra]

ack 07d1a6e

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

[real-or-random]

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]

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]

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]

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]

(I reported the MacOS failures at cirruslabs/macos-image-templates#33 (comment) .)

[real-or-random]

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]

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

EDIT: seems it's fine.

[sipa]

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]

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]

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]

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

[real-or-random]

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.

#if SECP256K1_GNUC_PREREQ(4, 4)
    extern int secp256k1_not_supposed_to_survive()
        __attribute__((pure))
        __attribute__((error("VERIFY_CHECK cannot be proven to have no side effects")));
    #define ASSERT_NO_SIDE_EFFECT(cond) do { (void)(secp256k1_not_supposed_to_survive() || (cond)); } while(0)
#else
    extern int VERIFY_CHECK_cannot_be_proven_to_have_no_side_effects;
    #define ASSERT_NO_SIDE_EFFECT(cond) do { (void)(VERIFY_CHECK_cannot_be_proven_to_have_no_side_effects || (cond)); } while(0)
#endif

int main(void) {
    int a = 1;
    int b = 17;
    ASSERT_NO_SIDE_EFFECT(b + a);
    ASSERT_NO_SIDE_EFFECT(a = 0);
    return a;
} 

(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]

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.