UNREACHABLE macro
#26504
The UNREACHABLE macro was suggested in #24812, but during reviewing it has been dropped:
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 writeassert(false)will just lead to bike-shedding without any benefit.
This macro prevents compiler warnings – -Wreturn-type (GCC) and C4715 (MSVC) – when building for/on Windows.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | fanquake |
| Concept ACK | aureleoules, MarcoFalke |
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.
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 (fatal, aborting)", __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT) << std::endl; \
This approach requires that libbitcoinconsensus.la being linked against libbitcoin_util.a. The latter, in turn, must be converted into a Libtool archive.
Therefore, I’d prefer to keep this in the header.
support/cleanse.cpp, at least that way the dependency on the macro is obvious
~0 - agree with the earlier discussion in #24812. Why do we need to redefine assert(), because Windows compilers are broken? We’ll have an unintuitive split where assert() is used, except in a switch, because we’re trying to suppress randomly (the warnings aren’t emitted consistently from switches with mingw-w64) happening warnings.
This will be another thing developers have to look up, rather than seeing assert(), and knowing what it means.
If we were going to introduce an unreachable macro, we could at least use __builtin_unreachable (GCC, Clang, ICC). Of course MSVC doesn’t implement that, and instead has __assume() (not to be confused with our own Assume()).
because Windows compilers are broken
They are not broken, as they do not violate the C++ standard. Of course, the way how they consider warnings for int f(){ assert(false); } differs from other popular compilers.
We’ll have an unintuitive split where
assert()is used, except in a switch, because we’re trying to suppress randomly (the warnings aren’t emitted consistently from switches with mingw-w64) happening warnings.
For non-Windows targets warnings are inconsistent between GCC and Clang as well (remove assert(false); statement to see it).
If we were going to introduce an unreachable macro, we could at least use
__builtin_unreachable(GCC, Clang, ICC). Of course MSVC doesn’t implement that, and instead has__assume()(not to be confused with our ownAssume()).
That could be like an implementation of std::unreachable from C++23.
But the suggested macro is safer when it comes to handle, for example, non-defined-explicitly enumeration values, as std::abort is better than UB.
Updated 05ffaf4591911b3c19ce6ab347538642765190c0 -> 39d681e1c741880f96ef5c2f4c0ae2e93e5ddbcc (pr26504.01 -> pr26504.02):
94@@ -95,4 +95,12 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
95 format_internal_error("Unreachable code reached (non-fatal)", \
96 __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT))
97
98+[[noreturn]] void inline_unreachable(const char* file, int line, const char* func);
unreachable_fail for symmetry?
695@@ -695,7 +696,7 @@ uint32_t CNetAddr::GetLinkedIPv4() const
696 // Teredo tunneled IPv4: the IPv4 address is in the last 4 bytes of the address, but bitflipped
697 return ~ReadBE32(Span{m_addr}.last(ADDR_IPV4_SIZE).data());
698 }
699- assert(false);
700+ UNREACHABLE();
I assume doesn’t warn under mingw-w64
It does when compiling with -O0.
Updated 39d681e1c741880f96ef5c2f4c0ae2e93e5ddbcc -> dd47e2aaf869806455289d55222dd981ee8cea3f (pr26504.02 -> pr26504.03):
27@@ -27,3 +28,9 @@ void assertion_fail(const char* file, int line, const char* func, const char* as
28 fwrite(str.data(), 1, str.size(), stderr);
29 std::abort();
30 }
31+
32+void unreachable_fail(const char* file, int line, const char* func)
33+{
34+ std::cerr << strprintf("Internal bug detected: Unreachable code reached (fatal, aborting)\n%s:%d (%s)\nPlease report this issue here: %s\n", file, line, func, PACKAGE_BUGREPORT) << std::endl;
Updated dd47e2aaf869806455289d55222dd981ee8cea3f -> 426d6bed7318ad38e0932498d3730af04993ca94 (pr26504.03 -> pr26504.04):
To make this PR compatible with the recently merged #26645, the CopyrightHolders() and LicenseInfo() functions have been moved from clientversion.{h,cpp}' to util/string.{h,cpp}.
972@@ -973,7 +973,7 @@ if BUILD_BITCOIN_LIBS
973 lib_LTLIBRARIES += $(LIBBITCOINCONSENSUS)
974
975 include_HEADERS = script/bitcoinconsensus.h
976-libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
977+libbitcoinconsensus_la_SOURCES = clientversion.cpp support/cleanse.cpp util/check.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
src/script/interpreter.cpp for now and only use the second and third commit?
Concept ACK on UNREACHABLE. I find it frustrating to play compiler golf against g++ in the CI on every code push that uses a switch-case on an enum class.
First push is -Werror=return-type, the second push is Assert(false);, but g++ failing to understand it, then the third push is assert(false);
Co-authored-by: Aurèle Oulès <aurele@oules.com>
Co-authored-by: MacroFake <falke.marco@gmail.com>
Maybe drop this change and the one to
src/script/interpreter.cppfor now and only use the second and third commit?
Done. Rebased.
492@@ -493,7 +493,7 @@ class CNode
493 return true;
494 } // no default case, so the compiler can warn about missing cases
495
Looks like you forgot to include the doc change? https://github.com/bitcoin/bitcoin/commit/abacaef3f3b541ead566532eaae9f5db31ccb7ff#diff-5e319039d67254bed6ca82562ec5f560f102004aa4806e4ca77f5d0065c65fbb
Also, could update title, description and commit message?
11@@ -12,6 +12,7 @@
12 #include <logging.h>
13 #include <tinyformat.h>
14 #include <util/chaintype.h>
15+#include <util/check.h>
util/check.h code is added to a fair number of compilation units (unless the header was already missing). Also, it looks like some cases haven’t been updated. Maybe only use it for new or touched code going forward?
This change prevents `-Wreturn-type` and `C4715` warnings when building
for/on Windows.
Also, could update title, description and commit message?
The title an the description have been updated. What update for commit messages would you like to see?
848+ UNREACHABLE();
849 }
850 ```
851
852-*Rationale*: The comment documents skipping `default:` label, and it complies with `clang-format` rules. The assertion prevents firing of `-Wreturn-type` warning on some compilers.
853+*Rationale*: The comment documents skipping `default:` label, and it complies with `clang-format` rules. The `UNREACHABLE` macro prevents firing of `-Wreturn-type`/`C4715` warning on some compilers. In RPC code the `NONFATAL_UNREACHABLE` macro might be more appropriate instead.
0*Rationale*: The comment documents skipping the `default:` case and it complies with `clang-format` rules. The `UNREACHABLE` macro prevents firing of `-Wreturn-type`/`C4715` warnings on some compilers. In the RPC code, the `NONFATAL_UNREACHABLE` macro might be more appropriate instead.
Apparently, this macro prevents compiler warnings
Either it does or it doesn’t? If it does, then any workarounds should be removed, and warnings re-enabled.
If it doesn’t, there’s no point to this refactoring at all, if it doesn’t even acheive the stated goals.
I find it frustrating to play compiler golf against g++ in the CI on every code push that uses a switch-case on an enum class.
Which CI is this? The only one where it is an issue has the warnings disabled?
Which CI is this? The only one where it is an issue has the warnings disabled?
I don’t use g++, nor Windows, locally, so this is a general problem of not knowing which function is marked noreturn by g++ or msvc internally. Having a macro that is known to work on all compilers is helpful, I think.
I don’t care about updating the existing code, but I think having a macro for those that find it helpful, can’t hurt, can it?
but I think having a macro for those that find it helpful, can’t hurt, can it?
I agree that having a macro could be useful.
Separately, note that upstream, the missing noreturn attributes have been added to assert in the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, so beginning with 11.0.0, there are no-longer -Wreturn-type warnings with assert(false) usage. See #28092 where I’ve updated the docs.
I don’t care about updating the existing code,
To be clear, I agree, and am still a NACK to any code changes here.