This PR replaces the macro CHECK_NONFATAL with an identity function.
I simplified the usage of CHECK_NONFATAL where applicable in src/rpc.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: #24804 (review).
Also adds UNREACHABLE_NONFATAL macro.
93+ *
94+ * - Should be used to run non-fatal checks.
95+ * - For fatal errors, use Assert().
96+ * - Appropriate for non-fatal errors in interactive sessions (e.g. RPC or command line interfaces).
97+ */
98+#define CheckNonFatal(val) inline_check_non_fatal(val, __FILE__, __LINE__, __func__, #val)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
915@@ -916,6 +916,7 @@ static std::string RecurseImportData(const CScript& script, ImportData& import_d
916 return "unrecognized script";
917 } // no default case, so the compiler can warn about missing cases
918 CHECK_NONFATAL(false);
919+ return ""; // not reached
Hmm, I am wondering if it makes sense to introduce a macro that throws NonFatalCheckError unconditionally?
CHECK_NONFATAL(false) is inspired by assert(false) to mark unreachable code. Though, hiding the implementation behind a function call makes it harder for the compiler to realize the function never returns when called with false. (The same happens when using Assert(false)).
UNREACHABLE_NONFATAL() and UNREACHABLE()?
Hmm, I am wondering if it makes sense to introduce a macro that throws NonFatalCheckError unconditionally?
CHECK_NONFATAL(false) is inspired by assert(false) to mark unreachable code. Though, hiding the implementation behind a function call makes it harder for the compiler to realize the function never returns when called with false. (The same happens when using Assert(false)).
Concept ACK, this will make the code much cleaner
93+#define NONFATAL_UNREACHABLE() \
94+ do { \
95+ throw NonFatalCheckError( \
96+ format_internal_error("Unreachable code reached", \
97+ __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT)); \
98+ } while (0)
0 } while (false)
do-while-0 is only used when there is a need to force a trailing semicolon or to bundle several statements or “leaking” statements into one, none of which apply here, so it could be dropped as well?
while(0) in C++, maybe C in yes
22+ strprintf("Internal bug detected: '%s'\n%s:%d (%s)\nPlease report this issue here: %s\n", \
23+ msg, file, line, func, report)
24+
25+/** Helper for CHECK_NONFATAL() */
26+template <typename T>
27+T&& inline_check_non_fatal(T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion)
maybe_unused used or actually unused here?
102+ * This is used to mark code that is not yet implemented or is not yet reachable.
103+ */
104+#define UNREACHABLE() \
105+ do { \
106+ std::cerr << format_internal_error("Unreachable code reached", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT) << std::endl; \
107+ abort(); \
0 std::abort(); \
90+ * NONFATAL_UNREACHABLE() is a macro that is used to mark unreachable code. It throws a NonFatalCheckError.
91+ * This is used to mark code that is not yet implemented or is not yet reachable.
92+ */
93+#define NONFATAL_UNREACHABLE() \
94+ throw NonFatalCheckError( \
95+ format_internal_error("Unreachable code reached", \
maybe
0 format_internal_error("Unreachable code reached (non-fatal)", \
99+ * UNREACHABLE is a macro that is used to mark unreachable code. It aborts the program.
100+ * This is used to mark code that is not yet implemented or is not yet reachable.
101+ */
102+#define UNREACHABLE() \
103+ do { \
104+ std::cerr << format_internal_error("Unreachable code reached", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT) << std::endl; \
maybe
0 std::cerr << format_internal_error("Unreachable code reached (fatal, aborting)", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT) << std::endl; \
ACK 1e4d38267feb69bab20ed06367d0a0adcd211068
Tested NONFATAL_UNREACHABLE
0error code: -1
1error message:
2Internal bug detected: 'Unreachable code reached'
3wallet/rpc/addresses.cpp:38 (operator())
4Please report this issue here: https://github.com/bitcoin/bitcoin/issues
Tested UNREACHABLE
0Internal bug detected: 'Unreachable code reached'
1wallet/rpc/addresses.cpp:38 (operator())
2Please report this issue here: https://github.com/bitcoin/bitcoin/issues
3
4Aborted
If you retouch, it looks like #include <util/check.h> ought to be added to /rpc/blockchain.cpp and /rpc/util.cpp as well here.
17@@ -18,8 +18,24 @@ class NonFatalCheckError : public std::runtime_error
18 using std::runtime_error::runtime_error;
19 };
20
21+#define format_internal_error(msg, file, line, func, report) \
22+ strprintf("Internal bug detected: '%s'\n%s:%d (%s)\nPlease report this issue here: %s\n", \
0 strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\nPlease report this issue here: %s\n", \
which would output the following, for example:
0error code: -1
1error message:
2Internal bug detected: "Unreachable code reached"
3wallet/rpc/addresses.cpp:38 (operator())
4Please report this issue here: https://github.com/bitcoin/bitcoin/issues
Might be good to adjust the rpc linter as well for the new macros: #24856
Can be done in a follow-up to avoid a conflict for now.
97+
98+/**
99+ * UNREACHABLE is a macro that is used to mark unreachable code. It aborts the program.
100+ * This is used to mark code that is not yet implemented or is not yet reachable.
101+ */
102+#define UNREACHABLE() \
assert(false) just fine. Introducing another way to write assert(false) will just lead to bike-shedding without any benefit.
in favor to have a macro like UNREACHABLE is that we can the possibility to add more failures to the error message, that it is always good.
Maybe somethings assert(false && "err message") can be a good motivation to have this macros, otherwise I agree with @MarcoFalke
I think that this is unreachable code, so by definition it is not something that a user will (or should) ever see.
It could be useful for developers modifying code and stumbling across an unreachable block as a result.
Though I agree that it’s not an essential feature considering assert(false) works alright.
assert(false) means and what it does. Printing PACKAGE_BUGREPORT for them also doesn’t seem to add much value.
793@@ -793,7 +794,7 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
794 return;
795 }
796 case Type::ANY: {
797- CHECK_NONFATAL(false); // Only for testing
798+ NONFATAL_UNREACHABLE(); // Only for testing
799 }
This will also fix a warning on current master with gcc-12:
0In file included from ./rpc/util.h:19,
1 from rpc/util.cpp:8:
2rpc/util.cpp: In member function 'void RPCResult::ToSections(Sections&, OuterType, int) const':
3./util/check.h:34:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
4 34 | if (!(condition)) { \
5 | ^~
6rpc/util.cpp:796:9: note: in expansion of macro 'CHECK_NONFATAL'
7 796 | CHECK_NONFATAL(false); // Only for testing
8 | ^~~~~~~~~~~~~~
9rpc/util.cpp:798:5: note: here
10 798 | case Type::NONE: {
11 | ^~~~
24+
25+/** Helper for CHECK_NONFATAL() */
26+template <typename T>
27+T&& inline_check_non_fatal(T&& val, const char* file, int line, const char* func, const char* assertion)
28+{
29+ if (!(val)) {
0 if (!val) {
nit: No need for ().
28+{
29+ if (!(val)) {
30+ throw NonFatalCheckError(
31+ format_internal_error(assertion, file, line, func, PACKAGE_BUGREPORT));
32+ }
33+
Looks like this also fixes a warning with gcc-12.
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjsdwwArddL1iP4wywDs5YPWiBmm1hOzJSazUrpOb4gx4plkEEHXOXvGqIIwoRQ
8IPnUtiwBnIfZ7UDTHRUCgY8jeLNmlfZAtKRqd8n9V7fat6Uarmubynkz66O4C/pH
9lPV6WRkkcby+KDy01EJW10vGa0NqLg9VoxWiTIfb5LwoiyqcxnLYvJ6/Ir30GYsn
10cgG9pOIRhyYPYJQp79Q2R1fZmNe1Vi53RfxCBBkNXJI+tlMeCT7N8A6IHh239Dak
11KPQP43FDbgkhTuwDmf+RNipSqyvH0sWRTqNCei9Q6WWqwGiiRMH883GWtWO392bR
12gvkf+TmQeVoWqELVi+fFk7d+14zLJnnIvouMESjacwCEbkL5j5OKSw8WW6g0Uffb
13y5VyRSEFV02OrxMHmKFKn3UEAQm551gnILgu0ERvkKopedGXxttNdvsnj9nuYiMj
14IyPjD7MVuu+Ri6IaFtJ7jxCUGm+cPgei79bOuumTrRt/6g/+QZZnniZyhNx5oVHz
15CKvY4MQC
16=l4Sj
17-----END PGP SIGNATURE-----