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

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

It looks like missing [[noreturn]] attributes can cause compiler false positives/other issues; see discussion in #35896 (review). It could be worthwhile warning when they are missing, so they are added.

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)
#35905 (cmake: Add -Wunused-template to warn_interface by hebasto)
#34132 (coins,dbwrapper: remove error catcher, make point-read failures fatal by l0rinc)
#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

[nomerge] leveldb: missing [[noreturn]] 09ed0aa531

[nomerge] libmultiprocess: missing [[noreturn]] 43b070715e

refactor: add missing [[noreturn]] attributes 8650a2ffca

refactor: pragma around build-dependant missing noreturn f79b057fc9

build: add -Wmissing-noreturn to warning flags bb74482dcd

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

fanquake force-pushed on Aug 10, 2026

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

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

Concept ACK