Do not run functions with necessary side-effects in assert() #20575

Do not run functions with necessary side-effects in assert().

Concept ACK. This feels very deja-vu. Is it somehow possible to prevent these from being introduced? How did you find them?

An alternative (and shorter patch) would be to use Assert, whose expression is always evaluated, even if for some unexplainable reason NDEBUG is set.

Code review ACK 977139eac694c3039d49a982322768e8b588a52e.

@MarcoFalke that's a different approach right? It would be allowed to use stuff like Assert(--i).

Concept ACK

Concept ACK.

Thanks!

This feels very deja-vu. Is it somehow possible to prevent these from being introduced?

Heh, the deja-vu is understandable :) You asked the same question ("How can we avoid these from being introduced again?") in a gmaxwell PR with exactly the same title four years ago: #9344 ("Do not run functions with necessary side-effects in assert()") :D

Luckily assertion side-effects can trivially be guarded against using liniting. Let me know if we want such a linter: I'd be happy to write one if we have a consensus Concept ACK on that.

How did you find them?

I stumbled upon these when reading static analyzer logs in search for more severe issues. Static analysers typically flag assertions with side effects. See #18920 for instructions on how to analyze Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck.

Example:

$ CPPCHECK_VERSION=2.3
$ curl -s https://codeload.github.com/danmar/cppcheck/tar.gz/${CPPCHECK_VERSION} | tar -zxf - --directory /tmp/
$ (cd /tmp/cppcheck-${CPPCHECK_VERSION}/ && make CFGDIR=/tmp/cppcheck-${CPPCHECK_VERSION}/cfg/ > /dev/null)
$ /tmp/cppcheck-${CPPCHECK_VERSION}/cppcheck \
    --enable=all \
    --language=c++ \
    --std=c++17 \
    -D__cplusplus \
    -DCLIENT_VERSION_BUILD \
    -DCLIENT_VERSION_IS_RELEASE \
    -DCLIENT_VERSION_MAJOR \
    -DCLIENT_VERSION_MINOR \
    -DCLIENT_VERSION_REVISION \
    -DCOPYRIGHT_YEAR \
    -DDEBUG \
    -DHAVE_WORKING_BOOST_SLEEP_FOR \
    -I src/ \
    -q \
    $(git ls-files -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/") 2>&1 | \
    grep -A2 assertWithSideEffect

src/bench/chacha_poly_aead.cpp:34:21: warning: Assert statement calls a function which may have desired side effects: 'Crypt'. [assertWithSideEffect]
        assert(aead.Crypt(seqnr_payload, seqnr_aad, aad_pos, out.data(), out.size(), in.data(), buffersize, true));
                    ^
src/bench/chacha_poly_aead.cpp:38:25: warning: Assert statement calls a function which may have desired side effects: 'GetLength'. [assertWithSideEffect]
            assert(aead.GetLength(&len, seqnr_aad, aad_pos, in.data()));
                        ^
src/bench/chacha_poly_aead.cpp:39:25: warning: Assert statement calls a function which may have desired side effects: 'Crypt'. [assertWithSideEffect]
            assert(aead.Crypt(seqnr_payload, seqnr_aad, aad_pos, out.data(), out.size(), in.data(), buffersize, true));
                        ^
--
src/init.cpp:1666:40: warning: Assert statement calls a function which may have desired side effects: 'CanFlushToDisk'. [assertWithSideEffect]
                    assert(chainstate->CanFlushToDisk());
                                       ^

Luckily assertion side-effects can trivially be guarded against using liniting. Let me know if we want such a linter: I'd be happy to write one if we have a consensus Concept ACK on that.

Maybe—@sipa's find shows that at least a trivial linter could be easily confused.

An alternative (and shorter patch) would be to use Assert, whose expression is always evaluated, even if for some unexplainable reason NDEBUG is set.

This seems like the best long-term strategy to me. Move to our own assert function that is always evaluated, then forbid (using a trivial linter) use of built-in assert.

Code review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc

Code Review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc 🟢 Agree that using our own Assert would be a good long-term plan.

@practicalswift good catch ....

General Remark: /> When I read assert, my AI I read hazard too. Question: /> Morph from assert to general own Assert #20575 (comment) has my sympathy too. /> why do we here and in some other places const ... and not like in Assert i.e. auto&& check; assert(... & check); .... /> const is a promise in my view to a compiler that the src try to keep and the compiler might optimize such val out to regs.

So my question, why const and not auto&& or any clear var definition? I really would like to know why and rest my soul in peace.with that.

@Saibato

Is the question why const bool foo = bar(…) (with const) is used instead of bool foo = bar(…) (without const)?

I try to make objects immutable by default.

Rationale from C++ Core Guidelines' "Con.1: By default, make objects immutable": "Immutable objects are easier to reason about, so make objects non-const only when there is a need to change their value. Prevents accidental or hard-to-notice change of value."

Does that answer your question? :)

@practicalswift

Is the question why const bool foo = bar(…) (with const) is used instead of bool foo = bar(…) (without const)?

Yup, that was the question, thx for the reasoning for that.

And in every other case other that assert, I would say perfect and sufficient and best good practice but in case of assert we check almost always for things that are volatile var and critical and my point is if we put them even temp by promise const and expose those critical statements about what we expect to interpreted other as in the following lines we like to protect ( const maybe put by the compiler to rd-only mem or optimized out or hooked ).

I just wonder if it is in this case not better to leave them var volatile as is and declare them like they will be used in the code lines we want to protect?

@Saibato

And in every other case other that assert, I would say perfect and sufficient and best good practice but in case of assert we check almost always for things that are volatile var and critical and my point is if we put them even temp by promise const and expose those critical statements about what we expect to interpreted other as in the following lines we like to protect ( const maybe put by the compiler to rd-only mem or optimized out or hooked ).

Is the concern that this change would give permission to the compiler to optimise away the assertions?

If so, there is no need for such concern: the compiler is simply not allowed to do that :)

@practicalswift

Is the concern that this change would give permission to the compiler to optimise away the assertions?

I hope not and also that no compiler does that ever in a way i,e hey const bla = blabla(blablabla) , why update more than once, always true or false at first run and then save processor time? , I was wary more that those values land in special regs or in general treated different that have we use those values we check for in the codelines that follow assertions. Havvy nitpicking, but . that was what i thought might happen.

Two ACKs (laanwj & theStack) and one stale ACK (promag): getting ready for merge? :)

@Saibato The C and C++ languages specify exactly what kind of optimizations the compiler is allowed to make, and with a very small set of exceptions, that is the "as if" rule: optimizations may not change observable behavior: https://en.cppreference.com/w/cpp/language/as_if . Making an object const may enable optimizations (though only very rarely), but the effect can never be observable (and an assertion failure is observable).

utACK 5021810650afc3073c2af6953ff046ad4d27a1fc

@practicalswift @sipa thx for addressing my remarks I rest this now,assuming the overall consensus is that https://github.com/bitcoin/bitcoin/commit/5021810650afc3073c2af6953ff046ad4d27a1fc is well defined save behavior,