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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
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.
1498@@ -1499,17 +1499,19 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
1499 if (format == DatabaseFormat::SQLITE) {
1500 #ifdef USE_SQLITE
1501 return MakeSQLiteDatabase(path, options, status, error);
1502-#endif
1503+#else
ACK fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876
At least on my machine, this does’t catch #28830 (review) 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?
At least on my machine, this does’t catch
You’ll have to use clang, because GCC does not have those warnings.
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.
0 // ...and construct a CTxMemPool from it
1 std::optional<bilingual_str> error{};
2 return CTxMemPool{mempool_opts, error};
3 error.reset(); // just to add unreachable code
4 Assert(!error);
0% make
1Making all in src
2 GEN obj/build.h
3 CXX test/fuzz/fuzz-package_eval.o
4test/fuzz/package_eval.cpp:130:5: error: code will never be executed [-Werror,-Wunreachable-code]
5 error.reset(); // just to add unreachable code
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.
ACK fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876 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.
Could alternatively use -Wunreachable-code-aggressive – it catches a few additional unreachable
breakstatements in the current code.
They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
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.
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.
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?
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.
I did some testing too. Adding a std::cout << "Hello\n"; at the end of MakeMempool results in a warning:
0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
1index 5a08d0ff44..2369c2ee0d 100644
2--- a/src/test/fuzz/package_eval.cpp
3+++ b/src/test/fuzz/package_eval.cpp
4@@ -126,6 +126,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
5
6 // ...and construct a CTxMemPool from it
7 return CTxMemPool{mempool_opts};
8+ std::cout << "Hello\n";
9 }
10
11 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
0test/fuzz/package_eval.cpp:129:15: error: code will never be executed [-Werror,-Wunreachable-code]
1 std::cout << "Hello\n";
Replacing it with a macro then compiles fine:
0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
1index 5a08d0ff44..1e616272d9 100644
2--- a/src/test/fuzz/package_eval.cpp
3+++ b/src/test/fuzz/package_eval.cpp
4@@ -106,6 +106,8 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
5 SetMockTime(time);
6 }
7
8+#define Print(val) std::cout << val;
9+
10 CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
11 {
12 // Take the default options for tests...
13@@ -126,6 +128,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
14
15 // ...and construct a CTxMemPool from it
16 return CTxMemPool{mempool_opts};
17+ Print("Hello\n");
18 }
19
20 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
Wrapping the macro in a function then again results in a warning:
0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
1index 5a08d0ff44..ec8b8fd4fb 100644
2--- a/src/test/fuzz/package_eval.cpp
3+++ b/src/test/fuzz/package_eval.cpp
4@@ -106,6 +106,9 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chains
5 SetMockTime(time);
6 }
7
8+#define Print(val) std::cout << val;
9+void PrintFn(const std::string& val) { Print(val); }
10+
11 CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
12 {
13 // Take the default options for tests...
14@@ -126,6 +129,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
15
16 // ...and construct a CTxMemPool from it
17 return CTxMemPool{mempool_opts};
18+ PrintFn("Hello\n");
19 }
20
21 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
0test/fuzz/package_eval.cpp:132:5: error: code will never be executed [-Werror,-Wunreachable-code]
1 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
0diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
1index 5a08d0ff44..58d7b6feb2 100644
2--- a/src/test/fuzz/package_eval.cpp
3+++ b/src/test/fuzz/package_eval.cpp
4@@ -126,6 +126,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
5
6 // ...and construct a CTxMemPool from it
7 return CTxMemPool{mempool_opts};
8+ Assert(false);
9 }
10
11 FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
12diff --git a/src/util/check.h b/src/util/check.h
13index a02a1de8dc..4d37edeca4 100644
14--- a/src/util/check.h
15+++ b/src/util/check.h
16@@ -73,8 +73,13 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
17 #define CHECK_NONFATAL(condition) \
18 inline_check_non_fatal(condition, __FILE__, __LINE__, __func__, #condition)
19
20+#define AssertImpl(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
21+
22 /** Identity function. Abort if the value compares equal to zero */
23-#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
24+template <typename T>
25+T&& Assert(T&& val) { return AssertImpl(std::forward<T>(val)); }
26+
27+#define AssumeImpl(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
28
29 /**
30 * Assume is the identity function.
31@@ -86,7 +91,8 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
32 * - For non-fatal errors in interactive sessions (e.g. RPC or command line
33 * interfaces), CHECK_NONFATAL() might be more appropriate.
34 */
35-#define Assume(val) inline_assertion_check<false>(val, __FILE__, __LINE__, __func__, #val)
36+template <typename T>
37+T&& Assume(T&& val) { return AssumeImpl(std::forward<T>(val)); }
38
39 /**
40 * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.
0test/fuzz/package_eval.cpp:129:5: error: code will never be executed [-Werror,-Wunreachable-code]
1 Assert(false);
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.
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
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.
1508 }
1509
1510 #ifdef USE_BDB
1511 return MakeBerkeleyDatabase(path, options, status, error);
1512-#endif
1513+#else
In commit “build: Enable -Wunreachable-code” (fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876)
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.