diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-12-04 11:06:57 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-12-04 11:07:28 +0100 |
commit | dca80ffb45fcc8e6eedb6dc481d500dedab4248b (patch) | |
tree | c7137925cfe6cb6740dd9bded7d6d3d5740d7ea4 | |
parent | ca759bc743c9b3b240289c467349cefc5f4444af (diff) | |
parent | faa05854f832405231c9198787a4eafe2cd4c5f0 (diff) | |
download | bitcoin-dca80ffb45fcc8e6eedb6dc481d500dedab4248b.tar.xz |
Merge #20255: util: Add Assume() identity function
faa05854f832405231c9198787a4eafe2cd4c5f0 util: Remove probably misleading TODO (MarcoFalke)
fac5efe730ab5b8694920547203deefc5252d294 util: Add Assume() identity function (MarcoFalke)
fa861569dcfff9e72686dc5f30b1faa645b4d54e util: Allow Assert(...) to be used in all contexts (practicalswift)
Pull request description:
This is needed for #20138. Please refer to the added documentation for motivation.
ACKs for top commit:
practicalswift:
cr ACK faa05854f832405231c9198787a4eafe2cd4c5f0
jnewbery:
utACK faa05854f832405231c9198787a4eafe2cd4c5f0
hebasto:
ACK faa05854f832405231c9198787a4eafe2cd4c5f0, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 72165fbd898b92ab9a79b070993fa1faa86c2e3545b6645e72c652bda295d5107bc298d0482bf3aaf0926fc0c3e6418a445c0e073b08568c44231f547f76a688
-rw-r--r-- | configure.ac | 3 | ||||
-rw-r--r-- | doc/developer-notes.md | 27 | ||||
-rw-r--r-- | src/util/check.h | 20 |
3 files changed, 48 insertions, 2 deletions
diff --git a/configure.ac b/configure.ac index 5a538a1de3..b8521d6e50 100644 --- a/configure.ac +++ b/configure.ac @@ -348,6 +348,7 @@ if test "x$enable_debug" = xyes; then AX_CHECK_PREPROC_FLAG([-DDEBUG],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DDEBUG"]],,[[$CXXFLAG_WERROR]]) AX_CHECK_PREPROC_FLAG([-DDEBUG_LOCKORDER],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DDEBUG_LOCKORDER"]],,[[$CXXFLAG_WERROR]]) + AX_CHECK_PREPROC_FLAG([-DABORT_ON_FAILED_ASSUME],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DABORT_ON_FAILED_ASSUME"]],,[[$CXXFLAG_WERROR]]) AX_CHECK_COMPILE_FLAG([-ftrapv],[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -ftrapv"],,[[$CXXFLAG_WERROR]]) fi @@ -1189,6 +1190,8 @@ if test "x$enable_fuzz" = "xyes"; then use_upnp=no use_zmq=no + AX_CHECK_PREPROC_FLAG([-DABORT_ON_FAILED_ASSUME],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DABORT_ON_FAILED_ASSUME"]],,[[$CXXFLAG_WERROR]]) + AC_MSG_CHECKING([whether main function is needed]) AX_CHECK_LINK_FLAG( [[-fsanitize=$use_sanitizers]], diff --git a/doc/developer-notes.md b/doc/developer-notes.md index fa188dbcd6..9cb416bb30 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -276,6 +276,33 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts run-time checks to keep track of which locks are held and adds warnings to the `debug.log` file if inconsistencies are detected. +### Assertions and Checks + +The util file `src/util/check.h` offers helpers to protect against coding and +internal logic bugs. They must never be used to validate user, network or any +other input. + +* `assert` or `Assert` should be used to document assumptions when any + violation would mean that it is not safe to continue program execution. The + code is always compiled with assertions enabled. + - For example, a nullptr dereference or any other logic bug in validation + code means the program code is faulty and must terminate immediately. +* `CHECK_NONFATAL` should be used for recoverable internal logic bugs. On + failure, it will throw an exception, which can be caught to recover from the + error. + - For example, a nullptr dereference or any other logic bug in RPC code + means that the RPC code is faulty and can not be executed. However, the + logic bug can be shown to the user and the program can continue to run. +* `Assume` should be used to document assumptions when program execution can + safely continue even if the assumption is violated. In debug builds it + behaves like `Assert`/`assert` to notify developers and testers about + nonfatal errors. In production it doesn't warn or log anything, though the + expression is always evaluated. + - For example it can be assumed that a variable is only initialized once, + but a failed assumption does not result in a fatal bug. A failed + assumption may or may not result in a slightly degraded user experience, + but it is safe to continue program execution. + ### Valgrind suppressions file Valgrind is a programming tool for memory debugging, memory leak detection, and diff --git a/src/util/check.h b/src/util/check.h index 9edf394492..e7620d97a0 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -46,7 +46,7 @@ class NonFatalCheckError : public std::runtime_error #error "Cannot compile without assertions!" #endif -/** Helper for Assert(). TODO remove in C++14 and replace `decltype(get_pure_r_value(val))` with `T` (templated lambda) */ +/** Helper for Assert() */ template <typename T> T get_pure_r_value(T&& val) { @@ -54,6 +54,22 @@ T get_pure_r_value(T&& val) } /** Identity function. Abort if the value compares equal to zero */ -#define Assert(val) [&]() -> decltype(get_pure_r_value(val)) { auto&& check = (val); assert(#val && check); return std::forward<decltype(get_pure_r_value(val))>(check); }() +#define Assert(val) ([&]() -> decltype(get_pure_r_value(val)) { auto&& check = (val); assert(#val && check); return std::forward<decltype(get_pure_r_value(val))>(check); }()) + +/** + * Assume is the identity function. + * + * - Should be used to run non-fatal checks. In debug builds it behaves like + * Assert()/assert() to notify developers and testers about non-fatal errors. + * In production it doesn't warn or log anything. + * - For fatal errors, use Assert(). + * - For non-fatal errors in interactive sessions (e.g. RPC or command line + * interfaces), CHECK_NONFATAL() might be more appropriate. + */ +#ifdef ABORT_ON_FAILED_ASSUME +#define Assume(val) Assert(val) +#else +#define Assume(val) ((void)(val)) +#endif #endif // BITCOIN_UTIL_CHECK_H |