Warn on and add missing [[noreturn]] by fanquake · Pull Request #35911 · bitcoin/bitcoin

fanquake · 2026-08-06T09:34:14Z

Missing [[noreturn]] attributes can cause compiler false positives and other issues; see discussion in #35896 (comment). 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.

Related:
bitcoin-core/libmultiprocess#339.
bitcoin-core/leveldb-subtree#64.
arun11299/cpp-subprocess#132.

DrahtBot · 2026-08-06T09:34:21Z

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  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)
#36244 (validation, net: Process blocks asynchronously and reduce cs_main contention by w0xlt)
#36167 ([RFC] Enable -Wunused by fanquake)
#35932 (ipc: make ipc::disconnectIncoming wait for in-progress calls to complete by ryanofsky)
#35744 (coins: prevent DB resize from invalidating cursors by l0rinc)
#34132 (coins, dbwrapper: remove error catcher, make point-read failures fatal by l0rinc)
#32387 (ipc: add windows support by ryanofsky)
#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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.

maflcko

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 · 2026-08-06T10:05:41Z

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

Hints

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.

fanquake · 2026-08-06T10:34:13Z

Opened bitcoin-core/leveldb-subtree#64 for the leveldb changes.

DrahtBot · 2026-08-06T10:36:41Z

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

Hints

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.

hebasto

Concept ACK.

hebasto

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

fanquake · 2026-08-06T16:28:35Z

Could you submit commit 2b6dee1 in a separate PR?

It is part of #35896.

DrahtBot · 2026-08-07T10:04:12Z

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

Hints

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.

stickies-v · 2026-08-11T10:30:31Z

Concept ACK

I don't think there's a code path that can reach RunCommandParseJSON if we compile with `-DENABLE_EXTERNAL_SIGNER=OFF`. This also requires more workarounds in bitcoin#35911.

I don't think there's a code path that can reach RunCommandParseJSON if we compile with `-DENABLE_EXTERNAL_SIGNER=OFF`. This also requires more workarounds in bitcoin#35911. Co-authored-by: stickies-v <stickies-v@protonmail.com>

…SON` 8b5da67 common: remove ::runtime_error from RunCommandParseJSON (fanquake) Pull request description: I don't think there's a code path that can reach `RunCommandParseJSON` if we compile with `ENABLE_EXTERNAL_SIGNER=OFF`. If there is a reason for having the code this way, it could be good to document. This also requires more workarounds in #35911. ACKs for top commit: stickies-v: re-ACK 8b5da67 sedited: ACK 8b5da67 willcl-ark: ACK 8b5da67 Tree-SHA512: b0c50372fed35afe47713310851f0b58cd1803fbe87a3a5a75877772172a3881283394a91663251de22a9972f56b46d84ddc868686dec8b970474cfaf5dc0d32

fanquake · 2026-08-18T10:13:52Z

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

These will be used downstream, see bitcoin#35911.

maflcko · 2026-08-31T08:41:28Z

    ~FuzzedSock() override;

-    FuzzedSock& operator=(Sock&& other) override;
+ [[noreturn]] FuzzedSock& operator=(Sock&& other) override;


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.

fanquake mentioned this pull request Aug 6, 2026

refactor: Default uint256::operator==, add operator<=> #35896

Merged

maflcko reviewed Aug 6, 2026

View reviewed changes

Comment thread src/test/util/setup_common.cpp Outdated

Comment thread src/test/threadpool_tests.cpp Outdated

Comment thread src/util/subprocess.h

DrahtBot added the CI failed label Aug 6, 2026

maflcko reviewed Aug 6, 2026

View reviewed changes

Comment thread src/test/fuzz/fuzz.cpp Outdated

fanquake force-pushed the missing_noreturn branch from 78f74b1 to 906dd19 Compare August 6, 2026 10:27

fanquake mentioned this pull request Aug 6, 2026

refactor: add [[noreturn]] attribute to BackgroundThread* bitcoin-core/leveldb-subtree#64

Merged

maflcko removed the CI failed label Aug 6, 2026

DrahtBot added the CI failed label Aug 6, 2026

Sjors reviewed Aug 6, 2026

View reviewed changes

Comment thread CMakeLists.txt

hebasto reviewed Aug 6, 2026

View reviewed changes

fanquake force-pushed the missing_noreturn branch from 411f42b to d713ecf Compare August 6, 2026 17:06

This was referenced Aug 7, 2026

Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog bitcoin-core/gui#762

Open

cmake: Add -Wunused-template to warn_interface #35905

Closed

fanquake force-pushed the missing_noreturn branch 2 times, most recently from a8321bc to 8d88646 Compare August 7, 2026 09:02

maflcko removed the CI failed label Aug 7, 2026

DrahtBot added the CI failed label Aug 7, 2026

This was referenced Aug 7, 2026

coins, dbwrapper: remove error catcher, make point-read failures fatal #34132

Open

build: Use clang-cl to build on Windows natively #31507

Open

fanquake force-pushed the missing_noreturn branch 2 times, most recently from 16166b9 to ab78cb5 Compare August 10, 2026 11:11

fanquake mentioned this pull request Aug 10, 2026

refactor: add [[noreturn]] attributes bitcoin-core/libmultiprocess#339

Merged

fanquake force-pushed the missing_noreturn branch from f4481b5 to 08cd0fd Compare August 12, 2026 16:25

fanquake mentioned this pull request Aug 12, 2026

common: remove ::runtime_error from RunCommandParseJSON #35960

Merged

fanquake force-pushed the missing_noreturn branch from 08cd0fd to 27c66eb Compare August 14, 2026 15:21

This was referenced Aug 14, 2026

ipc: add windows support #32387

Draft

Multiprocess bitcoin #10102

Draft

fanquake force-pushed the missing_noreturn branch from 27c66eb to afff3e7 Compare August 18, 2026 10:13

xyzconstant pushed a commit to xyzconstant/bitcoin that referenced this pull request Aug 27, 2026

refactor: add missing [[noreturn]] attributes

8c84be5

These will be used downstream, see bitcoin#35911.

maflcko reviewed Aug 31, 2026

View reviewed changes

DrahtBot mentioned this pull request Sep 5, 2026

[RFC] Enable -Wunused #36167

Draft

fanquake force-pushed the missing_noreturn branch from afff3e7 to 21fc8ad Compare September 9, 2026 11:27

fanquake added 5 commits September 29, 2026 17:27

[nomerge] libmultiprocess: missing [[noreturn]]

ee3cbb4

refactor: add missing [[noreturn]] attributes

d25cc88

refactor: pragma around build-dependant missing noreturn

f59fa11

build: add -Wmissing-noreturn to warning flags

934c041

refactor: use C++ std::abort/exit over abort/exit()

f607752

fanquake force-pushed the missing_noreturn branch from 21fc8ad to f607752 Compare September 29, 2026 16:28

Conversation

fanquake commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Aug 6, 2026

Uh oh!

Uh oh!

fanquake commented Aug 6, 2026

Uh oh!

DrahtBot commented Aug 6, 2026

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 6, 2026

Uh oh!

DrahtBot commented Aug 7, 2026

Uh oh!

stickies-v commented Aug 11, 2026

Uh oh!

fanquake commented Aug 18, 2026

Uh oh!

maflcko Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fanquake commented Aug 6, 2026 •

edited

Loading

DrahtBot commented Aug 6, 2026 •

edited

Loading