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

remark: Disabling dangling reference warnings globally is not ideal, but chances that it will not be reverted as the container is upgraded seem slim.

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.

Tested running...

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

C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
...
/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_method()’:
/ci_container_base/src/test/blockmanager_tests.cpp:63:17: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
   63 |     const auto& chainman = Assert(m_node.chainman);
      |                 ^~~~~~~~
In file included from /ci_container_base/src/streams.h:13,
                 from /ci_container_base/src/dbwrapper.h:11,
                 from /ci_container_base/src/node/blockstorage.h:10,
                 from /ci_container_base/src/test/blockmanager_tests.cpp:8:
/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")))’
  116 | #define Assert(val) inline_assertion_check<true>(val, std::source_location::current(), #val)
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/src/test/blockmanager_tests.cpp:63:28: note: in expansion of macro ‘Assert’
   63 |     const auto& chainman = Assert(m_node.chainman);
      |                            ^~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:382: src/test/CMakeFiles/test_bitcoin.dir/blockmanager_tests.cpp.obj] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1810: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake: *** [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.

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

src/index/coinstatsindex.cpp:204:                const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
src/init.cpp:2006:        const auto& tip{*Assert(chainman.ActiveTip())};
src/node/miner.cpp:304:    const auto& mempool{*Assert(m_mempool)};
src/policy/rbf.cpp:41:    const auto& entry{*Assert(pool.GetEntry(tx.GetHash()))};
src/test/blockmanager_tests.cpp:63:    const auto& chainman = Assert(m_node.chainman);
src/test/fuzz/package_eval.cpp:174:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
src/test/miniminer_tests.cpp:182:        const auto& entry{*Assert(pool.GetEntry(tx->GetHash()))};
src/test/miniminer_tests.cpp:450:    const auto& tx3_entry{*Assert(pool.GetEntry(tx3->GetHash()))};
src/test/miniminer_tests.cpp:454:    const auto& tx6_entry{*Assert(pool.GetEntry(tx6->GetHash()))};
src/test/miniminer_tests.cpp:457:    const auto& tx7_entry{*Assert(pool.GetEntry(tx7->GetHash()))};
src/test/util/txmempool.cpp:149:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
src/test/util/txmempool.cpp:186:        const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash()));
src/validation.cpp:1756:            const auto& entry{*Assert(m_pool.GetEntry(txid))};
src/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?

auto& chainman = Assert(m_node.chainman);

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.

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?

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.

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?

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.

Removing const over a bunch of places

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

Rebased #33775 on this, and dropped the workarounds back out.

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.

nit: can we align the formatting - either in column or at the end of each line, but this in-between looks weird

thx, ran clang-format while changing the {} initialization.

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?

thx, removed redundant and fixed NOLINT

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:

StrFormatInternalBug("Node context not found!", 
                    std::source_location::current()));

Thx, done for all in a new, unrelated, commit.

Same, we don't have a function name here anymore, do we still need the linter suppression?

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

    auto str = strprintf("%s:%d %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);

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.

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...

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

...and also add...

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

Then run this afterwards...

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

...it results in:

 #define NONFATAL_UNREACHABLE() \
-    throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
+    throw NonFatalCheckError { "Unreachable code reached (non-fatal)", std::source_location::current() }

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

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).

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

#define NONFATAL_UNREACHABLE()                             \
    do {                                                   \
        throw NonFatalCheckError{                          \
            "Unreachable code reached (non-fatal)",        \
            std::source_location::current(),               \
        };                                                 \
    } while (0)