build: enable `-Wunreachable-code-aggressive` #36623

fanquake commented at 2:12 PM on October 23, 2026: member

Swap -Wunreachable-code for -Wunreachable-code-aggressive, which also enables -Wunreachable-code-return and -Wunreachable-code-break.

DrahtBot added the label Build system on Oct 23, 2026

DrahtBot commented at 2:12 PM on October 23, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36623
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36623.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36623#author-ralph

Reviews

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

Type	Reviewers
ACK	willcl-ark, sedited, hodlinator, maflcko

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:

#31507 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/31507.svg"> (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/script/interpreter.cpp:685 in 8f8ea0b2af outdated

 677 | @@ -678,7 +678,14 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
 678 |                  {
 679 |                      return set_error(serror, SCRIPT_ERR_OP_RETURN);
 680 |                  }
 681 | +#if defined(__clang__)
 682 | +#pragma clang diagnostic push
 683 | +#pragma clang diagnostic ignored "-Wunreachable-code-break"
 684 | +#endif
 685 |                  break;

willcl-ark commented at 2:57 PM on October 23, 2026:

It does appear that this is likely removable too.

fanquake commented at 3:36 PM on October 30, 2026:

Happy to do what reviewers prefer here.

hodlinator commented at 9:08 AM on October 31, 2026:

FWIW the least worst solution to me seems to be removing the break together with the braces, so this case gets formatting like the default at the end: https://github.com/bitcoin/bitcoin/blob/15ba42294eb57bd4850da0a830741c35aa8b46cc/src/script/interpreter.cpp#L1227-L1228

--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -675,18 +675,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                 break;
 
                 case OP_RETURN:
-                {
                     return set_error(serror, SCRIPT_ERR_OP_RETURN);
-                }
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wunreachable-code-break"
-#endif
-                break;
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
-
 
                 //
                 // Stack ops

willcl-ark commented at 10:04 AM on November 6, 2026:

OK I pointed Astra at this to try and get more guarantees around removal...

It compiled the interpreter translation unit before and after removing this break, using GCC 15.3.0 and Clang 19.1.7 at both -O0 and -O2 (as we use in Guix builds), with assertions enabled.

All four pairs produced byte-identical object files when preserving the newline. Deleting the whole line changed only assertion diagnostic line numbers.

Clanker's full report.

This further supports removing the unreachable break IMO.

fanquake commented at 10:18 AM on November 6, 2026:

I might swap to that once rebased for #36783.

fanquake commented at 11:28 AM on November 6, 2026:

Now that I've taken the diff, I'm going to mark this as resolved, so review can restart in a clean thread.

DrahtBot added the label CI failed on Oct 23, 2026

DrahtBot commented at 3:20 PM on October 23, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/39967612158/job/120461497226 LLM reason (✨ experimental): CI failed the subtree lint check because the LevelDB and minisketch subtrees were modified without a 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 referenced this in commit 3c8ff755a3 on Oct 26, 2026

fanquake referenced this in commit 24a446f834 on Oct 26, 2026

hebasto referenced this in commit 15ba42294e on Oct 27, 2026

fanquake force-pushed on Oct 27, 2026

sipa referenced this in commit 75488667cd on Nov 3, 2026

sipa referenced this in commit d44dca4e3f on Nov 3, 2026

DrahtBot added the label Needs rebase on Nov 4, 2026

sedited referenced this in commit f12cc4aed1 on Nov 6, 2026

refactor: fix -Wunreachable-code-aggressive warnings 27da2de66f

build: swap -Wunreachable-code for aggressive variant

Enables `-Wunreachable-code-break` and `-Wunreachable-code-return`.

1f69c5cfdd

fanquake force-pushed on Nov 6, 2026

fanquake marked this as ready for review on Nov 6, 2026

DrahtBot removed the label Needs rebase on Nov 6, 2026

fanquake commented at 11:27 AM on November 6, 2026: member

Rebased past #36783 and took @hodlinator's diff.

willcl-ark approved

willcl-ark commented at 12:25 PM on November 6, 2026: member

ACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c

Reviewed the updated diff and had me clanker rerun GCC/Clang object comparisons at -O0 and -O2. The interpreter changes only assertion diagnostic line numbers, and preserving those gives byte-identical objects.

Also prefer hodlinator’s formatting in interpreter.cpp as the braces serve no scoping purpose and removing them makes the unconditional return clearer IMO.

DrahtBot removed the label CI failed on Nov 6, 2026

sedited approved

sedited commented at 12:44 PM on November 6, 2026: contributor

ACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c

sedited requested review from maflcko on Nov 6, 2026

hodlinator approved

hodlinator commented at 12:45 PM on November 6, 2026: contributor

crACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c

maflcko commented at 1:04 PM on November 6, 2026: member

lgtm. Should be fine for this codebase, as we already require "unreachable" code to be marked with an abort instead of UB.

review ACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c 🐟

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c 🐟
RfKjLyaCfQSFPVwu0TlGLLPf3EMPNdZy51M+nj4gPNvoeybfInb/vw1Hu6UbbM7kWvcAbc8ERY2+j1y1TsCwAQ==

</details>

sedited merged this on Nov 6, 2026

sedited closed this on Nov 6, 2026

fanquake deleted the branch on Nov 6, 2026

l0rinc commented at 5:09 PM on November 6, 2026: contributor

Post-merge ACK 1f69c5cfdd9a21e03bedcb4edd14c4fb13954b7c

Trivial change: applying the following whitespace-only patch gives byte-identical assembly with GCC/Clang - can't be simpler than this.

<details><summary>Patch</summary>

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -675,7 +675,11 @@
                 break;
 
                 case OP_RETURN:
+
                     return set_error(serror, SCRIPT_ERR_OP_RETURN);
+
+
+
 
                 //
                 // Stack ops

</details>