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: 
    0    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:

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

    Before:

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

    With this pull:

    0$ ./src/test/test_bitcoin -t util_tests/util_check --catch_system_errors=no 
1Running 1 test case...
2test/util_tests.cpp:87 test_method: Assertion `two != 2' failed.
3Aborted (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)

    0    bool test()
1    {
  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.

    0T&& 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 …

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

    Left some more nits (can be ignored).

    ACK 3bcffa1146beb40e28bd1bcfaccf72e0d138c9c0 🐤

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 3bcffa1146beb40e28bd1bcfaccf72e0d138c9c0 🐤
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgTQgv9HP5fwUc67QUWBjyLMXITzFCx7fUOKh97aePdHaegYMjauudEKiY5TOTi
 8Ks9HFnnirQmNq23B21wTGlkJBRkGIG16v3S5dS7iDGETGepXDBc1myBJv5u2AElG
 9l4IKxqWPVs9EUAzpDaNo9nxXrz0O3xPlUVDPA46zVl+IUDM8gZ3t8uqEoJAly7Jj
109FMreyD2riiKrZ9+dEIhAsJ08/Ski9lwxforRvVvk5a8dIztKgScjHWP+TcL5bTA
11ICA6d5TF6Zbl1SpTgNFNM+Yw9rCiND+4Z/k8zDUYnrt0dF13I4uDHG1FX8xgTvL/
12yxj/0/PC8gZfphOPWnL++06MMtKuplTY5TJD3pcC2MJZORU/fJp+w1RKvhaaf0/W
13ZZzK2rj9kGeKOofSRKvqWq7zgd+8L8GK1xtV6AsCzuMGoEruXZlF0BI/CiLF1b0z
14hzunflN07ltCAl0R2LpAJ1YDazE/vY+A25K6iktkAhXpB0+6WGNGFjk1BKeXU3lX
15xDrgwPRG
16=7L0O
17-----END PGP SIGNATURE-----
  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:

     0diff --git a/src/util/check.h b/src/util/check.h
 1index 3fa49c0a3e..4c2ee4501b 100644
 2--- a/src/util/check.h
 3+++ b/src/util/check.h
 4@@ -51,21 +51,22 @@ void assertion_fail(const char* file, int line, const char* func, const char* as
 5 
 6 /** Helper for Assert() */
 7 template <typename T>
 8-inline T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 9+T&& maybe_abort_on_check(bool IS_ASSERT, T&& val, const char* file, int line, const char* func, const char* assertion)
10 {
11-    if (!val) assertion_fail(file, line, func, assertion);
12+    if (!val) {
13+        if (IS_ASSERT // Abort inside Assert()
14+#ifdef ABORT_ON_FAILED_ASSUME
15+            || true // Abort inside Assume()
16+#endif
17+        ) {
18+            assertion_fail(file, line, func, assertion);
19+        }
20+    }
21     return std::forward<T>(val);
22 }
23 
24 /** Identity function. Abort if the value compares equal to zero */
25-#define Assert(val) inline_assertion_check(val, __FILE__, __LINE__, __func__, #val)
26-
27-/** Helper for Assume() */
28-template <typename T>
29-inline T&& skip_assumption_check(T&& val)
30-{
31-    return std::forward<T>(val);
32-}
33+#define Assert(val) maybe_abort_on_check(true, val, __FILE__, __LINE__, __func__, #val)
34 
35 /**
36  * Assume is the identity function.
37@@ -77,10 +78,6 @@ inline T&& skip_assumption_check(T&& val)
38  * - For non-fatal errors in interactive sessions (e.g. RPC or command line
39  *   interfaces), CHECK_NONFATAL() might be more appropriate.
40  */
41-#ifdef ABORT_ON_FAILED_ASSUME
42-#define Assume(val) Assert(val)
43-#else
44-#define Assume(val) skip_assumption_check(val)
45-#endif
46+#define Assume(val) maybe_abort_on_check(false, val, __FILE__, __LINE__, __func__, #val)
47 
48 #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

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

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

    0  CXX      node/libbitcoin_node_a-blockstorage.o
1init.cpp:1544:77: error: passing variable 'm_block_tree_db' by reference requires holding mutex 'cs_main' [-Werror,-Wthread-safety-reference]
2        if (const auto error{CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db))}) {
3                                                                            ^
41 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 🚢

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55 🚢
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjUxwv7BclwGFAkVZZoIl0/IF1gA7gkGSNIRdHWw0oS+fOvd7SLO369yYsTParO
 8DdQ8FLTNoptoKsSJ2C6CyNl1zZ0WV7Wvndj66uFjsHntn5N4OWwBm+731zyBMRJo
 9xbOloJXrcLj5Aw1KXBCuKN4mylLN/PFythzdq2+VnAtjOTjw59JYKpqktfSQAXS3
10+51l9BCZJf/F2G4pa1y8yKqyeuAr+IGsC/4SvFybXCaFLiRAR8A+4/TFsAmTJnYn
11T3csvAXoLJDfdn0/mGV2d5pSlPwxqvIMJdhW01cm4Oj2BDgPdsSUizLiTATtTvIG
1269a2nOO9mDBNbtfj7Wy/BATVRMQDVBxZeafdbCYtUO7/BjyAzAE7hSOODAMtv98p
13XQFfDvhBZSsuYEaADQELDx1FqNn7fqz7I/NjiWZbz9+onINH8t+IDlO+qUcChQ3w
14aIX/q1FjmZVxWtqf/kiKyann0Gx3plqjnBFSv/54P0K3D5s4yS1BWNSZOGyciGb+
15Cm6dc3hb
16=xsn6
17-----END PGP SIGNATURE-----
  33. jonatack commented at 2:04 pm on March 30, 2022: member

    Tested re-ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55

    Nice updates in the last push.

    not worth changing unless you retouch for something else

     0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index b5d8411e1d..95d84593aa 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -79,9 +79,8 @@ BOOST_AUTO_TEST_CASE(util_datadir)
 5
 6-class NoCopyOrMove
 7+struct NoCopyOrMove
 8 {
 9-public:
10     int i;
11     explicit NoCopyOrMove(int i) : i{i} { }
12 
13diff --git a/src/util/check.cpp b/src/util/check.cpp
14index 2a9f885560..56d66f78c6 100644
15--- a/src/util/check.cpp
16+++ b/src/util/check.cpp
17@@ -8,7 +8,7 @@
18 
19 void assertion_fail(const char* file, int line, const char* func, const char* assertion)
20 {
21-    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
22+    const auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
23     fwrite(str.data(), 1, str.size(), stderr);
24     std::abort();
25 }
  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:

     0./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]’:
 1./validation.h:207:13:   required from here
 2./util/check.h:54:49: warning: parameter ‘file’ set but not used [-Wunused-but-set-parameter]
 3   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 4      |                                     ~~~~~~~~~~~~^~~~
 5./util/check.h:54:59: warning: parameter ‘line’ set but not used [-Wunused-but-set-parameter]
 6   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 7      |                                                       ~~~~^~~~
 8./util/check.h:54:77: warning: parameter ‘func’ set but not used [-Wunused-but-set-parameter]
 9   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
10      |                                                                 ~~~~~~~~~~~~^~~~
11./util/check.h:54:95: warning: parameter ‘assertion’ set but not used [-Wunused-but-set-parameter]
12   54 | T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
13      |                                                                                   ~~~~~~~~~~~~^~~~~~~~~
14  CXX      test/test_bitcoin-txrequest_tests.o

    configure options:

     0Options used to compile and link:
 1  external signer = yes
 2  multiprocess    = no
 3  with experimental syscall sandbox support = yes
 4  with libs       = yes
 5  with wallet     = no
 6  with gui / qt   = yes
 7    with qr       = no
 8  with zmq        = yes
 9  with test       = yes
10  with fuzz binary = yes
11  with bench      = yes
12  with upnp       = no
13  with natpmp     = no
14  use asm         = yes
15  USDT tracing    = no
16  sanitizers      = 
17  debug enabled   = no
18  gprof enabled   = no
19  werror          = no
20  LTO             = no
21
22  target os       = linux-gnu
23  build os        = linux-gnu
24
25  CC              = /usr/bin/ccache gcc
26  CFLAGS          = -pthread -g -O2
27  CPPFLAGS        =  -fmacro-prefix-map=$(abs_top_srcdir)=.  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION
28  CXX             = /usr/bin/ccache g++ -std=c++17
29  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
30  LDFLAGS         =  -lpthread  -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie  
31  ARFLAGS         = cr

    0→ gcc --version
1gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
2Copyright (C) 2019 Free Software Foundation, Inc.
3This is free software; see the source for copying conditions.  There is NO
4warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
5→ g++ --version
6g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
7Copyright (C) 2019 Free Software Foundation, Inc.
8This is free software; see the source for copying conditions.  There is NO
9warranty; 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)
     0diff --git a/src/util/check.h b/src/util/check.h
 1index 80e973e7e..dea498293 100644
 2--- a/src/util/check.h
 3+++ b/src/util/check.h
 4@@ -53,6 +53,7 @@ void assertion_fail(const char* file, int line, const char* func, const char* as
 5 template <bool IS_ASSERT, typename T>
 6 T&& inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
 7 {
 8+       (void)file,(void)line,(void)func,(void)assertion;
 9     if constexpr (IS_ASSERT
10 #ifdef ABORT_ON_FAILED_ASSUME
11                   || 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


ajtowns MarcoFalke jonatack jnewbery


practicalswift

Labels
Refactoring

Milestone
24.0

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: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me