util/check: Don't use a lambda for Assert/Assume #24714

pull ajtowns wants to merge 2 commits into bitcoin:master from ajtowns:202203-assume changing 6 files +67 −9
  1. ajtowns commented at 3:15 AM on March 30, 2022: member

    Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.

    Fixes #21596 Fixes #24654

  2. ajtowns added the label Refactoring on Mar 30, 2022
  3. ajtowns requested review from MarcoFalke on Mar 30, 2022
  4. ajtowns requested review from practicalswift on Mar 30, 2022
  5. ajtowns force-pushed on Mar 30, 2022
  6. in src/wallet/test/wallet_test_fixture.h:25 in a26daad358 outdated 
      18 | @@ -19,10 +19,13 @@ namespace wallet {
  19 |  /** Testing setup and teardown for wallet.
  20 |   */
  21 |  struct WalletTestingSetup : public TestingSetup {
  22 | -    explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
  23 | +    WalletTestingSetup() : WalletTestingSetup(CBaseChainParams::MAIN) { }
  24 | +    explicit WalletTestingSetup(const std::string& chainName);
  25 | +    WalletTestingSetup(const WalletTestingSetup&) = delete;
  26 | +    WalletTestingSetup(WalletTestingSetup&&) = delete;
    MarcoFalke commented at 6:10 AM on March 30, 2022:

    The changes look unrelated? Also, anything with a unique_ptr member should have them deleted already, before and after the changes here.

  7. in src/wallet/test/wallet_test_fixture.h:25 in a26daad358 outdated 
      25 | +    WalletTestingSetup(const WalletTestingSetup&) = delete;
  26 | +    WalletTestingSetup(WalletTestingSetup&&) = delete;
  27 |      ~WalletTestingSetup();
  28 |  
  29 | -    std::unique_ptr<interfaces::WalletLoader> m_wallet_loader = interfaces::MakeWalletLoader(*m_node.chain, *Assert(m_node.args));
  30 | +    std::unique_ptr<interfaces::WalletLoader> m_wallet_loader;
    MarcoFalke commented at 6:10 AM on March 30, 2022:

    I presume this is to silence clang TS warnings? If so, could mention it in the commit body.

    ajtowns commented at 6:48 AM on March 30, 2022:

    Assert uses __func__ which is not defined outside of a function body, but the initializer list counts as part of the constructor body.

  8. in src/test/util_tests.cpp:85 in b4d788499e outdated 
      77 | @@ -78,6 +78,29 @@ BOOST_AUTO_TEST_CASE(util_datadir)
  78 |      BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
  79 |  }
  80 |  
  81 | +namespace {
  82 | +class NoCopyOrMove
  83 | +{
  84 | +public:
  85 | +    int i = 0;
    MarcoFalke commented at 6:13 AM on March 30, 2022: 
        int i;

    nit: Can leave un-initialized for clarity, since the default value is never used.

  9. in src/test/util_tests.cpp:121 in b4d788499e outdated 
     111 | @@ -89,6 +112,14 @@ BOOST_AUTO_TEST_CASE(util_check)
 112 |      // Check that Assume can be used as unary expression
 113 |      const bool result{Assume(two == 2)};
 114 |      Assert(result);
 115 | +
 116 | +    // Check that Assert doesn't require copy/move
 117 | +    NoCopyOrMove x{9};
 118 | +    Assert(x).i += 3;
 119 | +    Assert(x).test();
    MarcoFalke commented at 6:15 AM on March 30, 2022:

    Maybe check the return value, if that was the point?

    ajtowns commented at 7:23 AM on March 30, 2022:

    Point was mostly to call test()

  10. in src/test/util_tests.cpp:99 in b4d788499e outdated 
      94 | +    int getp1() { return i + 1; }
  95 | +    int test() {
  96 | +        // Check that Assume can be used within a lambda and still call methods
  97 | +        [&]() { Assume(getp1()); }();
  98 | +        if (Assume(getp1() != 5)) return 3;
  99 | +        return 9;
    MarcoFalke commented at 6:15 AM on March 30, 2022:

    Looks like it would be sufficient to return a bool with either true or false, here?

  11. in src/test/util_tests.cpp:122 in b4d788499e outdated 
     117 | +    NoCopyOrMove x{9};
 118 | +    Assert(x).i += 3;
 119 | +    Assert(x).test();
 120 | +
 121 | +    // Check nested Asserts
 122 | +    Assert( Assert(x).i != 5 );
    MarcoFalke commented at 6:15 AM on March 30, 2022:

    clang-format new code?

  12. MarcoFalke approved
  13. MarcoFalke commented at 6:17 AM on March 30, 2022: member

    Looks good, some nits.

  14. wallet: move Assert() check into constructor
    This puts it in a function body, so that __func__ is available
for reporting any assertion failure.
    7c9fe25c16
  15. ajtowns force-pushed on Mar 30, 2022
  16. ajtowns commented at 7:24 AM on March 30, 2022: member

    Nits picked

  17. MarcoFalke commented at 11:27 AM on March 30, 2022: member

    Tested with diff:

    diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 1881573e7a..d7328ff450 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(util_check)
     const std::unique_ptr<int> p_two = Assert(std::make_unique<int>(2));
     // Check that Assert works on lvalues and rvalues
     const int two = *Assert(p_two);
-    Assert(two == 2);
+    Assert(two != 2);
     Assert(true);
     // Check that Assume can be used as unary expression
     const bool result{Assume(two == 2)};

    Before:

    $ ./src/test/test_bitcoin -t util_tests/util_check --catch_system_errors=no 
Running 1 test case...
test_bitcoin: test/util_tests.cpp:87: auto util_tests::util_check::test_method()::(anonymous class)::operator()() const: Assertion `"two != 2" && check' failed.
Aborted (core dumped)

    With this pull:

    $ ./src/test/test_bitcoin -t util_tests/util_check --catch_system_errors=no 
Running 1 test case...
test/util_tests.cpp:87 test_method: Assertion `two != 2' failed.
Aborted (core dumped)
  18. in src/test/util_tests.cpp:97 in 3bcffa1146 outdated 
      92 | +    NoCopyOrMove& operator=(NoCopyOrMove&&) = delete;
  93 | +
  94 | +    operator bool() const { return i != 0; }
  95 | +
  96 | +    int get_ip1() { return i + 1; }
  97 | +    bool test() {
    MarcoFalke commented at 11:31 AM on March 30, 2022:

    clang-format nit (in case you retouch for other reasons)

        bool test()
    {
  19. in src/util/check.h:54 in 3bcffa1146 outdated 
      50 | +void assertion_fail(const char* file, int line, const char* func, const char* assertion);
  51 | +
  52 |  /** Helper for Assert() */
  53 |  template <typename T>
  54 | -T get_pure_r_value(T&& val)
  55 | +inline T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
    MarcoFalke commented at 11:31 AM on March 30, 2022:

    nit: All templates are inline.

    T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
  20. in src/util/check.h:56 in 3bcffa1146 outdated 
      52 |  /** Helper for Assert() */
  53 |  template <typename T>
  54 | -T get_pure_r_value(T&& val)
  55 | +inline T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
  56 |  {
  57 | +    if (!val) assertion_fail(file, line, func, assertion);
    MarcoFalke commented at 11:34 AM on March 30, 2022:

    nit: I think it is better to use a new line unless it is a trivial single statement like return or continue or break ...

        if (!val) {
        assertion_fail(file, line, func, assertion);
    }
  21. MarcoFalke approved
  22. MarcoFalke commented at 11:35 AM on March 30, 2022: member

    Left some more nits (can be ignored).

    ACK 3bcffa1146beb40e28bd1bcfaccf72e0d138c9c0 🐤

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 3bcffa1146beb40e28bd1bcfaccf72e0d138c9c0 🐤
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgTQgv9HP5fwUc67QUWBjyLMXITzFCx7fUOKh97aePdHaegYMjauudEKiY5TOTi
Ks9HFnnirQmNq23B21wTGlkJBRkGIG16v3S5dS7iDGETGepXDBc1myBJv5u2AElG
l4IKxqWPVs9EUAzpDaNo9nxXrz0O3xPlUVDPA46zVl+IUDM8gZ3t8uqEoJAly7Jj
9FMreyD2riiKrZ9+dEIhAsJ08/Ski9lwxforRvVvk5a8dIztKgScjHWP+TcL5bTA
ICA6d5TF6Zbl1SpTgNFNM+Yw9rCiND+4Z/k8zDUYnrt0dF13I4uDHG1FX8xgTvL/
yxj/0/PC8gZfphOPWnL++06MMtKuplTY5TJD3pcC2MJZORU/fJp+w1RKvhaaf0/W
ZZzK2rj9kGeKOofSRKvqWq7zgd+8L8GK1xtV6AsCzuMGoEruXZlF0BI/CiLF1b0z
hzunflN07ltCAl0R2LpAJ1YDazE/vY+A25K6iktkAhXpB0+6WGNGFjk1BKeXU3lX
xDrgwPRG
=7L0O
-----END PGP SIGNATURE-----

    </details>

  23. in src/util/check.h:68 in 3bcffa1146 outdated 
      65 | +/** Helper for Assume() */
  66 | +template <typename T>
  67 | +inline T&& skip_assumption_check(T&& val)
  68 | +{
  69 | +    return std::forward<T>(val);
  70 | +}
    MarcoFalke commented at 12:09 PM on March 30, 2022:

    It might be possible to remove the separate helpers and just use the same function for both:

    diff --git a/src/util/check.h b/src/util/check.h
index 3fa49c0a3e..4c2ee4501b 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -51,21 +51,22 @@ void assertion_fail(const char* file, int line, const char* func, const char* as
 
 /** Helper for Assert() */
 template <typename T>
-inline T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
+T&& maybe_abort_on_check(bool IS_ASSERT, T&& val, const char* file, int line, const char* func, const char* assertion)
 {
-    if (!val) assertion_fail(file, line, func, assertion);
+    if (!val) {
+        if (IS_ASSERT // Abort inside Assert()
+#ifdef ABORT_ON_FAILED_ASSUME
+            || true // Abort inside Assume()
+#endif
+        ) {
+            assertion_fail(file, line, func, assertion);
+        }
+    }
     return std::forward<T>(val);
 }
 
 /** Identity function. Abort if the value compares equal to zero */
-#define Assert(val) inline_assertion_check(val, __FILE__, __LINE__, __func__, #val)
-
-/** Helper for Assume() */
-template <typename T>
-inline T&& skip_assumption_check(T&& val)
-{
-    return std::forward<T>(val);
-}
+#define Assert(val) maybe_abort_on_check(true, val, __FILE__, __LINE__, __func__, #val)
 
 /**
  * Assume is the identity function.
@@ -77,10 +78,6 @@ inline T&& skip_assumption_check(T&& val)
  * - 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) skip_assumption_check(val)
-#endif
+#define Assume(val) maybe_abort_on_check(false, val, __FILE__, __LINE__, __func__, #val)
 
 #endif // BITCOIN_UTIL_CHECK_H
    ajtowns commented at 1:14 PM on March 30, 2022:

    Made it a template argument instead of a function param, used if constexpr and left the !val as the inner if, but otherwise taken up.

  24. jonatack commented at 12:25 PM on March 30, 2022: member

    Concept ACK, testing

  25. MarcoFalke commented at 12:30 PM on March 30, 2022: member

    Looks like this also intentionally or accidentally fixes #21596, as the value is no longer evaluated inside the function, but before passing it to the function as parameter?

    Also, it fixes the nesting issue that is fixed only in C++20 (#23363)?

    Quite amazing that this relatively simple patch fixes three bugs.

  26. MarcoFalke added this to the milestone 24.0 on Mar 30, 2022
  27. util/check: stop using lambda for Assert/Assume 2ef47ba6c5
  28. ajtowns force-pushed on Mar 30, 2022
  29. jonatack commented at 1:15 PM on March 30, 2022: member

    ACK 3bcffa1146beb40e28bd1bcfaccf72e0d138c9c0

    If #24672 is reverted with the following diff

    --- a/src/init.cpp
+++ b/src/init.cpp
@@ -1541,7 +1541,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 8: start indexers
     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-        if (const auto error{WITH_LOCK(cs_main, return CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db)))}) {
+        if (const auto error{CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db))}) {
             return InitError(*error);
         }

    then with this pull Clang thread safety analysis is now able to detect the missing lock that on current master was previously undetected

      CXX      node/libbitcoin_node_a-blockstorage.o
init.cpp:1544:77: error: passing variable 'm_block_tree_db' by reference requires holding mutex 'cs_main' [-Werror,-Wthread-safety-reference]
        if (const auto error{CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db))}) {
                                                                            ^
1 error generated.
  30. ajtowns commented at 1:17 PM on March 30, 2022: member

    Nits picked again

  31. MarcoFalke approved
  32. MarcoFalke commented at 1:20 PM on March 30, 2022: member

    Couldn't find more nits.

    ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55 🚢

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55 🚢
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjUxwv7BclwGFAkVZZoIl0/IF1gA7gkGSNIRdHWw0oS+fOvd7SLO369yYsTParO
DdQ8FLTNoptoKsSJ2C6CyNl1zZ0WV7Wvndj66uFjsHntn5N4OWwBm+731zyBMRJo
xbOloJXrcLj5Aw1KXBCuKN4mylLN/PFythzdq2+VnAtjOTjw59JYKpqktfSQAXS3
+51l9BCZJf/F2G4pa1y8yKqyeuAr+IGsC/4SvFybXCaFLiRAR8A+4/TFsAmTJnYn
T3csvAXoLJDfdn0/mGV2d5pSlPwxqvIMJdhW01cm4Oj2BDgPdsSUizLiTATtTvIG
69a2nOO9mDBNbtfj7Wy/BATVRMQDVBxZeafdbCYtUO7/BjyAzAE7hSOODAMtv98p
XQFfDvhBZSsuYEaADQELDx1FqNn7fqz7I/NjiWZbz9+onINH8t+IDlO+qUcChQ3w
aIX/q1FjmZVxWtqf/kiKyann0Gx3plqjnBFSv/54P0K3D5s4yS1BWNSZOGyciGb+
Cm6dc3hb
=xsn6
-----END PGP SIGNATURE-----

    </details>

  33. jonatack commented at 2:04 PM on March 30, 2022: member

    Tested re-ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55

    Nice updates in the last push.

    <details><summary>couple of nits</summary><p>

    not worth changing unless you retouch for something else

    diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index b5d8411e1d..95d84593aa 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -79,9 +79,8 @@ BOOST_AUTO_TEST_CASE(util_datadir)

-class NoCopyOrMove
+struct NoCopyOrMove
 {
-public:
     int i;
     explicit NoCopyOrMove(int i) : i{i} { }
 
diff --git a/src/util/check.cpp b/src/util/check.cpp
index 2a9f885560..56d66f78c6 100644
--- a/src/util/check.cpp
+++ b/src/util/check.cpp
@@ -8,7 +8,7 @@
 
 void assertion_fail(const char* file, int line, const char* func, const char* assertion)
 {
-    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
+    const auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
     fwrite(str.data(), 1, str.size(), stderr);
     std::abort();
 }

    </p></details>

  34. MarcoFalke merged this on Mar 31, 2022
  35. MarcoFalke closed this on Mar 31, 2022
  36. jnewbery commented at 7:28 AM on March 31, 2022: member

    This appears to have introduced build warnings for me on master, when building with gcc 9:

    ./util/check.h: In instantiation of ‘T&& inline_assertion_check(T&&, const char*, int, const char*, const char*) [with bool IS_ASSERT = false; T = bool]’:
./validation.h:207:13:   required from here
./util/check.h:54:49: warning: parameter ‘file’ set but not used [-Wunused-but-set-parameter]
   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
      |                                     ~~~~~~~~~~~~^~~~
./util/check.h:54:59: warning: parameter ‘line’ set but not used [-Wunused-but-set-parameter]
   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
      |                                                       ~~~~^~~~
./util/check.h:54:77: warning: parameter ‘func’ set but not used [-Wunused-but-set-parameter]
   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
      |                                                                 ~~~~~~~~~~~~^~~~
./util/check.h:54:95: warning: parameter ‘assertion’ set but not used [-Wunused-but-set-parameter]
   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
      |                                                                                   ~~~~~~~~~~~~^~~~~~~~~
  CXX      test/test_bitcoin-txrequest_tests.o

    configure options:

    Options used to compile and link:
  external signer = yes
  multiprocess    = no
  with experimental syscall sandbox support = yes
  with libs       = yes
  with wallet     = no
  with gui / qt   = yes
    with qr       = no
  with zmq        = yes
  with test       = yes
  with fuzz binary = yes
  with bench      = yes
  with upnp       = no
  with natpmp     = no
  use asm         = yes
  USDT tracing    = no
  sanitizers      = 
  debug enabled   = no
  gprof enabled   = no
  werror          = no
  LTO             = no

  target os       = linux-gnu
  build os        = linux-gnu

  CC              = /usr/bin/ccache gcc
  CFLAGS          = -pthread -g -O2
  CPPFLAGS        =  -fmacro-prefix-map=$(abs_top_srcdir)=.  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION
  CXX             = /usr/bin/ccache g++ -std=c++17
  CXXFLAGS        =   -fdebug-prefix-map=$(abs_top_srcdir)=.  -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection  -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough  -Wno-unused-parameter -Wno-deprecated-copy   -g -O2 -fno-extended-identifiers
  LDFLAGS         =  -lpthread  -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie  
  ARFLAGS         = cr

    → gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
→ g++ --version
g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

    Updating to gcc-10 removes the warnings.

  37. MarcoFalke commented at 7:39 AM on March 31, 2022: member

    Other workarounds for gcc-9:

    • Replace if constexpr with if
    • Revert #24714 (review)
    • Mark them "used" (see diff below in this comment)
    • (edit) Mark them [[maybe_unused]] (see diff below in another comment)
    diff --git a/src/util/check.h b/src/util/check.h
index 80e973e7e..dea498293 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -53,6 +53,7 @@ void assertion_fail(const char* file, int line, const char* func, const char* as
 template <bool IS_ASSERT, typename T>
 T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 {
+       (void)file,(void)line,(void)func,(void)assertion;
     if constexpr (IS_ASSERT
 #ifdef ABORT_ON_FAILED_ASSUME
                   || true
  38. jnewbery commented at 7:49 AM on March 31, 2022: member

    I'd vote for the third (mark them used), with a comment that this is to workaround behaviour in gcc-9 and can be removed when the minimum gcc version is 10 or higher.

  39. ajtowns commented at 7:51 AM on March 31, 2022: member

    This appears to have introduced build warnings for me on master, when building with gcc 9:

    Looks like this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81676 which wasn't fixed until gcc 10

  40. ajtowns commented at 8:05 AM on March 31, 2022: member

    Looks like another workaround might be to say inline_assertion_check(T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion) -- https://en.cppreference.com/w/cpp/language/attributes/maybe_unused

  41. sidhujag referenced this in commit 2cd931a679 on Apr 3, 2022
  42. DrahtBot locked this on Mar 31, 2023
Contributors
ajtownsMarcoFalkejonatackjnewbery


practicalswift

Labels
Refactoring

Milestone
24.0

Linked (view graph)
#21596 How to tell compilers to not drop the lock stack when using Assume/Assert?#23363 Discussion: Upgrading to C++20#24654 util/check.h Assert/Assume: namespacing issues
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me