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)
I think you can just recycle the existing name. No need to add a second one.
It's now replaced I had to add unreachable return statements like below otherwise the CI would complain https://github.com/bitcoin/bitcoin/blob/0661baffe8044cc049f6494ab29b79f5e4b00998/src/rpc/util.cpp#L986-L993
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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)).
How about a macro 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)
} 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?
Concept ACK
Fixed it
LGTM, but I'm not a big fan of 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)
Are the maybe_unused used or actually unused here?
Removed annotations
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(); \
std::abort(); \
Fixed it
lgtm
I've addressed the nits, thanks for your reviews.
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
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
std::cerr << format_internal_error("Unreachable code reached (fatal, aborting)", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT) << std::endl; \
ACK 1e4d38267feb69bab20ed06367d0a0adcd211068
Tested NONFATAL_UNREACHABLE
error code: -1
error message:
Internal bug detected: 'Unreachable code reached'
wallet/rpc/addresses.cpp:38 (operator())
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
Tested UNREACHABLE
Internal bug detected: 'Unreachable code reached'
wallet/rpc/addresses.cpp:38 (operator())
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
Aborted
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", \
strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\nPlease report this issue here: %s\n", \
which would output the following, for example:
error code: -1
error message:
Internal bug detected: "Unreachable code reached"
wallet/rpc/addresses.cpp:38 (operator())
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
re-ACK 63b753416abddb5f82ed32ff64a3372e65722fc2
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() \
This is unused and on a second thought I wonder if there is any value in it. The current code can already use 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. So there is not much benefit in spending time to overengineer error messages and formatting of them.
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.
Developers would already know what assert(false) means and what it does. Printing PACKAGE_BUGREPORT for them also doesn't seem to add much value.
Makes sense, I removed it
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9
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:
In file included from ./rpc/util.h:19,
from rpc/util.cpp:8:
rpc/util.cpp: In member function 'void RPCResult::ToSections(Sections&, OuterType, int) const':
./util/check.h:34:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
34 | if (!(condition)) { \
| ^~
rpc/util.cpp:796:9: note: in expansion of macro 'CHECK_NONFATAL'
796 | CHECK_NONFATAL(false); // Only for testing
| ^~~~~~~~~~~~~~
rpc/util.cpp:798:5: note: here
798 | case Type::NONE: {
| ^~~~
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)) {
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 | +
nit: no need for newline
Looks like this also fixes a warning with gcc-12.
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjsdwwArddL1iP4wywDs5YPWiBmm1hOzJSazUrpOb4gx4plkEEHXOXvGqIIwoRQ
IPnUtiwBnIfZ7UDTHRUCgY8jeLNmlfZAtKRqd8n9V7fat6Uarmubynkz66O4C/pH
lPV6WRkkcby+KDy01EJW10vGa0NqLg9VoxWiTIfb5LwoiyqcxnLYvJ6/Ir30GYsn
cgG9pOIRhyYPYJQp79Q2R1fZmNe1Vi53RfxCBBkNXJI+tlMeCT7N8A6IHh239Dak
KPQP43FDbgkhTuwDmf+RNipSqyvH0sWRTqNCei9Q6WWqwGiiRMH883GWtWO392bR
gvkf+TmQeVoWqELVi+fFk7d+14zLJnnIvouMESjacwCEbkL5j5OKSw8WW6g0Uffb
y5VyRSEFV02OrxMHmKFKn3UEAQm551gnILgu0ERvkKopedGXxttNdvsnj9nuYiMj
IyPjD7MVuu+Ri6IaFtJ7jxCUGm+cPgei79bOuumTrRt/6g/+QZZnniZyhNx5oVHz
CKvY4MQC
=l4Sj
-----END PGP SIGNATURE-----
</details>