Add `UNREACHABLE` macro #26504

hebasto · 2022-11-15T12:47:06Z

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 write assert(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.

hebasto · 2022-11-15T12:47:42Z

cc @aureleoules @MarcoFalke

DrahtBot · 2022-11-16T00:16:25Z

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

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)
#26697 (logging: use bitset for categories by LarryRuane)

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.

src/util/check.h

aureleoules · 2022-11-16T12:25:15Z

Concept ACK

fanquake · 2022-11-16T12:26:39Z

~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()).

hebasto · 2022-11-16T14:26:17Z

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 own Assume()).

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.

hebasto · 2022-11-16T14:30:57Z

Updated 05ffaf4 -> 39d681e (pr26504.01 -> pr26504.02):

addressed @MarcoFalke's comment

src/util/check.h

fanquake · 2022-11-16T15:25:12Z

src/netaddress.cpp

@@ -695,7 +696,7 @@ uint32_t CNetAddr::GetLinkedIPv4() const
        // Teredo tunneled IPv4: the IPv4 address is in the last 4 bytes of the address, but bitflipped
        return ~ReadBE32(Span{m_addr}.last(ADDR_IPV4_SIZE).data());
    }
-    assert(false);
+    UNREACHABLE();


This is now also changing code that does not use switch statements, and I assume doesn't warn under mingw-w64, becoming inconsistent with the rest of the codebase. If we are going to change some of these, we should be changing all, otherwise this is just random refactoring.

I assume doesn't warn under mingw-w64

It does when compiling with -O0.

There'd be test/lint/lint-assertions.py if this is a goal

@fanquake

we should be changing all, otherwise this is just random refactoring.

In that case, what macro name could be appropriate?

hebasto · 2022-11-16T16:24:01Z

Updated 39d681e -> dd47e2a (pr26504.02 -> pr26504.03):

rebased due to conflict with util: Move error message formatting of NonFatalCheckError to cpp #25112
addressed @MarcoFalke's comment

maflcko · 2022-12-10T15:59:55Z

src/util/check.cpp

+
+void unreachable_fail(const char* file, int line, const char* func)
+{
+    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;


could rebase and use the existing string template?

hebasto · 2022-12-10T18:30:37Z

Updated dd47e2a -> 426d6be (pr26504.03 -> pr26504.04):

addressed @MarcoFalke's comment

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}.

hebasto · 2022-12-12T15:38:58Z

Rebased on top of the #26688 and drafted until the latter is being reviewed.

maflcko · 2023-07-14T10:53:14Z

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>

maflcko · 2023-07-14T13:11:19Z

@hebasto Let us know if you'll pick this up again or if you want someone else to do it?

hebasto · 2023-07-14T13:11:53Z

@hebasto Let us know if you'll pick this up again or if you want someone else to do it?

Almost ready :)

hebasto · 2023-07-14T13:20:54Z

re #26504 (comment)

Maybe drop this change and the one to src/script/interpreter.cpp for now and only use the second and third commit?

Done. Rebased.

maflcko

Nice. Review lgtm

src/net.h

maflcko · 2023-07-14T13:27:56Z

Looks like you forgot to include the doc change? abacaef#diff-5e319039d67254bed6ca82562ec5f560f102004aa4806e4ca77f5d0065c65fbb

Also, could update title, description and commit message?

jonatack

If this is done, IIUC the developer notes should be updated.

jonatack · 2023-07-14T13:39:17Z

src/chainparams.cpp

@@ -12,6 +12,7 @@
 #include <logging.h>
 #include <tinyformat.h>
 #include <util/chaintype.h>
+#include <util/check.h>


A potential downside with this change is the 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.

hebasto · 2023-07-14T13:59:34Z

@MarcoFalke

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?

doc/developer-notes.md

fanquake · 2023-07-14T14:35:27Z

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?

maflcko · 2023-07-14T15:33:31Z

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?

fanquake · 2023-07-17T14:26:50Z

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, mingw-w64/mingw-w64@1690994, 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.

fanquake · 2023-07-19T10:13:57Z

I don't care about updating the existing code,

To be clear, I agree, and am still a NACK to any code changes here.

hebasto mentioned this pull request Nov 15, 2022

build: Disable noisy -Wreturn-type for MinGW-w64 #26190

Closed

DrahtBot mentioned this pull request Nov 16, 2022

refactor: Extract util/exception from util/system #26301

Closed

maflcko reviewed Nov 16, 2022

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 16, 2022

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Closed

hebasto force-pushed the 221115-enum branch from 05ffaf4 to 39d681e Compare November 16, 2022 14:27

maflcko reviewed Nov 16, 2022

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

fanquake reviewed Nov 16, 2022

View reviewed changes

hebasto force-pushed the 221115-enum branch from 39d681e to dd47e2a Compare November 16, 2022 15:56

DrahtBot mentioned this pull request Nov 17, 2022

build: Build libbitcoinconsensus from its own convenience library #24994

Closed

hebasto mentioned this pull request Dec 10, 2022

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

Merged

maflcko reviewed Dec 10, 2022

View reviewed changes

hebasto force-pushed the 221115-enum branch from dd47e2a to 426d6be Compare December 10, 2022 18:26

hebasto mentioned this pull request Dec 12, 2022

Move CopyrightHolders() and LicenseInfo() into libbitcoin_common #26688

Closed

hebasto force-pushed the 221115-enum branch from 426d6be to 5bc9b37 Compare December 12, 2022 15:37

hebasto marked this pull request as draft December 12, 2022 15:39

hebasto force-pushed the 221115-enum branch from 5bc9b37 to 31b4779 Compare December 12, 2022 15:44

DrahtBot mentioned this pull request Dec 24, 2022

logging: use bitset for categories #26697

Closed

DrahtBot mentioned this pull request Jan 11, 2023

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot mentioned this pull request Jan 21, 2023

Add fee_est tool for debugging fee estimation code #10443

Closed

This was referenced Feb 17, 2023

OP_VAULT draft #26857

Closed

Deduplicate bitcoind and bitcoin-qt init code #27150

Merged

util: Add UNREACHABLE macro

0ef8f1b

Co-authored-by: Aurèle Oulès <aurele@oules.com> Co-authored-by: MacroFake <falke.marco@gmail.com>

hebasto reopened this Jul 14, 2023

hebasto force-pushed the 221115-enum branch from 31b4779 to 5f7af01 Compare July 14, 2023 13:19

hebasto marked this pull request as ready for review July 14, 2023 13:21

hebasto removed the Up for grabs label Jul 14, 2023

maflcko approved these changes Jul 14, 2023

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

jonatack reviewed Jul 14, 2023

View reviewed changes

Use UNREACHABLE macro in non-RPC code

924f45d

This change prevents `-Wreturn-type` and `C4715` warnings when building for/on Windows.

hebasto force-pushed the 221115-enum branch from 5f7af01 to a87e153 Compare July 14, 2023 13:56

DrahtBot added the CI failed label Jul 14, 2023

hebasto changed the title ~~Add UNREACHABLE macro and drop -Wreturn-type/C4715 warnings suppressions~~ Add UNREACHABLE macro Jul 14, 2023

jonatack reviewed Jul 14, 2023

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

doc: Amend enumerations chapter in Developer Notes

1544bae

hebasto force-pushed the 221115-enum branch from a87e153 to 1544bae Compare July 14, 2023 14:06

DrahtBot removed the Needs rebase label Jul 14, 2023

DrahtBot removed the CI failed label Jul 14, 2023

This was referenced Jul 15, 2023

net, refactor: extract Network and BIP155Network logic to node/network #27385

Closed

Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs #27231

Closed

hebasto closed this Jul 20, 2023

bitcoin locked and limited conversation to collaborators Jul 19, 2024

Add UNREACHABLE macro #26504

Add UNREACHABLE macro #26504

Uh oh!

Conversation

hebasto commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 15, 2022

Uh oh!

DrahtBot commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

aureleoules commented Nov 16, 2022

Uh oh!

fanquake commented Nov 16, 2022

Uh oh!

hebasto commented Nov 16, 2022

Uh oh!

hebasto commented Nov 16, 2022

Uh oh!

Uh oh!

fanquake Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 16, 2022

Uh oh!

maflcko Dec 10, 2022 • 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!

hebasto Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 10, 2022

Uh oh!

hebasto commented Dec 12, 2022

Uh oh!

maflcko commented Jul 14, 2023

Uh oh!

maflcko commented Jul 14, 2023

Uh oh!

hebasto commented Jul 14, 2023

Uh oh!

hebasto commented Jul 14, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jul 14, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 14, 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!

hebasto commented Jul 14, 2023

Uh oh!

Uh oh!

fanquake commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 14, 2023

Uh oh!

fanquake commented Jul 17, 2023

Uh oh!

Add `UNREACHABLE` macro #26504

Add `UNREACHABLE` macro #26504

hebasto commented Nov 15, 2022 •

edited

Loading

DrahtBot commented Nov 16, 2022 •

edited

Loading

maflcko Dec 10, 2022 •

edited

Loading

jonatack Jul 14, 2023 •

edited

Loading

fanquake commented Jul 14, 2023 •

edited

Loading