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

ajtowns · 2022-03-30T03:15:50Z

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

maflcko

Looks good, some nits.

src/wallet/test/wallet_test_fixture.h

src/test/util_tests.cpp

maflcko · 2022-03-30T06:15:17Z

src/test/util_tests.cpp

+    // Check that Assert doesn't require copy/move
+    NoCopyOrMove x{9};
+    Assert(x).i += 3;
+    Assert(x).test();


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

Point was mostly to call test()

src/test/util_tests.cpp

This puts it in a function body, so that __func__ is available for reporting any assertion failure.

ajtowns · 2022-03-30T07:24:22Z

Nits picked

maflcko · 2022-03-30T11:27:19Z

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)

maflcko

Left some more nits (can be ignored).

ACK 3bcffa1 🐤

Show signature

Signature:

src/test/util_tests.cpp

src/util/check.h

jonatack · 2022-03-30T12:25:25Z

Concept ACK, testing

maflcko · 2022-03-30T12:30:24Z

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.

jonatack

ACK 3bcffa1

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.

ajtowns · 2022-03-30T13:17:39Z

Nits picked again

maflcko

Couldn't find more nits.

ACK 2ef47ba 🚢

Show signature

Signature:

jonatack

Tested re-ACK 2ef47ba

Nice updates in the last push.

couple of nits

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();
 }

jnewbery · 2022-03-31T07:28:55Z

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.

maflcko · 2022-03-31T07:39:03Z

Other workarounds for gcc-9:

Replace if constexpr with if
Revert util/check: Don't use a lambda for Assert/Assume #24714 (comment)
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

jnewbery · 2022-03-31T07:49:14Z

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.

ajtowns · 2022-03-31T07:51:26Z

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

ajtowns · 2022-03-31T08:05:46Z

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

2ef47ba util/check: stop using lambda for Assert/Assume (Anthony Towns) 7c9fe25 wallet: move Assert() check into constructor (Anthony Towns) Pull request description: 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 bitcoin#21596 Fixes bitcoin#24654 ACKs for top commit: MarcoFalke: ACK 2ef47ba 🚢 jonatack: Tested re-ACK 2ef47ba Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae

ajtowns added the Refactoring label Mar 30, 2022

ajtowns requested review from maflcko and practicalswift March 30, 2022 03:15

ajtowns mentioned this pull request Mar 30, 2022

util/check.h Assert/Assume: namespacing issues #24654

Closed

ajtowns force-pushed the 202203-assume branch from cbe8111 to b4d7884 Compare March 30, 2022 04:15

maflcko approved these changes Mar 30, 2022

View reviewed changes

wallet: move Assert() check into constructor

7c9fe25

This puts it in a function body, so that __func__ is available for reporting any assertion failure.

ajtowns force-pushed the 202203-assume branch from b4d7884 to 3bcffa1 Compare March 30, 2022 07:18

maflcko approved these changes Mar 30, 2022

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

src/util/check.h Outdated Show resolved Hide resolved

src/util/check.h Outdated Show resolved Hide resolved

maflcko reviewed Mar 30, 2022

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

maflcko added this to the 24.0 milestone Mar 30, 2022

maflcko mentioned this pull request Mar 30, 2022

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

util/check: stop using lambda for Assert/Assume

2ef47ba

ajtowns force-pushed the 202203-assume branch from 3bcffa1 to 2ef47ba Compare March 30, 2022 13:09

jonatack reviewed Mar 30, 2022

View reviewed changes

maflcko approved these changes Mar 30, 2022

View reviewed changes

maflcko mentioned this pull request Mar 30, 2022

Discussion: Upgrading to C++20 #23363

Closed

jonatack reviewed Mar 30, 2022

View reviewed changes

maflcko merged commit 87dc1dc into bitcoin:master Mar 31, 2022

maflcko mentioned this pull request Mar 31, 2022

doc: Use DecodeTxDoc helper #24721

Merged

ajtowns mentioned this pull request Apr 1, 2022

util/check: avoid unused parameter warnings #24729

Merged

bitcoin locked and limited conversation to collaborators Mar 31, 2023

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

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

Uh oh!

Conversation

ajtowns commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajtowns commented Mar 30, 2022

Uh oh!

maflcko commented Mar 30, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Mar 30, 2022

Uh oh!

maflcko commented Mar 30, 2022

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Mar 30, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 31, 2022

Uh oh!

maflcko commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 31, 2022

Uh oh!

ajtowns commented Mar 31, 2022

Uh oh!

ajtowns commented Mar 31, 2022

Uh oh!

Uh oh!

ajtowns commented Mar 30, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

maflcko commented Mar 31, 2022 •

edited

Loading