util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro by aureleoules · Pull Request #24812 · bitcoin/bitcoin

aureleoules · 2022-04-08T23:48:03Z

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

Also adds UNREACHABLE_NONFATAL macro.

src/util/check.h

DrahtBot · 2022-04-11T01:50:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24455 (refactor: Split ArgsManager out of util/system by Empact)
#22341 (rpc: add getxpub by Sjors)
#19792 (rpc: Add dumpcoinstats by fjahr)

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/wallet/rpc/backup.cpp

vincenzopalazzo

LGTM, but I'm not a big fan of while(0) in C++, maybe C in yes

src/util/check.h

maflcko

lgtm

src/util/check.h

aureleoules · 2022-04-12T11:49:29Z

I've addressed the nits, thanks for your reviews.

vincenzopalazzo

ACK 1e4d382

Good job!

jonatack

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.

src/util/check.h

jonatack · 2022-04-13T14:24:02Z

re-ACK 63b753416abddb5f82ed32ff64a3372e65722fc2

maflcko · 2022-04-15T11:59:25Z

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.

src/util/check.h

…E AND UNREACHABLE macros

jonatack · 2022-04-18T10:55:26Z

ACK ee02c8b

maflcko

Looks like this also fixes a warning with gcc-12.

ACK ee02c8b 🍨

Show signature

Signature:

maflcko · 2022-04-24T09:37:08Z

src/rpc/util.cpp

    case Type::ANY: {
-        CHECK_NONFATAL(false); // Only for testing
+        NONFATAL_UNREACHABLE(); // Only for testing
    }


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: { | ^~~~

maflcko · 2022-04-24T09:53:20Z

src/util/check.h

+template <typename T>
+T&& inline_check_non_fatal(T&& val, const char* file, int line, const char* func, const char* assertion)
+{
+    if (!(val)) {


Suggested change

if (!(val)) {

if (!val) {

nit: No need for ().

maflcko · 2022-04-24T09:53:30Z

src/util/check.h

+        throw NonFatalCheckError(
+            format_internal_error(assertion, file, line, func, PACKAGE_BUGREPORT));
+    }
+


nit: no need for newline

… and NO…

…CHABLE macro Summary: This is a backport of [[bitcoin/bitcoin#24812 | core#24812]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13096

aureleoules force-pushed the check_non_fatal_identity branch 2 times, most recently from 70bff71 to 5eeb1e5 Compare April 9, 2022 00:14

DrahtBot added RPC/REST/ZMQ Utils/log/libs labels Apr 9, 2022

maflcko reviewed Apr 9, 2022

View reviewed changes

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

aureleoules force-pushed the check_non_fatal_identity branch 3 times, most recently from ebdd846 to 0661baf Compare April 10, 2022 00:24

aureleoules changed the title ~~util/check: Add CheckNonFatal identity function and use it in src/rpc~~ util/check: Replace CHECK_NONFATAL macro with identity function and use it in src/rpc Apr 10, 2022

bitcoin deleted a comment from joshuadbryant1985 Apr 10, 2022

maflcko reviewed Apr 11, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 11, 2022

Improve Indices on pruned nodes via prune blockers #21726

Merged

aureleoules force-pushed the check_non_fatal_identity branch from 0661baf to 3665422 Compare April 12, 2022 04:40

vincenzopalazzo suggested changes Apr 12, 2022

View reviewed changes

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

maflcko approved these changes Apr 12, 2022

View reviewed changes

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

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

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

aureleoules force-pushed the check_non_fatal_identity branch from 3665422 to 1e4d382 Compare April 12, 2022 11:46

aureleoules changed the title ~~util/check: Replace CHECK_NONFATAL macro with identity function and use it in src/rpc~~ util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros Apr 12, 2022

vincenzopalazzo approved these changes Apr 12, 2022

View reviewed changes

DrahtBot mentioned this pull request Apr 13, 2022

add static_check_equal for easier to read compiler errors #19531

Closed

jonatack reviewed Apr 13, 2022

View reviewed changes

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

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

jonatack reviewed Apr 13, 2022

View reviewed changes

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

aureleoules force-pushed the check_non_fatal_identity branch 2 times, most recently from f9cffe2 to 63b7534 Compare April 13, 2022 14:13

This was referenced Apr 13, 2022

refactor: Split ArgsManager out of util/system #24455

Closed

rpc: add path to gethdkey #22341

Closed

rpc: Add dumpcoinstats #19792

Closed

maflcko reviewed Apr 16, 2022

View reviewed changes

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

util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABL…

ee02c8b

…E AND UNREACHABLE macros

aureleoules force-pushed the check_non_fatal_identity branch from 63b7534 to ee02c8b Compare April 16, 2022 13:08

aureleoules changed the title ~~util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros~~ util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro Apr 16, 2022

maflcko approved these changes Apr 24, 2022

View reviewed changes

maflcko merged commit b1c5991 into bitcoin:master Apr 24, 2022

aureleoules mentioned this pull request Apr 25, 2022

Sanity assert GetAncestor() != nullptr where appropriate #24804

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 26, 2022

Merge bitcoin#24812: util/check: Add CHECK_NONFATAL identity function…

51ba4ab

… and NO…

aureleoules deleted the check_non_fatal_identity branch May 20, 2022 22:14

maflcko mentioned this pull request Jun 3, 2022

[kernel 2c/n] Introduce kernel::Context, encapsulate global init/teardown #25065

Merged

maflcko mentioned this pull request Sep 14, 2022

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

Merged

This was referenced Nov 15, 2022

Add UNREACHABLE macro #26504

Closed

util: Move error message formatting of NonFatalCheckError to cpp #25112

Merged

bitcoin locked and limited conversation to collaborators May 20, 2023

Conversation

aureleoules commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aureleoules commented Apr 12, 2022

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 13, 2022

Uh oh!

maflcko commented Apr 15, 2022

Uh oh!

Uh oh!

jonatack commented Apr 18, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aureleoules commented Apr 8, 2022 •

edited

Loading

DrahtBot commented Apr 11, 2022 •

edited

Loading