util: Allow Assert (et al.) in contexts without func #33785

pull maflcko wants to merge 4 commits into bitcoin:master from maflcko:2511-assert-func changing 6 files +42 −51
  1. maflcko commented at 9:04 pm on November 4, 2025: member

    Without this, compile warnings could be hit about __func__ being only valid inside functions.

    0warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
1  115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
2      |                                                                           ^

    Ref #32740 (review)

    This also introduces a slight behaviour change, because std::source_location::function_name usually includes the entire function signature instead of just the name.

  2. maflcko commented at 9:16 pm on November 4, 2025: member

    Can be tested via the following diff:

     0diff --git a/src/logging.cpp b/src/logging.cpp
 1index 2ed6835197..890e461cb9 100644
 2--- a/src/logging.cpp
 3+++ b/src/logging.cpp
 4@@ -20,6 +20,7 @@
 5 using util::Join;
 6 using util::RemovePrefixView;
 7 
 8+const int g_FOO{Assert(false)};
 9 const char * const DEFAULT_DEBUGLOGFILE = "debug.log";
10 constexpr auto MAX_USER_SETABLE_SEVERITY_LEVEL{BCLog::Level::Info};
11

    Or with a function name (slight change in behavior):

     0diff --git a/src/logging.cpp b/src/logging.cpp
 1index 2ed6835197..761dbcbf58 100644
 2--- a/src/logging.cpp
 3+++ b/src/logging.cpp
 4@@ -20,6 +20,7 @@
 5 using util::Join;
 6 using util::RemovePrefixView;
 7 
 8+const int g_FOO{[]() { return Assert(false); }()};
 9 const char * const DEFAULT_DEBUGLOGFILE = "debug.log";
10 constexpr auto MAX_USER_SETABLE_SEVERITY_LEVEL{BCLog::Level::Info};
11
  3. maflcko force-pushed on Nov 4, 2025
  4. util: Allow Assert() in contexts without __func__ 
    Without this, compile warnings could be hit about __func__ being only
valid inside functions.

warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
note: expanded from macro Assert
  115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
      |                                                                           ^

Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
    fa5fbcd615
  5. refactor: Use const reference to std::source_location 
    Performance likely does not matter here, but from a perspective of
code-readablilty, a const reference should be preferred for read-only
access.

So use it here.

This requires to set -Wno-error=dangling-reference for GCC 13.1
compilations, but this false-positive is fixed in later GCC versions.

See also https://godbolt.org/z/fjc6be65M
    fae1d99651
  6. maflcko force-pushed on Nov 5, 2025
  7. stickies-v approved
  8. stickies-v commented at 4:13 pm on November 5, 2025: contributor

    ACK fae1d99651e29341e486a10e6340335c71a2144e

    Note that this also introduces behaviour change by including the entire function signature instead of just the name. Might be worth mentioning in OP.

  9. in ci/test/00_setup_env_win64.sh:1 in fae1d99651 outdated
    hodlinator commented at 9:39 am on November 6, 2025:
    remark: Disabling dangling reference warnings globally is not ideal, but chances that it will not be reverted as the container is upgraded seem slim.
    maflcko commented at 9:48 am on November 6, 2025:
    Correct, it is not ideal. Though, it is only disabled for two specific tasks and only temporary. Also, the GCC check is mostly or fully redundant anyway with the clang check (which works better due to lifetime annotations), as well as all the runtime sanitizers.
    hodlinator commented at 10:58 am on November 6, 2025:

    Tested running…

    0₿ env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh'

    …without the modification to the env shell scripts. Got the expected error:

     0C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
 1...
 2/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_method()’:
 3/ci_container_base/src/test/blockmanager_tests.cpp:63:17: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
 4   63 |     const auto& chainman = Assert(m_node.chainman);
 5      |                 ^~~~~~~~
 6In file included from /ci_container_base/src/streams.h:13,
 7                 from /ci_container_base/src/dbwrapper.h:11,
 8                 from /ci_container_base/src/node/blockstorage.h:10,
 9                 from /ci_container_base/src/test/blockmanager_tests.cpp:8:
10/ci_container_base/src/util/check.h:116:49: note: the temporary was destroyed at the end of the full expression ‘inline_assertion_check<true, std::unique_ptr<ChainstateManager>&>(((blockmanager_tests::blockmanager_scan_unlink_already_pruned_files*)this)->blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::<anonymous>.TestChain100Setup::<anonymous>.TestingSetup::<anonymous>.ChainTestingSetup::<anonymous>.BasicTestingSetup::m_node.node::NodeContext::chainman, std::source_location{(& *.Lsrc_loc27)}, std::basic_string_view<char>(((const char*)"m_node.chainman")))’
11  116 | #define Assert(val) inline_assertion_check<true>(val, std::source_location::current(), #val)
12      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13/ci_container_base/src/test/blockmanager_tests.cpp:63:28: note: in expansion of macro ‘Assert’
14   63 |     const auto& chainman = Assert(m_node.chainman);
15      |                            ^~~~~~
16cc1plus: all warnings being treated as errors
17gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
18gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:382: src/test/CMakeFiles/test_bitcoin.dir/blockmanager_tests.cpp.obj] Error 1
19gmake[1]: *** [CMakeFiles/Makefile2:1810: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
20gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
21gmake: *** [Makefile:146: all] Error 2

    Makes sense as this PR is doing, changing the warning to no longer be an error, but still output it as a reminder, as is already done for -Wno-error=maybe-uninitialized.

    l0rinc commented at 11:37 am on November 6, 2025:

    I’m also not a fan of globally disabling these, they could hide real problems.

    Doing a git grep -n "const auto&.*Assert(" shows there are only a few of these

     0src/index/coinstatsindex.cpp:204:                const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
 1src/init.cpp:2006:        const auto& tip{*Assert(chainman.ActiveTip())};
 2src/node/miner.cpp:304:    const auto& mempool{*Assert(m_mempool)};
 3src/policy/rbf.cpp:41:    const auto& entry{*Assert(pool.GetEntry(tx.GetHash()))};
 4src/test/blockmanager_tests.cpp:63:    const auto& chainman = Assert(m_node.chainman);
 5src/test/fuzz/package_eval.cpp:174:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
 6src/test/miniminer_tests.cpp:182:        const auto& entry{*Assert(pool.GetEntry(tx->GetHash()))};
 7src/test/miniminer_tests.cpp:450:    const auto& tx3_entry{*Assert(pool.GetEntry(tx3->GetHash()))};
 8src/test/miniminer_tests.cpp:454:    const auto& tx6_entry{*Assert(pool.GetEntry(tx6->GetHash()))};
 9src/test/miniminer_tests.cpp:457:    const auto& tx7_entry{*Assert(pool.GetEntry(tx7->GetHash()))};
10src/test/util/txmempool.cpp:149:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
11src/test/util/txmempool.cpp:186:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
12src/validation.cpp:1756:            const auto& entry{*Assert(m_pool.GetEntry(txid))};
13src/validation.cpp:1766:            const auto& entry{*Assert(m_pool.GetEntry(txid))};

    But most of them seem to be harmless pointer dereferences, only the mentioned one seems to have the problematic forwarding reference. I haven’t checked, but is there a simpler way to resolve the false positive without the global suppression, maybe by just removing the const?

    0auto& chainman = Assert(m_node.chainman);
    maflcko commented at 1:20 pm on November 6, 2025:
    I think a temporary workaround, local to two CI tasks, is fine. I don’t think there is value in doing more here. See #33785 (review). Leaving as-is for now.
    stickies-v commented at 5:21 pm on November 6, 2025:
    There are 2 places in validation.cpp where STR_INTERNAL_BUG could be used too, but I suppose you didn’t because it (afaik) requires us to abandon the compile time format string checking?
    maflcko commented at 7:18 pm on November 6, 2025:

    There are 2 places in validation.cpp where STR_INTERNAL_BUG could be used too, but I suppose you didn’t because it (afaik) requires us to abandon the compile time format string checking?

    I think all format strings should be checked at compile time, even with STR_INTERNAL_BUG. I think STR_INTERNAL_BUG can’t be used in the validation places, because STR_INTERNAL_BUG includes raw newlines and those are discouraged from logging. Though, it only happens on internal bugs anyway, so I can also see using STR_INTERNAL_BUG there and just dealing with the raw newlines in the debug log output.

    l0rinc commented at 10:18 am on November 10, 2025:
    Bumped into this again - if we remove the const from https://godbolt.org/z/fjc6be65M (int& ref{Assert(a)};) that seems to also fix the problem - why is that worse than introducing global suppression?
    maflcko commented at 10:24 am on November 10, 2025:

    Removing const over a bunch of places, including consensus code, seems like the wrong fix for a short-term temporary workaround for a long-fixed single compiler bug that affects a single version.

    The suppression is not global, all other CI or local runs will still have the warning enabled.

    Also, the temporary workaround can trivially and quickly removed in #33775, which is needed anyway for other reasons.

    l0rinc commented at 10:30 am on November 10, 2025:

    Removing const over a bunch of places

    k, I had the impression this was the only one 👍

    fanquake commented at 1:23 pm on November 10, 2025:
    Rebased #33775 on this, and dropped the workarounds back out.
    maflcko commented at 2:11 pm on November 10, 2025:

    k, I had the impression this was the only one 👍

    Oh, I think I misunderstood the previous comments. Clearly a test-only workaround to a single file is better than a ci-wide workaround to all files.

    The test code is “wrong” anyway. Using a pointer here makes little sense, when a reference should be used.

  10. hodlinator approved
  11. hodlinator commented at 9:40 am on November 6, 2025: contributor

    ACK fae1d99651e29341e486a10e6340335c71a2144e

    Tested inserting CHECK_NONFATAL to see how output differed

    Was concerned that std::source_location::current() inside of macros might behave slightly differently than __func__ etc. But only expected differences where observed.

    0--- a/src/script/miniscript.cpp
1+++ b/src/script/miniscript.cpp
2@@ -17,6 +17,7 @@ namespace miniscript {
3 namespace internal {
4 
5 Type SanitizeType(Type e) {
6+    CHECK_NONFATAL(false);
7     int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst);
8     if (num_types == 0) return ""_mst; // No valid type, don't care about the rest
9     CHECK_NONFATAL(num_types == 1); // K, V, B, W all conflict with each other
    Before
    0₿ build/bin/bitcoin-cli -regtest deriveaddresses "wsh(1)#mrg7xj7p"
1error code: -1
2error message:
3Internal bug detected: false
4../src/script/miniscript.cpp:20 (SanitizeType)
5...
    After
    0₿ build/bin/bitcoin-cli -regtest deriveaddresses "wsh(1)#mrg7xj7p"
1error code: -1
2error message:
3Internal bug detected: false
4../src/script/miniscript.cpp:20 (miniscript::Type miniscript::internal::SanitizeType(miniscript::Type))
5...
  12. in src/util/check.h:133 in fae1d99651 
    129+#define Assume(val) inline_assertion_check<false>(val, std::source_location::current(), #val)
130 
131 /**
132  * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.
133  */
134 #define NONFATAL_UNREACHABLE()                                        \
    l0rinc commented at 11:04 am on November 6, 2025:
    nit: can we align the formatting - either in column or at the end of each line, but this in-between looks weird
    maflcko commented at 1:21 pm on November 6, 2025:
    thx, ran clang-format while changing the {} initialization.
  13. in src/util/check.h:96 in fae1d99651 
    95@@ -95,7 +96,7 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
96 // All macros may use __func__ inside a lambda, so put them under nolint.
    l0rinc commented at 11:12 am on November 6, 2025:
    is the comment still accurate now that there’s no __func__ here anymore? And do we need it in the first place, given that it seems to have been introduced because of it?
    maflcko commented at 1:22 pm on November 6, 2025:
    thx, removed redundant and fixed NOLINT
  14. in src/util/check.h:112 in fae1d99651 outdated 
    109@@ -109,10 +110,10 @@ constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] con
110  * caller, which can then report the issue to the developers.
111  */
112 #define CHECK_NONFATAL(condition) \
    l0rinc commented at 11:14 am on November 6, 2025:

    might be unrelated, but we have remaining __FILE__, __LINE__, __func__ combos in one more file: https://github.com/bitcoin/bitcoin/blob/1a1f46c2285994908df9c11991c1f363c9733087/src/rest.cpp#L91-L94

    Could we migrate that as well to something like:

    0StrFormatInternalBug("Node context not found!", 
1                    std::source_location::current()));
    maflcko commented at 1:20 pm on November 6, 2025:
    Thx, done for all in a new, unrelated, commit.
  15. in src/util/check.h:137 in fae1d99651 
    134 #define NONFATAL_UNREACHABLE()                                        \
135     throw NonFatalCheckError(                                         \
136-        "Unreachable code reached (non-fatal)", __FILE__, __LINE__, __func__)
137+        "Unreachable code reached (non-fatal)", std::source_location::current())
138 
139 // NOLINTEND(bugprone-lambda-function-name)
    l0rinc commented at 11:15 am on November 6, 2025:
    Same, we don’t have a function name here anymore, do we still need the linter suppression?
  16. in src/util/check.cpp:39 in fae1d99651 outdated 
    41     if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) {
42-        throw NonFatalCheckError{assertion, file, line, func};
43+        throw NonFatalCheckError{assertion, loc};
44     }
45-    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
46+    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
    l0rinc commented at 11:20 am on November 6, 2025:

    nit: we should probably use %d for the line number instead of %s

    0    auto str = strprintf("%s:%d %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
    maflcko commented at 1:20 pm on November 6, 2025:

    My preference is to just use %s for all args, unless there is a reason not to. So I’ll leave this as-is for now.

    Now that the libc++ minimum version is 17, which includes std::format, in about 12 months, we can probably bump GCC to 13 as well. Then {} can be used for all args, unless there is a reason not to.

  17. l0rinc changes_requested
  18. l0rinc commented at 11:41 am on November 6, 2025: contributor

    Concept ACK

    It seems to me we should do a deeper cleanup now that __func__ isn’t used, apply it to a few more cases and not use a global suppression but adjust the call-sites instead.

    nit: it seems to me it’s not just Assert that’s affected (as the PR title claims), but Assume as well.

  19. doc: Remove unused bugprone-lambda-function-name suppression 
    Now that the __func__ is no longer used, the
NOLINTBEGIN(bugprone-lambda-function-name) can be removed.

Also, re-format the NONFATAL_UNREACHABLE macro, while touching the
adjacent line.
    fada379589
  20. refactor: Use STR_INTERNAL_BUG macro where possible 
    This ensures a uniform bug template and allows to drop includes and
logic at the call sites.
    fad6efd3be
  21. maflcko renamed this:
    util: Allow Assert() in contexts without __func__
    util: Allow `Assert` (et al.) in contexts without __func__
    on Nov 6, 2025
  22. l0rinc approved
  23. l0rinc commented at 3:06 pm on November 6, 2025: contributor

    Code review ACK fad6efd3bef1d123f806d492f019e29530b03a5e

    The only nit I still have is the global suppression, but we can also just tackle that in a follow-up if needed.

  24. stickies-v approved
  25. stickies-v commented at 5:21 pm on November 6, 2025: contributor
    ACK fad6efd3bef1d123f806d492f019e29530b03a5e
  26. in src/util/check.h:131 in fad6efd3be 
    132-    throw NonFatalCheckError(                                         \
133-        "Unreachable code reached (non-fatal)", __FILE__, __LINE__, __func__)
134-
135-// NOLINTEND(bugprone-lambda-function-name)
136+#define NONFATAL_UNREACHABLE() \
137+    throw NonFatalCheckError { "Unreachable code reached (non-fatal)", std::source_location::current() }
    hodlinator commented at 6:59 pm on November 6, 2025:

    nit: This is a case of clang-format failing to understand what is happening inside a macro.

    https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle

    https://github.com/bitcoin/bitcoin/blob/fada379589a17e86396aa7c2ce458ff2ff602b84/src/.clang-format#L87

    If I in the same commit change this to…

    0    throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}

    …and also add…

    0void Func_NONFATAL_UNREACHABLE()
1{
2    throw NonFatalCheckError { "Unreachable code reached (non-fatal)", std::source_location::current() };
3}

    Then run this afterwards…

    0₿ git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

    …it results in:

    0 #define NONFATAL_UNREACHABLE() \
1-    throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
2+    throw NonFatalCheckError { "Unreachable code reached (non-fatal)", std::source_location::current() }
3
4 void Func_NONFATAL_UNREACHABLE()
5 {
6-    throw NonFatalCheckError { "Unreachable code reached (non-fatal)", std::source_location::current() };
7+    throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()};
8 }

    Proving what clang-format is aiming to do when it better understands the context. (I’m running 19.1.7 which seems to have the same behavior as PR author’s).

    maflcko commented at 9:37 am on November 10, 2025:

    Just for ref, one could write this to please clang-format, but not sure if it is worth it:

    0#define NONFATAL_UNREACHABLE()                             \
1    do {                                                   \
2        throw NonFatalCheckError{                          \
3            "Unreachable code reached (non-fatal)",        \
4            std::source_location::current(),               \
5        };                                                 \
6    } while (0)
  27. hodlinator approved
  28. hodlinator commented at 7:35 pm on November 6, 2025: contributor

    re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e

    2 added commits since previous review (https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513):

    • Remove no longer needed disabling of linter according to CI. 👍 (Re-touching NONFATAL_UNREACHABLE but no big deal).
    • De-duplicate code by using STR_INTERNAL_BUG().
  29. maflcko commented at 8:45 am on November 10, 2025: member
    Is this rfm with 3 acks?
  30. fanquake merged this on Nov 10, 2025
  31. fanquake closed this on Nov 10, 2025
  32. maflcko deleted the branch on Nov 10, 2025


maflcko stickies-v hodlinator l0rinc fanquake

Labels
Utils/log/libs

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