From 1db98906821c86bb71e87d2a0b52bbdd0ff5b8ec Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 22 Oct 2024 18:39:10 +0100 Subject: [PATCH] Minor fixes --- include/xrpl/beast/utility/instrumentation.h | 23 ++++++++++++-------- src/libxrpl/basics/contract.cpp | 3 +++ src/libxrpl/json/Object.cpp | 2 +- src/libxrpl/json/json_reader.cpp | 2 +- src/libxrpl/protocol/Indexes.cpp | 2 +- src/xrpld/app/paths/detail/StrandFlow.h | 2 +- src/xrpld/app/tx/detail/OfferStream.cpp | 2 +- src/xrpld/app/tx/detail/SetSignerList.cpp | 3 +-- 8 files changed, 23 insertions(+), 16 deletions(-) diff --git a/include/xrpl/beast/utility/instrumentation.h b/include/xrpl/beast/utility/instrumentation.h index 9dfb5bc89f8..c35f6f2cfa1 100644 --- a/include/xrpl/beast/utility/instrumentation.h +++ b/include/xrpl/beast/utility/instrumentation.h @@ -37,22 +37,27 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #define ASSERT ALWAYS_OR_UNREACHABLE -// How to use the above macros: +// How to use the instrumentation macros: // -// ALWAYS if it must be reached during fuzzing -// ASSERT if you expect that it might, or might not, be reached during fuzzing -// REACHABLE if it must be reached during fuzzing -// SOMETIMES a hint for the fuzzer to try to make the condition true -// UNREACHABLE if it must not be reached (in fuzzing or in normal use) +// ALWAYS if cond must be true and the line must be reached during fuzzing +// ASSERT if cond must be true but the line might not be reached during fuzzing +// REACHABLE if the line must be reached during fuzzing +// SOMETIMES a hint for the fuzzer to try to make the cond true +// UNREACHABLE if the line must not be reached (in fuzzing or in normal use) // -// NOTE: ASSERT has similar semantics as assert macro, with minor differences: +// NOTE: ASSERT has similar semantics as C assert macro, with minor differences: // * ASSERT must have an unique name (naming convention in CONTRIBUTING.md) // * the condition (which comes first) must be *implicitly* convertible to bool // * during fuzzing, the program will continue execution past a failed ASSERT -// We continue to use assert inside unit tests and in constexpr functions. +// +// We continue to use regular C assert inside unit tests and inside constexpr +// functions. // // NOTE: UNREACHABLE does *not* have the same semantics as std::unreachable. // The program will continue execution past an UNREACHABLE in a Release build -// and during fuzzing (similar to ASSERT). +// and during fuzzing (similar to ASSERT). Also the naming convention is subtly +// different from all other macros - name in UNREACHABLE describes the condition +// which was meant to never happen, while name in other macros describe the +// condition that is meant to happen (e.g. as in "assert that this happens"). #endif diff --git a/src/libxrpl/basics/contract.cpp b/src/libxrpl/basics/contract.cpp index 249c589ff83..b26058be537 100644 --- a/src/libxrpl/basics/contract.cpp +++ b/src/libxrpl/basics/contract.cpp @@ -49,6 +49,9 @@ LogicError(std::string const& s) noexcept { JLOG(debugLog().fatal()) << s; std::cerr << "Logic error: " << s << std::endl; + // Use a non-standard contract naming here (without namespace), because + // it's the only location where various unrelated execution paths may + // register an error; this is also why "message" parameter is passed here. UNREACHABLE("LogicError", {{"message", s}}); detail::accessViolation(); } diff --git a/src/libxrpl/json/Object.cpp b/src/libxrpl/json/Object.cpp index 2ecc64e7bb4..d38132b1f21 100644 --- a/src/libxrpl/json/Object.cpp +++ b/src/libxrpl/json/Object.cpp @@ -168,7 +168,7 @@ Array::append(Json::Value const& v) return; } } - UNREACHABLE("Json::Array::append : invalid type"); + UNREACHABLE("Json::Array::append : invalid type"); } void diff --git a/src/libxrpl/json/json_reader.cpp b/src/libxrpl/json/json_reader.cpp index b8744ba8ea6..3e445ab5cc8 100644 --- a/src/libxrpl/json/json_reader.cpp +++ b/src/libxrpl/json/json_reader.cpp @@ -953,7 +953,7 @@ operator>>(std::istream& sin, Value& root) Json::Reader reader; bool ok = reader.parse(sin, root); - // ASSERT( ok ); + // ASSERT(ok, "Json::operator>>() : parse succeeded"); if (!ok) ripple::Throw(reader.getFormatedErrorMessages()); diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index bf922ef04a7..bbd0cabac79 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -208,7 +208,7 @@ line( // There is code in SetTrust that calls us with id0 == id1, to allow users // to locate and delete such "weird" trustlines. If we remove that code, we // could enable this assert: - // ASSERT(id0 != id1); + // ASSERT(id0 != id1, "ripple::keylet::line : accounts must be different"); // A trust line is shared between two accounts; while we typically think // of this as an "issuer" and a "holder" the relationship is actually fully diff --git a/src/xrpld/app/paths/detail/StrandFlow.h b/src/xrpld/app/paths/detail/StrandFlow.h index 3b2fdadcf77..72ca97a42ac 100644 --- a/src/xrpld/app/paths/detail/StrandFlow.h +++ b/src/xrpld/app/paths/detail/StrandFlow.h @@ -847,7 +847,7 @@ flow( // running debug builds of rippled. While this issue still needs to // be resolved, the assert is causing more harm than good at this // point. - // UNREACHABLE(); + // UNREACHABLE("ripple::flow : rounding error"); return {tefEXCEPTION, std::move(ofrsToRmOnFail)}; } diff --git a/src/xrpld/app/tx/detail/OfferStream.cpp b/src/xrpld/app/tx/detail/OfferStream.cpp index f987be501bb..2fb9ad6a143 100644 --- a/src/xrpld/app/tx/detail/OfferStream.cpp +++ b/src/xrpld/app/tx/detail/OfferStream.cpp @@ -342,7 +342,7 @@ TOfferStreamBase::step() } UNREACHABLE( "rippls::TOfferStreamBase::step::rmSmallIncreasedQOffer : XRP " - "vs XRP offer"); // xrp/xrp offer!?! should never happen + "vs XRP offer"); return false; }(); diff --git a/src/xrpld/app/tx/detail/SetSignerList.cpp b/src/xrpld/app/tx/detail/SetSignerList.cpp index 542d10f6e4e..9c4fcd0e6c1 100644 --- a/src/xrpld/app/tx/detail/SetSignerList.cpp +++ b/src/xrpld/app/tx/detail/SetSignerList.cpp @@ -128,8 +128,7 @@ SetSignerList::doApply() default: break; } - UNREACHABLE( - "ripple::SetSignerList::doApply : invalid operation"); + UNREACHABLE("ripple::SetSignerList::doApply : invalid operation"); return temMALFORMED; }