build: Enable -Wunreachable-code #28999

maflcko · 2023-12-05T14:38:19Z

It seems a bit confusing to write code after a return. This can even lead to bugs, or incorrect code, such as https://github.com/bitcoin/bitcoin/pull/28830/files#r1415372320 . (Edit: The linked instance is not found by clang's -Wunreachable-code).

Fix all issues by enabling -Wunreachable-code.

This flag also enables -Wunreachable-code-loop-increment, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code, so remove that.

DrahtBot · 2023-12-05T14:38:22Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, jonatack
Concept ACK	fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
#25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)

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.

jonatack · 2023-12-05T15:02:52Z

Concept ACK

Sjors · 2023-12-05T15:42:33Z

src/wallet/walletdb.cpp

@@ -1499,17 +1499,19 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
    if (format == DatabaseFormat::SQLITE) {
 #ifdef USE_SQLITE
        return MakeSQLiteDatabase(path, options, status, error);
-#endif
+#else


cc @achow101

This doesn't functionally change anything, it just removes dead code if there's an earlier return statement.

@Sjors The previous code was correct as well. However, this refactoring change is required if the warning is enabled.

stickies-v

ACK fa8adbe

At least on my machine, this does't catch #28830 (comment) though. I tried changing the Assert (and inline_assertion_check) definition/implementation but can't seem to get that to work. Would be pretty helpful to fix, but even without it, this is still an improvement?

stickies-v · 2023-12-05T15:45:12Z

src/wallet/walletdb.cpp

@@ -1499,17 +1499,19 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
    if (format == DatabaseFormat::SQLITE) {
 #ifdef USE_SQLITE
        return MakeSQLiteDatabase(path, options, status, error);
-#endif
+#else


This doesn't functionally change anything, it just removes dead code if there's an earlier return statement.

maflcko · 2023-12-05T15:51:19Z

At least on my machine, this does't catch

You'll have to use clang, because GCC does not have those warnings.

stickies-v · 2023-12-05T15:54:35Z

You'll have to use clang, because GCC does not have those warnings.

I'm using clang, and it also caught the walletdb issue you fixed. It also catches e.g. error.reset() dead code I added. Just not the Assert.

    // ...and construct a CTxMemPool from it
    std::optional<bilingual_str> error{};
    return CTxMemPool{mempool_opts, error};
    error.reset();  // just to add unreachable code
    Assert(!error);

% make
Making all in src
  GEN      obj/build.h
  CXX      test/fuzz/fuzz-package_eval.o
test/fuzz/package_eval.cpp:130:5: error: code will never be executed [-Werror,-Wunreachable-code]
    error.reset();  // just to add unreachable code

maflcko · 2023-12-05T16:12:09Z

Just not the Assert.

I thought I tested this, but it looks like you are right. Any code from macros won't trigger the warning.

jonatack

ACK fa8adbe tested with arm64 clang 17.0.6

Could alternatively use -Wunreachable-code-aggressive -- it catches a few additional unreachable break statements in the current code.

maflcko · 2023-12-05T21:02:49Z

Could alternatively use -Wunreachable-code-aggressive -- it catches a few additional unreachable break statements in the current code.

They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?

jonatack · 2023-12-05T21:08:46Z

They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?

Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.

fanquake · 2023-12-06T11:01:35Z

Concept ACK

maflcko · 2023-12-08T15:46:45Z

They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?

Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.

Ok, but that'd require touching leveldb, which is a subtree, so I can't touch it here.

ajtowns · 2023-12-08T19:40:38Z

ACK fa8adbe

At least on my machine, this does't catch #28830 (comment) though.

Assert(!error) in that case is trivially true (error was just set to nullopt, !nullopt is true), so presumably it's getting compiled to a no-op, at which point there is no unreachable code to warn about?

maflcko · 2023-12-11T15:49:36Z

At least on my machine, this does't catch #28830 (comment) though.

Assert(!error) in that case is trivially true (error was just set to nullopt, !nullopt is true), so presumably it's getting compiled to a no-op, at which point there is no unreachable code to warn about?

I did some earlier testing, and I think generally any code injected from macros will be ignored by this warning. Adding a no-op verbatim should warn, IIRC.

stickies-v · 2023-12-11T16:44:29Z

I did some testing too. Adding a std::cout << "Hello\n"; at the end of MakeMempool results in a warning:

git diff on fa8adbe

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 5a08d0ff44..2369c2ee0d 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -126,6 +126,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 
     // ...and construct a CTxMemPool from it
     return CTxMemPool{mempool_opts};
+    std::cout << "Hello\n";
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)

test/fuzz/package_eval.cpp:129:15: error: code will never be executed [-Werror,-Wunreachable-code]
    std::cout << "Hello\n";

Replacing it with a macro then compiles fine:

git diff on fa8adbe

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 5a08d0ff44..1e616272d9 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -106,6 +106,8 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
     SetMockTime(time);
 }
 
+#define Print(val) std::cout << val;
+
 CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
 {
     // Take the default options for tests...
@@ -126,6 +128,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 
     // ...and construct a CTxMemPool from it
     return CTxMemPool{mempool_opts};
+    Print("Hello\n");
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)

Wrapping the macro in a function then again results in a warning:

git diff on fa8adbe

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 5a08d0ff44..ec8b8fd4fb 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -106,6 +106,9 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
     SetMockTime(time);
 }
 
+#define Print(val) std::cout << val;
+void PrintFn(const std::string& val) { Print(val); }
+
 CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
 {
     // Take the default options for tests...
@@ -126,6 +129,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 
     // ...and construct a CTxMemPool from it
     return CTxMemPool{mempool_opts};
+    PrintFn("Hello\n");
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)

test/fuzz/package_eval.cpp:132:5: error: code will never be executed [-Werror,-Wunreachable-code]
    PrintFn("Hello\n");

We currently don't have any unreached instances of Assert or Assume, and I'm not sure it's worth changing this, but if we want to catch this going forward we could wrap the macros in a function, e.g. as such below, which does create a warning. edit: doesn't work, as per the next comment

git diff on fa8adbe

diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 5a08d0ff44..58d7b6feb2 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -126,6 +126,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
 
     // ...and construct a CTxMemPool from it
     return CTxMemPool{mempool_opts};
+    Assert(false);
 }
 
 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
diff --git a/src/util/check.h b/src/util/check.h
index a02a1de8dc..4d37edeca4 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -73,8 +73,13 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
 #define CHECK_NONFATAL(condition) \
     inline_check_non_fatal(condition, __FILE__, __LINE__, __func__, #condition)
 
+#define AssertImpl(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
+
 /** Identity function. Abort if the value compares equal to zero */
-#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
+template <typename T>
+T&& Assert(T&& val) { return AssertImpl(std::forward<T>(val)); }
+
+#define AssumeImpl(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
 
 /**
  * Assume is the identity function.
@@ -86,7 +91,8 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
  * - For non-fatal errors in interactive sessions (e.g. RPC or command line
  *   interfaces), CHECK_NONFATAL() might be more appropriate.
  */
-#define Assume(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
+template <typename T>
+T&& Assume(T&& val) { return AssumeImpl(std::forward<T>(val)); }
 
 /**
  * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.

test/fuzz/package_eval.cpp:129:5: error: code will never be executed [-Werror,-Wunreachable-code]
    Assert(false);

maflcko · 2023-12-11T17:19:44Z

we could wrap the macros in a function

I don't think this works, because it will replace __FILE__, __LINE__, __func__ with a meaningless string literal constant.

maflcko · 2023-12-12T12:33:05Z

I don't think this works, because it will replace __FILE__, __LINE__, __func__ with a meaningless string literal constant.

C++20 fixed that, see https://en.cppreference.com/w/cpp/utility/source_location

fanquake · 2023-12-12T13:21:35Z

C++20 fixed that, see https://en.cppreference.com/w/cpp/utility/source_location

Hopefully we'll be able to use this at some point, however Apple Clang does not support this at all yet, and it looks like it will also come with a LLVM 15, if not 16+ requirement as well.

maflcko · 2023-12-12T13:26:12Z

Jup, it is GCC libstdc++11, or Clang libc++16, or later

ryanofsky · 2024-01-24T14:38:17Z

src/wallet/walletdb.cpp

    }

 #ifdef USE_BDB
    return MakeBerkeleyDatabase(path, options, status, error);
-#endif
+#else


In commit "build: Enable -Wunreachable-code" (fa8adbe)

Not a big deal, but IMO, this change is not an improvement. The unreachable code warning seems helpful to catch cases where code is unintentionally unreachable. Or to encourage rewriting code where code is intentionally reachable but written in a hard to understand way.

But this code was intentionally unreachable in obvious way with two return statements right next to each other. It was written this way so changes to code could be checked at compile time without developers needing rebuild with --without-bdb and --with-sqlite=yes options. So this seems like a case where the compiler warning is unhelpful, and it would be nice if there was a way to disable it.

Also, I might have missed it but it doesn't seem like we have CI builds that test building with wallet enabled and sqlite or BDB disabled. So now maybe bugs in this code could be unnoticed?

It was written this way so changes to code could be checked at compile time without developers needing rebuild with --without-bdb and --with-sqlite=yes options. So this seems like a case where the compiler warning is unhelpful, and it would be nice if there was a way to disable it.

I think it can be disabled via the verbose pragma clang diagnostic ignored, but that is ugly and verbose. A better workaround could be to use if constexpr(...USE_BDB...) { return MakeBDB();} else { return error; }? This should ensure the error code is compile-able, even though it is never run.

Also, I might have missed it but it doesn't seem like we have CI builds that test building with wallet enabled and sqlite or BDB disabled. So now maybe bugs in this code could be unnoticed?

Seems unrelated to the changes here, because if a piece of code compiles does not mean it has no bugs. If a test is truly missing, I have no objection to adding one.

if a piece of code compiles does not mean it has no bugs

I've noticed that sometimes. And it would be unreasonable to expect our CI to run every line of code, but I think it's reasonable to expect our CI to at least compile every line of code and not leave blind spots like this. So I really like your constexpr idea, and can try it out in #25722 after I finish rebasing it. Thanks for suggesting that.

When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes bitcoin#28999 (comment) Can be reviewed with --ignore-all-space --patience

When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes bitcoin#28999 (comment) Can be reviewed with --ignore-all-space

fa3373d refactor: Compile unreachable code (MarcoFalke) Pull request description: When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes #28999 (comment) ACKs for top commit: achow101: ACK fa3373d ryanofsky: Code review ACK fa3373d. This looks good, and should prevent code in the else blocks from accidentally breaking. Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e

When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes bitcoin/bitcoin#28999 (comment) Can be reviewed with --ignore-all-space

fa3373d refactor: Compile unreachable code (MarcoFalke) Pull request description: When unreachable code isn't compiled, compile failures are not detected. Fix this by leaving it unreachable, but compiling it. Fixes bitcoin#28999 (comment) ACKs for top commit: achow101: ACK fa3373d ryanofsky: Code review ACK fa3373d. This looks good, and should prevent code in the else blocks from accidentally breaking. Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e

build: Enable -Wunreachable-code

fa8adbe

DrahtBot added the Build system label Dec 5, 2023

maflcko mentioned this pull request Dec 5, 2023

[refactor] Check CTxMemPool options in ctor #28830

Merged

Sjors reviewed Dec 5, 2023

View reviewed changes

stickies-v approved these changes Dec 5, 2023

View reviewed changes

DrahtBot requested a review from jonatack December 5, 2023 15:49

jonatack reviewed Dec 5, 2023

View reviewed changes

This was referenced Dec 5, 2023

wallet: Allow users to create a wallet that encrypts all database records #28142

Closed

build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972

Merged

fanquake merged commit d5e5810 into bitcoin:master Dec 11, 2023

maflcko deleted the 2312-warn-unreachable-code- branch December 11, 2023 16:43

This was referenced Dec 14, 2023

refactor: Remove c_str from util/check #26960

Merged

build: Require libc++-16 or later #29077

Closed

ajtowns mentioned this pull request Jan 8, 2024

logging: Simplify API for level based logging #28318

Merged

ryanofsky reviewed Jan 24, 2024

View reviewed changes

maflcko mentioned this pull request Jan 25, 2024

refactor: Compile unreachable walletdb code #29315

Merged

maflcko mentioned this pull request Apr 10, 2024

fuzz: use std::source_location in fuzz/rpc.cpp #29841

Closed

maflcko mentioned this pull request Jun 10, 2024

build: Bump clang minimum supported version to 16 #30263

Merged

bitcoin locked and limited conversation to collaborators Jan 23, 2025

build: Enable -Wunreachable-code #28999

build: Enable -Wunreachable-code #28999

Uh oh!

Conversation

maflcko commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

jonatack commented Dec 5, 2023

Uh oh!

Sjors Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 5, 2023

Uh oh!

stickies-v commented Dec 5, 2023

Uh oh!

maflcko commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 5, 2023

Uh oh!

jonatack commented Dec 5, 2023

Uh oh!

fanquake commented Dec 6, 2023

Uh oh!

maflcko commented Dec 8, 2023

Uh oh!

ajtowns commented Dec 8, 2023

Uh oh!

maflcko commented Dec 11, 2023

Uh oh!

stickies-v commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 11, 2023

Uh oh!

maflcko commented Dec 12, 2023

Uh oh!

fanquake commented Dec 12, 2023

Uh oh!

maflcko commented Dec 12, 2023

Uh oh!

ryanofsky Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 5, 2023 •

edited

Loading

DrahtBot commented Dec 5, 2023 •

edited

Loading

stickies-v Dec 5, 2023 •

edited

Loading

stickies-v Dec 5, 2023 •

edited

Loading

maflcko commented Dec 5, 2023 •

edited

Loading

jonatack left a comment •

edited

Loading

stickies-v commented Dec 11, 2023 •

edited

Loading

ryanofsky Jan 24, 2024 •

edited

Loading