Warn on and add missing [[noreturn]] #35911

fanquake commented at 9:34 AM on August 6, 2026: member

Missing [[noreturn]] attributes can cause compiler false positives and other issues; see discussion in #35896 (review). Add missing attributes, and turn on compiler warnings.

This produces C++23 related warnings under Clang:

[819/1115] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/threadpool_tests.cpp.o
../src/test/threadpool_tests.cpp:212:64: warning: an attribute specifier sequence in this position is a C++23 extension [-Wc++23-lambda-attributes]
  212 |         futures.emplace_back(Submit(threadPool, [&make_err, i] [[noreturn]] () {
      |                                                                ^
1 warning generated.

DrahtBot commented at 9:34 AM on August 6, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35911.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	hebasto, stickies-v

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/762 (Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog by pablomartin4btc)
#36087 (util: Add and use AssertUnreachable by maflcko)
#34132 (coins,dbwrapper: remove error catcher, make point-read failures fatal by l0rinc)
#32387 (ipc: add windows support by ryanofsky)
#31507 (build: Use clang-cl to build on Windows natively by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/test/util/setup_common.cpp:139 in 78f74b1a29 outdated

 132 | @@ -133,7 +133,7 @@ void SetupCommonTestArgs(ArgsManager& argsman)
 133 |  }
 134 |  
 135 |  /** Test setup failure */
 136 | -static void ExitFailure(std::string_view str_err)
 137 | +[[noreturn]] static void ExitFailure(std::string_view str_err)
 138 |  {
 139 |      std::cerr << str_err << std::endl;
 140 |      exit(EXIT_FAILURE);

maflcko commented at 9:52 AM on August 6, 2026:

nit: Should probably use std::exit https://en.cppreference.com/cpp/utility/program/exit

to avoid confusing with the one from C23?

But either is fine and I don't think this matters.

fanquake commented at 10:38 AM on August 6, 2026:

Added a commit for this.

in src/test/threadpool_tests.cpp:212 in 78f74b1a29 outdated

 208 | @@ -209,7 +209,7 @@ BOOST_AUTO_TEST_CASE(task_exception_propagates_to_future)
 209 |      std::vector<std::future<void>> futures;
 210 |      futures.reserve(num_tasks);
 211 |      for (int i = 0; i < num_tasks; i++) {
 212 | -        futures.emplace_back(Submit(threadPool, [&make_err, i] { throw std::runtime_error(make_err(i)); }));
 213 | +        futures.emplace_back(Submit(threadPool, [&make_err, i]() __attribute__((noreturn)) { throw std::runtime_error(make_err(i)); }));

maflcko commented at 9:53 AM on August 6, 2026:

nit: Can probably split/clang-format this long line into 3 or so?

        futures.emplace_back(Submit(threadPool, [&make_err, i]() __attribute__((noreturn)) {
          throw std::runtime_error(make_err(i));
           }));

fanquake commented at 4:36 PM on August 6, 2026:

Split this up.

in src/util/subprocess.h:751 in 78f74b1a29 outdated

 747 | @@ -748,7 +748,7 @@ class Child
 748 |      err_wr_pipe_(err_wr_pipe)
 749 |    {}
 750 |  
 751 | -  void execute_child();
 752 | +  [[noreturn]] void execute_child();

maflcko commented at 9:53 AM on August 6, 2026:

Could also add upstream, but doesn't matter for this pull.

fanquake commented at 10:06 AM on August 6, 2026:

Opened https://github.com/arun11299/cpp-subprocess/pull/132.

maflcko commented at 9:59 AM on August 6, 2026: member

Nice. lgtm. Left some style nits, but feel free to ignore.

../src/test/fuzz/fuzz.cpp:239:26: warning: code will never be executed [-Wunreachable-code]

This looks like a compiler bug?

DrahtBot added the label CI failed on Aug 6, 2026

DrahtBot commented at 10:05 AM on August 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/31089666107/job/92577355450 LLM reason (✨ experimental): CI failed at build time because clang with -Werror rejected fuzz test code in src/test/fuzz/fuzz.cpp (invalid noreturn function return and unreachable code).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in src/test/fuzz/fuzz.cpp:93 in 78f74b1a29

  89 | @@ -90,7 +90,7 @@ const std::function<std::string()> G_TEST_GET_FULL_NAME{[]{
  90 |      return std::string{g_fuzz_target};
  91 |  }};
  92 |  
  93 | -static void initialize()
  94 | +[[noreturn]] static void initialize()

maflcko commented at 10:10 AM on August 6, 2026:

Oh, I missed: This one is wrong:

cd /home/runner/work/bitcoin/bitcoin/ci_build/src/test/fuzz/util && /usr/bin/ccache /usr/bin/clang++ -DABORT_ON_FAILED_ASSUME -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DDEBUG -DDEBUG_LOCKCONTENTION -DDEBUG_LOCKORDER -DPROVIDE_FUZZ_MAIN_FUNCTION -DRPC_DOC_CHECK -I/home/runner/work/bitcoin/bitcoin/ci_build/src -I/home/runner/work/bitcoin/bitcoin/src -I/home/runner/work/bitcoin/bitcoin/src/univalue/include -Wno-error=unused-member-function -Wno-error=unused-function -O0 -ftrapv -g3 -std=c++20 -fPIC -fmacro-prefix-map=/home/runner/work/bitcoin/bitcoin/src=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wcovered-switch-default -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -Werror -MD -MT src/test/fuzz/util/CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o -MF CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o.d -o CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o -c /home/runner/work/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp  -O3 -g2
/home/runner/work/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:168:1: error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
  168 | }
      | ^
/home/runner/work/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:239:26: error: code will never be executed [-Werror,-Wunreachable-code]
  239 |     std::vector<uint8_t> buffer;
      |                          ^~~~~~
2 errors generated.

maflcko commented at 12:41 PM on August 6, 2026:

Mabye a bug in clang: https://github.com/bitcoin/bitcoin/actions/runs/31093885302/job/92591251807?pr=35911#step:11:3252:

cd "/home/runner/work/_temp/build_ ₿🧪_/src/test/fuzz/util" && /usr/bin/ccache /usr/bin/clang++ -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DPROVIDE_FUZZ_MAIN_FUNCTION -I"/home/runner/work/_temp/build_ ₿🧪_/src" -I/home/runner/work/_temp/src -I/home/runner/work/_temp/src/univalue/include -ftrivial-auto-var-init=pattern -O2 -g -std=c++20 -fPIC -fmacro-prefix-map=/home/runner/work/_temp/src=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -fsanitize=address,float-divide-by-zero,integer,undefined -Wall -Wextra -Wgnu -Wcovered-switch-default -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wthread-safety-pointer -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wmissing-noreturn -Wno-unused-parameter -Werror -MD -MT src/test/fuzz/util/CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o -MF CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o.d -o CMakeFiles/test_fuzz.dir/__/fuzz.cpp.o -c /home/runner/work/_temp/src/test/fuzz/fuzz.cpp -DARENA_DEBUG -DDEBUG_LOCKORDER -std=c++23
/home/runner/work/_temp/src/test/fuzz/fuzz.cpp:94:1: error: function 'initialize' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
   94 | {
      | ^
1 error generated.

fanquake force-pushed on Aug 6, 2026

maflcko removed the label CI failed on Aug 6, 2026

fanquake commented at 10:34 AM on August 6, 2026: member

Opened https://github.com/bitcoin-core/leveldb-subtree/pull/64 for the leveldb changes.

DrahtBot added the label CI failed on Aug 6, 2026

DrahtBot commented at 10:36 AM on August 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/31093313597/job/92589235946 LLM reason (✨ experimental): CI failed because the lint check subtree detected that a subtree directory was modified without the required subtree merge.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in CMakeLists.txt:505 in 906dd19986 outdated

 501 | @@ -502,6 +502,7 @@ else()
 502 |    try_append_cxx_flags("-Wundef" TARGET warn_interface SKIP_LINK)
 503 |    try_append_cxx_flags("-Wleading-whitespace=spaces" TARGET warn_interface SKIP_LINK)
 504 |    try_append_cxx_flags("-Wtrailing-whitespace=any" TARGET warn_interface SKIP_LINK)
 505 | +  try_append_cxx_flags("-Wmissing-noreturn" TARGET warn_interface SKIP_LINK)

Sjors commented at 11:57 AM on August 6, 2026:

Documentation says:

Note these are only possible candidates, not absolute ones.

So this might give us false positives, but those could be suppressed?

https://gcc.gnu.org/onlinedocs/gcc-4.2.2/gcc/Warning-Options.html

fanquake commented at 12:39 PM on August 6, 2026:

https://gcc.gnu.org/onlinedocs/gcc-4.2.2/gcc/Warning-Options.html

Your documentation is about 20 years out of date. The modern docs say:

Warn about functions that might be candidates for attributes pure, const, noreturn, malloc or returns_nonnull. The compiler only warns for functions visible in other compilation units or (in the case of pure and const) if it cannot prove that the function returns normally. A function returns normally if it doesn’t contain an infinite loop or return abnormally by throwing, calling abort or trapping. This analysis requires option -fipa-pure-const, which is enabled by default at -O and higher. Higher optimization levels improve the accuracy of the analysis.

hebasto commented at 3:39 PM on August 6, 2026: member

Concept ACK.

hebasto commented at 4:26 PM on August 6, 2026: member

Could you submit commit 2b6dee1f51651ecf7245552b467417cc72a28fe3 in a separate PR? That way, we can review and land it sooner to unbreak the nightly CI.

fanquake commented at 4:28 PM on August 6, 2026: member

Could you submit commit https://github.com/bitcoin/bitcoin/commit/2b6dee1f51651ecf7245552b467417cc72a28fe3 in a separate PR?

It is part of #35896.

fanquake force-pushed on Aug 6, 2026

fanquake force-pushed on Aug 7, 2026

maflcko removed the label CI failed on Aug 7, 2026

DrahtBot added the label CI failed on Aug 7, 2026

DrahtBot commented at 10:04 AM on August 7, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/31164196113/job/92821231499 LLM reason (✨ experimental): CI failed because the linter’s “subtree” check detected modified subtree directories without a corresponding subtree merge.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

fanquake force-pushed on Aug 7, 2026

fanquake force-pushed on Aug 10, 2026

stickies-v commented at 10:30 AM on August 11, 2026: contributor

Concept ACK

fanquake referenced this in commit 13da2d6758 on Aug 11, 2026

sedited referenced this in commit c1967c4453 on Aug 11, 2026

fanquake referenced this in commit 636aaff576 on Aug 12, 2026

ryanofsky referenced this in commit bdd0cd6941 on Aug 12, 2026

fanquake force-pushed on Aug 12, 2026

fanquake referenced this in commit 0274dd3b6a on Aug 12, 2026

fanquake force-pushed on Aug 12, 2026

fanquake referenced this in commit 36d172695f on Aug 12, 2026

fanquake referenced this in commit b0399cdb70 on Aug 12, 2026

fanquake referenced this in commit 8b5da677d7 on Aug 14, 2026

fanquake referenced this in commit e95bab98f0 on Aug 14, 2026

fanquake force-pushed on Aug 14, 2026

[nomerge] build: add -Wno-c++23-lambda-attributes 84c6e46ee0

[nomerge] libmultiprocess: missing [[noreturn]] 9a621498ca

refactor: add missing [[noreturn]] attributes a739c804bd

refactor: pragma around build-dependant missing noreturn 8a6020521f

build: add -Wmissing-noreturn to warning flags e739c984e6

refactor: use C++ std::exit over exit() afff3e70d7

fanquake force-pushed on Aug 18, 2026

fanquake commented at 10:13 AM on August 18, 2026: member

Not sure it's worth trying to pull any changes out here, so this will remain drafted until the next libmultiprocess subtree update.

xyzconstant referenced this in commit 8c84be5cc8 on Aug 27, 2026

in src/test/fuzz/util/net.h:195 in afff3e70d7

 191 | @@ -192,7 +192,7 @@ class FuzzedSock : public Sock
 192 |  
 193 |      ~FuzzedSock() override;
 194 |  
 195 | -    FuzzedSock& operator=(Sock&& other) override;
 196 | +    [[noreturn]] FuzzedSock& operator=(Sock&& other) override;

maflcko commented at 8:41 AM on August 31, 2026:

Looks like this still fails to compile on Windows for both clang-cl and MSVC? IIRC I tried this last week on godbolt and for some reason inlining the impl into the header worked around the bug.