diff --git a/CHANGELOG.md b/CHANGELOG.md index d31d268a..bffc2ca5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ API surface). ## [Unreleased] +### Fixed + +- **`Rational` no longer invokes undefined behaviour on `INT64_MIN`.** The whole-integer constructor now canonicalises like the other constructors, and unary negation, `abs`, `reciprocal`, and multiplication cross-cancellation remain defined even if the public `numerator` or `denominator` is manually set to `INT64_MIN`. Unrepresentable magnitude is clamped to `INT64_MAX`, matching the existing saturation policy. `reciprocal` hands the inverted pair to the canonicalising constructor rather than negating a component itself, so the clamp is reported through the usual `error` log instead of applied silently. +- **`checkedMul` no longer reports success for a product canonicalisation then changes.** A reduced product of exactly `INT64_MIN` fits an `int64_t` but is not a representable `Rational` component — `canonicalise` clamps it to `-INT64_MAX` — so `mulWouldOverflow` now reports it. `checkedMul(Rational{-2^62}, Rational{2})` returns `Overflow` where it previously returned a clamped value as a success; `operator*` saturates to the same `-INT64_MAX/1` it produced before, under the overflow log rather than the clamp log. +- **`checkedDiv` no longer absorbs an inexact reciprocal.** A divisor carrying a hand-poisoned `INT64_MIN` component has no representable inverse, so `checkedDiv` reports `Overflow` instead of returning the product of the clamped one as a success. `dividedBy`/`operator/` are unchanged and still saturate. + ### Changed - **`LocalBackend::execute` no longer rescans the pending-completion list on diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 5056487d..78be56dc 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -256,11 +256,12 @@ a value reads identically everywhere. There is a single formatting path and in-code references to one are references to `std::formatter`. `formatRationalDecimal` takes the numerator's magnitude through -`math::detail::absU64`, in unsigned arithmetic. Negating in `int64_t` instead -is undefined for `INT64_MIN`, and that value is reachable here: the -whole-integer `Rational{value, DecimalPlaces{n}}` constructor does not -canonicalise, so the clamp that would otherwise remove the trap value never -runs. +`math::detail::absU64`, in unsigned arithmetic. It negated in `int64_t` until +morph#496, which is undefined for `INT64_MIN`. Since morph#537 every `Rational` +constructor canonicalises — including the whole-integer +`Rational{value, DecimalPlaces{n}}` one — so no *constructed* value carries the +trap value. `numerator` is still a public member, so one can be assigned +directly, and the formatter stays defined for that. **The decimal form.** `formatRationalDecimal` renders the exact `Rational` as a fixed decimal at its **runtime `DecimalPlaces`** and then trims trailing zeros diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index ebc4ac39..4bf732e5 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -93,8 +93,8 @@ the canonical `(numerator, denominator)` pair and ignores `decimalPlaces`. |---|---|---| | `operator+`, `operator-`, `operator*` (plain `Rational` × `Rational`) | `Rational` | `noexcept`, return a bare `Rational` — no error channel. This means *representable* results never fail; it does **not** mean the operation cannot go wrong. Reduced int64 cross-terms exceeding ~2^63 **saturate** at `±INT64_MAX/1` and log at `error`; the result is clamped and inexact, and the return type does not say so (see [Overflow & value-range envelope](#overflow--value-range-envelope)). Reduce-before-multiply (Knuth 4.5.1) to extend safe int64 range. Cross-cancellation before multiplication. | | `operator/`, `dividedBy` (plain `Rational` ÷ `Rational`) | `expected` | `DivisionByZero` when divisor's numerator is zero — and **that is the only error it reports.** Implemented by multiplying `*this` by the reciprocal (`den/num`, sign carried onto the numerator), so it **also propagates `max` precision**, and so it **saturates on overflow exactly like `operator*`**: an out-of-envelope quotient clamps to `±INT64_MAX/1`, logs at `error`, and is still returned as a *successful* `expected`. Use `checkedDiv` to have that reported. | -| `operator-` (unary) | `Rational` | Negates numerator. Precision preserved. **Negating `INT64_MIN` overflows.** | -| `reciprocal` | `expected` | Multiplicative inverse. `DivisionByZero` when the value is zero. **Precision is the operand's own `decimalPlaces`, not `max`** (it is a unary operation with no second operand to widen against). | +| `operator-` (unary) | `Rational` | Negates numerator. Precision preserved. A hand-poisoned `INT64_MIN` numerator **clamps** to `INT64_MAX` rather than overflowing. | +| `reciprocal` | `expected` | Multiplicative inverse. `DivisionByZero` when the value is zero. **Precision is the operand's own `decimalPlaces`, not `max`** (it is a unary operation with no second operand to widen against). The inverted pair goes through the canonicalising constructor, which carries the sign and clamps a hand-poisoned `INT64_MIN` component; nothing is negated here. | | `operator+=`, `-=`, `*=` (in-place) | `Rational&` | Mutate `*this`, widen precision to `max`, canonicalise. | ## Overflow & value-range envelope @@ -177,37 +177,24 @@ result and `llround` maps `(-2^63 - 0.5, -2^63]` onto it. On x86's 80-bit a poisoned `INT64_MIN` numerator); where `long double == double` the same literal rounds past the bound and is rejected by the plain `2^63` check. -**`INT64_MIN` negation hazards.** `INT64_MIN` (`-2^63`) has no positive -counterpart in `int64`, so every place that negates a component is a latent UB -site when that exact value reaches it: - -- **unary `operator-`** — `Rational{Numerator{-numerator}, ...}`: negating an - `INT64_MIN` numerator overflows. -- **`from`** — guards **only** `denominator == 0`; it does not screen - `INT64_MIN` components, so a hostile-but-nonzero `(INT64_MIN, …)` pair flows - straight into the canonicalising constructor. -- **`reciprocal`** — negates the numerator in the `numerator < 0` branch; - `INT64_MIN` there overflows. -- **Rendering** (`morph::units::detail::formatRationalDecimal`) — **not one of - these.** It takes the numerator's magnitude through `detail::absU64`, which - negates in unsigned arithmetic. Negating in `int64_t` there is UB that UBSan - catches, and it is reachable: the whole-integer - `Rational{value, DecimalPlaces{n}}` constructor does not canonicalise, so the - clamp never runs on that path, and `numerator` is public. -- **`canonicalise`** — **not one of these either.** It clamps an `INT64_MIN` - numerator to `-INT64_MAX` (with an `error`-level log, `reportClamp`) *before* - any sign flip, and computes the gcd through `detail::absU64`, which negates in - unsigned arithmetic. Since it is the shared sink for every constructor and - operator, a value that reaches it is safe. - -The wire codec (`setWire`) also defends independently: it maps an `INT64_MIN` -`num`/`den` to `-INT64_MAX` *before* constructing, so untrusted input never -reaches the trap value at all. - -The entry points that do **not** canonicalise are where the hazard remains — the -whole-integer `Rational{value, DecimalPlaces{n}}` constructor retains its -numerator verbatim, and `numerator` is a public member. A UB site reached that -way is a confirmed, not a hypothetical, shape. +**`INT64_MIN` handling.** `INT64_MIN` (`-2^63`) has no positive counterpart +in `int64`, so direct signed negation is undefined. The whole-integer +constructor therefore delegates to the canonicalising constructor and clamps +`INT64_MIN` to `-INT64_MAX`, matching the full constructor and wire path. + +The public `numerator` member can still be assigned `INT64_MIN` manually, so +operations that may observe such a value defend independently: unary negation, +`abs`, `reciprocal`, and multiplication overflow/cross-cancellation use either +an explicit saturating branch or `detail::absU64`, which computes magnitude in +unsigned arithmetic. These operations remain defined even for a manually +poisoned value; where the exact magnitude is unrepresentable they clamp to the +adjacent `INT64_MAX` magnitude and preserve the existing error/saturation +policy. + +The wire codec (`setWire`) likewise maps an `INT64_MIN` `num`/`den` to +`-INT64_MAX` before constructing, so untrusted input never reaches a signed +negation trap. + ### Checked arithmetic @@ -266,7 +253,11 @@ there is no valid answer to inspect. `checkedMul` checks the *cross-cancelled* factors `operator*` actually multiplies, not the raw operands: cross-cancelling is what keeps most products in range, so checking beforehand would reject pairs that multiply perfectly -well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). +well (`INT64_MAX/2 * 2/1` reduces to `INT64_MAX/1`). A reduced product of +exactly `INT64_MIN` counts as an overflow even though it fits an `int64_t`: +`canonicalise` clamps such a component to `-INT64_MAX`, so reporting success +would hand back a value canonicalisation has already changed. `(-2^62) * 2` is +the shortest case, and needs no poisoned operand. `checkedDiv` is the division member of the family, and it exists because division was the one operation with no exact-or-nothing form: `dividedBy` @@ -275,7 +266,17 @@ checks *its* result is told a clamped quotient succeeded. `checkedDiv` is `checkedMul` against `rhs.reciprocal()` — the same operand pair `dividedBy` forms internally — and it folds both failure modes into the one channel: `DivisionByZero` propagated from `reciprocal`, `Overflow` from `checkedMul`. -`dividedBy` itself saturates: `Quantity` already folds a +It also reports `Overflow` for a divisor carrying a hand-poisoned `INT64_MIN` +component, *before* forming the reciprocal: that component's `2^63` magnitude +has no `int64` counterpart, so `reciprocal` returns a clamped value that is not +the divisor's inverse, and the product of it would be an inexact success. This +is conservative rather than exact — `2 / (INT64_MIN/1)` has the representable +exact quotient `-1/2^62` — and deliberately so: reaching it would require +carrying the unsigned magnitude through the cross-cancellation rather than going +through `reciprocal`, which would let `checkedDiv` accept quotients `dividedBy` +still saturates, breaking the one-set-of-predicates property below. No value the +type can construct is affected. +`dividedBy` itself is unchanged and still saturates: `Quantity` already folds a failed division to `nullopt` (`docs/spec/error_handling.md`), and making `/` the sole operation that refuses to saturate would impose "overflow is fatal" on every caller, in-tree and out. @@ -491,7 +492,7 @@ through `setWire`. | `checkedAdd(a, b)` | `constexpr expected noexcept` — exact sum, or `Overflow`. | | `checkedSub(a, b)` | `constexpr expected noexcept` — exact difference, or `Overflow`. | | `checkedMul(a, b)` | `constexpr expected noexcept` — exact product, or `Overflow`. | -| `checkedDiv(a, b)` | `constexpr expected noexcept` — exact quotient, or `DivisionByZero`, or `Overflow`. `checkedMul` against `b.reciprocal()`; the form `dividedBy`/`operator/` do not provide, since those saturate and report success. | +| `checkedDiv(a, b)` | `constexpr expected noexcept` — exact quotient, or `DivisionByZero`, or `Overflow` (including when `b` carries a poisoned `INT64_MIN` component, whose reciprocal is not representable). `checkedMul` against `b.reciprocal()`; the form `dividedBy`/`operator/` do not provide, since those saturate and report success. | | `setWire(Wire)` | `void noexcept` — rebuilds through the canonicalising constructor, clamping what it cannot represent and counting the clamp. | | `Wire::validate()` | `constexpr bool noexcept` — whether these raw values decode without being clamped. | | `WireClampScope` | Scoped observer: how many `Rational` values were clamped while decoding. | @@ -535,7 +536,7 @@ expected operator+(Left const&, Right const&) noexcept; | Rounding mode is a parameter, defaulting to half away from zero | **`RoundingMode{HalfAwayFromZero, HalfEven}`, `HalfAwayFromZero` default** | A mode had to become visible once rounding became a *storage* operation rather than an implementation detail of display. The default follows morph's own formatter rather than the standards' `HALF_EVEN`, because the point of rounding on the dispatch path is that the stored value equals the displayed one; a `HalfEven` default would break that for every tie until the formatter learned the same mode. | | No `checkedRound` | **`roundToDecimalPlaces` saturates and logs** | Consistent with `+`/`-`/`*`/`/`: the `checked*` family covers the operations a caller is likely to drive with unbounded inputs. Rounding a value already representable at the target scale — the overwhelmingly common case, and every integer — takes a fast path that cannot overflow at all. | | 128-bit cross-product comparison | **`detail::mulU64`** | Exact ordering over the full int64 range without overflow. Uses `unsigned __int128` when available (GCC/Clang), portable 32-bit limb decomposition on MSVC. | -| Negation limitation | **`INT64_MIN` overflows** | Documented limitation. The wire codec clamps `INT64_MIN` components away for untrusted input. | +| `INT64_MIN` is not a component | **Clamped to `-INT64_MAX`, with an `error` log** | `-INT64_MIN` is not representable, so canonicalising it is undefined. Every constructor canonicalises, so no constructed value carries it; the members are public, so the operations that could still observe one clamp instead of negating. The wire codec clamps it independently for untrusted input. | | `fromFloat` not `constexpr` | **Uses `std::llround` / `std::isfinite`** | These standard library functions are not `constexpr`. The `fromFloat` overloads are `inline` out-of-class, `noexcept` but not `constexpr`. | ## Payload shape tag diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index ea4497c9..39e7e98b 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -106,10 +106,11 @@ namespace detail { // invariant guarantees denominator > 0, so only the numerator carries sign. bool const negative = value.numerator < 0; // Negate in unsigned arithmetic: `-INT64_MIN` is undefined as a signed - // operation, and INT64_MIN reaches here through the whole-integer - // `Rational{value, DecimalPlaces{n}}` constructor, which does not - // canonicalise (and `numerator` is public). `absU64` is the shared helper - // that gets this right. + // operation. Every Rational constructor now canonicalises, so no + // constructed value carries INT64_MIN -- but `numerator` is a public + // member, so a caller can still assign one, and a formatter must stay + // defined for whatever it is handed. `absU64` is the shared helper that + // gets this right -- see morph#496 and morph#537. auto const num = ::morph::math::detail::absU64(value.numerator); auto const den = static_cast(value.denominator); auto const places = static_cast(value.decimalPlaces.value); diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index b0d1705c..7f308d62 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -86,9 +86,14 @@ /// constructor, so a non-canonical payload (`1234/100`) or a hostile one /// (`den == 0`, out-of-range `dp`) always lands as a valid, reduced value. /// -/// @note Negating a `Rational` built from `INT64_MIN` overflows; avoid that -/// extreme value in code (the wire codec clamps it away for untrusted -/// input). +/// @note `INT64_MIN` is never a canonical component. It has no positive +/// counterpart in `int64`, so `canonicalise` -- which every constructor +/// runs, including the whole-integer one -- clamps such a component to +/// `-INT64_MAX` and logs at `error`. The members are public, so a caller +/// can still assign `INT64_MIN` by hand; the operations that may observe +/// one (unary `-`, `abs`, `reciprocal`, multiplication and its overflow +/// predicate) stay defined and clamp its magnitude rather than negating +/// it. The wire codec clamps it independently for untrusted input. /// @note Comparison is exact over the full int64 range (128-bit cross /// products). Arithmetic, however, has the usual fixed-width envelope: /// `+`, `-`, `*` overflow int64 when reduced cross terms exceed ~2^63 @@ -386,10 +391,14 @@ struct Rational { constexpr Rational() noexcept = default; /// @brief Constructs from a whole integer at the given precision. + /// + /// Delegates to the canonicalising constructor, so this path applies the + /// same clamps as every other: an `INT64_MIN` @p whole is stored as + /// `-INT64_MAX/1` with an `error` log rather than kept verbatim. /// @param whole The integer value; stored as `whole/1`. /// @param wantedPrecision Decimal precision; clamped to [0, kMaxDecimalPlaces]. constexpr Rational(std::int64_t whole, DecimalPlaces wantedPrecision) noexcept - : numerator{whole}, decimalPlaces{detail::clampDecimalPlaces(wantedPrecision.value)} {} + : Rational{Numerator{whole}, Denominator{1}, wantedPrecision} {} /// @brief Constructs from explicit numerator/denominator, then canonicalises. /// @@ -483,21 +492,32 @@ struct Rational { /// @return The rounded floating-point reading. [[nodiscard]] double toDouble(std::uint32_t requestedDecimalPlaces) const noexcept; - /// @brief Negates. @note Negating a Rational built from `INT64_MIN` overflows. + /// @brief Negates, clamping an `INT64_MIN` numerator to `INT64_MAX`. /// @return The value with the numerator's sign flipped. [[nodiscard]] constexpr Rational operator-() const noexcept { - return Rational{Numerator{-numerator}, Denominator{denominator}, decimalPlaces}; + constexpr auto minValue = std::numeric_limits::min(); + constexpr auto maxValue = std::numeric_limits::max(); + auto const negated = numerator == minValue ? maxValue : -numerator; + return Rational{Numerator{negated}, Denominator{denominator}, decimalPlaces}; } /// @brief Multiplicative inverse. + /// + /// The inverted pair is handed straight to the canonicalising constructor + /// rather than sign-corrected here: `canonicalise` already moves a negative + /// denominator's sign onto the numerator, and it clamps an `INT64_MIN` + /// component (with an `error` log) *before* the flip. Negating either + /// component here instead would be undefined for a hand-poisoned + /// `INT64_MIN` in that position -- which is how this function reached + /// `-denominator` unguarded. + /// + /// A poisoned component therefore yields a clamped, inexact inverse. That + /// is the type's saturation policy, and `checkedDiv` declines to absorb it. /// @return `denominator/numerator`, or `unexpected(DivisionByZero)` if zero. [[nodiscard]] constexpr std::expected reciprocal() const noexcept { if (numerator == 0) { return std::unexpected(RationalError::DivisionByZero); } - if (numerator < 0) { - return Rational{Numerator{-denominator}, Denominator{-numerator}, decimalPlaces}; - } return Rational{Numerator{denominator}, Denominator{numerator}, decimalPlaces}; } @@ -828,14 +848,16 @@ struct Rational { /// @brief `operator*=`'s arithmetic, without the overflow check. /// @param rhs Value to multiply by. constexpr void mulAssignUnchecked(const Rational& rhs) noexcept { - auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); - auto const reducedLeftNumerator = numerator / crossDivisorOne; - auto const reducedRightNumerator = rhs.numerator / crossDivisorTwo; - auto const reducedLeftDenominator = denominator / crossDivisorTwo; - auto const reducedRightDenominator = rhs.denominator / crossDivisorOne; + auto const absoluteLeftNumerator = detail::absU64(numerator); + auto const absoluteRightNumerator = detail::absU64(rhs.numerator); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, detail::absU64(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, detail::absU64(denominator)); + auto const signedCrossDivisorOne = static_cast(crossDivisorOne); + auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); + auto const reducedLeftNumerator = numerator / signedCrossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / signedCrossDivisorTwo; + auto const reducedLeftDenominator = denominator / signedCrossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / signedCrossDivisorOne; numerator = reducedLeftNumerator * reducedRightNumerator; denominator = reducedLeftDenominator * reducedRightDenominator; widenPrecisionTo(rhs.decimalPlaces); @@ -890,18 +912,38 @@ struct Rational { /// Checks the cross-cancelled factors `operator*=` actually multiplies: /// cross-cancelling is what keeps most products in range, so checking the /// raw operands would reject pairs that multiply perfectly well. + /// + /// "Representable" is stricter than "fits an `int64_t`": `INT64_MIN` fits + /// and is still not a canonical component, because `canonicalise` clamps + /// it to `-INT64_MAX`. A product landing exactly there is reported here as + /// an overflow, so `checkedMul` cannot return a success that + /// canonicalisation has already altered -- `(-2^62) * 2` is the shortest + /// example, and needs no poisoned operand at all. /// @param rhs The factor. /// @return `true` if the product cannot be represented. [[nodiscard]] constexpr bool mulWouldOverflow(const Rational& rhs) const noexcept { - auto const absoluteLeftNumerator = numerator < 0 ? -numerator : numerator; - auto const absoluteRightNumerator = rhs.numerator < 0 ? -rhs.numerator : rhs.numerator; - auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, rhs.denominator); - auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, denominator); + auto const absoluteLeftNumerator = detail::absU64(numerator); + auto const absoluteRightNumerator = detail::absU64(rhs.numerator); + auto const crossDivisorOne = std::gcd(absoluteLeftNumerator, detail::absU64(rhs.denominator)); + auto const crossDivisorTwo = std::gcd(absoluteRightNumerator, detail::absU64(denominator)); if (crossDivisorOne == 0 || crossDivisorTwo == 0) { return false; // a zero numerator: the product is zero } - return detail::mulOverflows(numerator / crossDivisorOne, rhs.numerator / crossDivisorTwo) || - detail::mulOverflows(denominator / crossDivisorTwo, rhs.denominator / crossDivisorOne); + auto const signedCrossDivisorOne = static_cast(crossDivisorOne); + auto const signedCrossDivisorTwo = static_cast(crossDivisorTwo); + auto const reducedLeftNumerator = numerator / signedCrossDivisorOne; + auto const reducedRightNumerator = rhs.numerator / signedCrossDivisorTwo; + auto const reducedLeftDenominator = denominator / signedCrossDivisorTwo; + auto const reducedRightDenominator = rhs.denominator / signedCrossDivisorOne; + if (detail::mulOverflows(reducedLeftNumerator, reducedRightNumerator) || + detail::mulOverflows(reducedLeftDenominator, reducedRightDenominator)) { + return true; + } + // Forming the products is safe now, and only now: the checks above + // have just proved neither overflows. + constexpr auto minValue = std::numeric_limits::min(); + return reducedLeftNumerator * reducedRightNumerator == minValue || + reducedLeftDenominator * reducedRightDenominator == minValue; } private: @@ -968,9 +1010,14 @@ static_assert(std::is_standard_layout_v); /// @param value Value to take the absolute value of. /// @return The non-negative value with the same magnitude. [[nodiscard]] constexpr Rational abs(const Rational& value) noexcept { - return value.numerator < 0 - ? Rational{Numerator{-value.numerator}, Denominator{value.denominator}, value.decimalPlaces} - : value; + if (value.numerator >= 0) { + return value; + } + constexpr auto maxValue = std::numeric_limits::max(); + auto const magnitude = detail::absU64(value.numerator); + auto const clampedMagnitude = + magnitude > static_cast(maxValue) ? maxValue : static_cast(magnitude); + return Rational{Numerator{clampedMagnitude}, Denominator{value.denominator}, value.decimalPlaces}; } /// @brief Rounds toward positive infinity. @@ -1150,13 +1197,40 @@ static_assert(std::is_standard_layout_v); /// predicate the operator uses. The operators and the checked forms therefore /// cannot disagree about which quotients fit. /// +/// A divisor carrying a hand-poisoned `INT64_MIN` component is reported as +/// `Overflow` before the reciprocal is formed. `reciprocal()` has to clamp such +/// a component's `2^63` magnitude to `INT64_MAX`, so the reciprocal it returns +/// is not the divisor's inverse, and multiplying by it would produce an +/// *inexact* value this function would otherwise hand back as a success. This +/// is the conservative side of the contract rather than the exact one: for some +/// dividends -- `2 / (INT64_MIN/1)`, whose exact quotient is `-1/2^62` -- the +/// answer is representable even though the reciprocal is not, and reaching it +/// would mean carrying the unsigned magnitude through the cross-cancellation +/// instead of going through `reciprocal()` at all. That would make `checkedDiv` +/// accept quotients `dividedBy` still saturates, which is the one thing the +/// paragraph above promises it will not do. It costs nothing for any value the +/// type can actually produce: every constructor canonicalises, so `INT64_MIN` +/// only ever reaches a component by direct assignment. +/// /// @param lhs Dividend. /// @param rhs Divisor. /// @return The exact quotient; `unexpected(RationalError::DivisionByZero)` if /// @p rhs is zero; `unexpected(RationalError::Overflow)` if the -/// quotient is not representable. +/// quotient, or the divisor's reciprocal, is not representable. [[nodiscard]] constexpr std::expected checkedDiv(const Rational& lhs, const Rational& rhs) noexcept { + // The zero divisor is reported ahead of everything else, as it is by + // `reciprocal()` itself. + if (rhs.numerator == 0) { + return std::unexpected(RationalError::DivisionByZero); + } + // A poisoned component makes `reciprocal()` clamp, so what it returns is + // not this divisor's inverse and the product below would be inexact. + // Decline rather than hand an inexact value back as a success. + constexpr auto minValue = std::numeric_limits::min(); + if (rhs.numerator == minValue || rhs.denominator == minValue) { + return std::unexpected(RationalError::Overflow); + } auto const divisorReciprocal = rhs.reciprocal(); if (!divisorReciprocal.has_value()) { return std::unexpected(divisorReciprocal.error()); diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index d54bfc85..80fe9853 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -41,23 +41,23 @@ "entries": [ { "file": "include/morph/util/rational.hpp", - "line": 271, + "line": 276, "source": "assert(rawDecimalPlaces <= kMaxDecimalPlaces);", "reason": "assert()'s false arm calls abort(). Reaching it means violating the precondition and terminating the process, so no test in a Catch2 binary can observe it and still report; a death test would prove only that assert() works. The decision the assert guards is checked by the caller-side tests that stay inside kMaxDecimalPlaces." }, { "file": "include/morph/util/rational.hpp", - "line": 697, + "line": 717, "source": "if (!std::is_constant_evaluated()) {", "context": "/// @brief Logs an overflow that `operator+=`/`-=`/`*=` saturated instead", "reason": "The false arm is the constant-evaluated one, and constant evaluation does not execute instrumented code -- no counter can increment during it. So in every run this branch is true, structurally, and its other arm is not a gap in the tests but a gap in what runtime instrumentation can see. reportOverflow's constexpr behaviour is exercised by the constexpr Rational arithmetic in tests/test_rational.cpp; what cannot be exercised is llvm-cov's counter." }, { "file": "include/morph/util/rational.hpp", - "line": 731, + "line": 751, "source": "if (!std::is_constant_evaluated()) {", "context": "[[nodiscard]] constexpr int compareForSaturation(const Rational& rhs, bool addition) const noexcept {", - "reason": "reportClamp's copy of the same shape as line 697 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." + "reason": "reportClamp's copy of the same shape as line 717 above, and uncoverable for the same reason: the untaken arm is constant evaluation, which increments no counters." }, { "file": "include/morph/util/datetime.hpp", @@ -67,31 +67,31 @@ }, { "file": "include/morph/util/rational.hpp", - "line": 757, + "line": 777, "source": "return std::is_gt(ordering) ? 1 : 0;", - "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 775 below." + "reason": "The false arm (a tie, returning 0) is unreachable. compareForSaturation has exactly two call sites, both from the saturating branch of operator+=/operator-=, and both are guarded by addWouldOverflow/subWouldOverflow having just returned true. A tie in compareForSaturation (*this == -rhs for +=, or *this == rhs for -=) requires -- since operator== compares canonical numerator/denominator pairs directly -- identical denominators. With equal denominators, addWouldOverflow/subWouldOverflow's own scaling factors collapse to rightScaled == leftScaled == 1 (gcd(D, D) == D), so the cross-multiplication checks reduce to numerator +/- rhs.numerator directly on already-canonical (so already in-range) int64_t values whose sum/difference is provably 0 -- never an overflow (and canonicalise() already forbids either component from being INT64_MIN, so negating one to check the other is always representable). So whenever the two operands could produce a tie, the overflow predicate that gates the call to compareForSaturation is always false, and the saturating path -- hence compareForSaturation's tie return -- is never reached with a tied pair. Shares this root cause with saturateToward's sign == 0 arm at line 795 below." }, { "file": "include/morph/util/rational.hpp", - "line": 775, + "line": 795, "source": "numerator = sign == 0 ? 0 : (sign < 0 ? -maxValue : maxValue);", - "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 757 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." + "reason": "The sign == 0 arm is unreachable, for the same root cause as compareForSaturation's tie return at line 777 above: saturateToward is only ever called with the sign compareForSaturation returned, and compareForSaturation can only return 0 (a tie) for a pair whose overflow predicate is provably false -- so the saturating path that calls saturateToward is never entered with a tied pair, and sign is never 0 when this line runs." }, { "file": "include/morph/util/rational.hpp", - "line": 900, + "line": 929, "source": "if (crossDivisorOne == 0 || crossDivisorTwo == 0) {", "reason": "Both disjuncts are unreachable, and the body they guard (the zero-numerator short-circuit `return false;` right below) is correspondingly dead. crossDivisorOne = gcd(|numerator|, rhs.denominator), crossDivisorTwo = gcd(|rhs.numerator|, denominator). std::gcd(a, b) == 0 iff both a == 0 and b == 0. denominator/rhs.denominator can never be 0: every constructor path (Rational(Numerator, Denominator, DecimalPlaces)) calls canonicalise(), which clamps a 0 denominator to 1 before returning, and every mutating operation either recomputes the denominator as a product of positive denominators or calls canonicalise() again. So denominator > 0 (and rhs.denominator > 0) is a whole-class invariant, making crossDivisorOne/crossDivisorTwo == 0 impossible regardless of numerator/rhs.numerator." }, { "file": "include/morph/util/rational.hpp", - "line": 1498, + "line": 1572, "source": "if (ctx.begin() == ctx.end() || *ctx.begin() == '}') {", "reason": "The ctx.begin() == ctx.end() true arm is unreachable. This is the textbook cppreference-style custom-formatter parse() idiom. Empirically verified on this toolchain (libc++, via a standalone probe compiled with clang++ -std=c++23 -stdlib=libc++): for both std::format(\"{}\", x) and std::format(\"{:}\", x), ctx.end() always points past the terminating '}', so ctx.begin() == ctx.end() is false and the terminator is always reachable via *ctx.begin() == '}' -- matching the observed 0/14 split exactly. std::format's top-level parser (and std::vformat's, which performs the same replacement-field validation before dispatching to a type's parse()) rejects an unterminated '{...' before ever calling into formatter::parse, so this function is never invoked with an already-exhausted range. The first disjunct is defensive boilerplate that this standard library's implementation (and the standard's own guarantee about validated replacement fields) makes structurally unreachable." }, { "file": "include/morph/util/rational.hpp", - "line": 1407, + "line": 1481, "source": "if (!detail::addOverflows(whole, step)) {", "reason": "The false arm (the overflow-would-occur case, declining to step) is unreachable, contrary to Task 2's initial classification of this line as testable -- verified both mathematically and empirically (a standalone probe sweeping denominators/precisions found 476 cases where the scale-up saturated to whole == INT64_MAX, and every one had a fractional remainder of exactly 0, never >= 0.5) before writing this entry. A Rational's magnitude can never exceed INT64_MAX: its value is numerator/denominator with denominator >= 1 and numerator in [-INT64_MAX, INT64_MAX] (canonicalise() clamps INT64_MIN away), so |value| <= |numerator| <= INT64_MAX always. `whole` is trunc(scaled), so whole == INT64_MAX forces scaled == INT64_MAX/1 exactly -- there is no room for scaled to be in (INT64_MAX, INT64_MAX + 1) the way an unbounded rational could land. With scaled == whole exactly, `fraction` (scaled minus whole) is always 0, so `roundAway` (set from comparing fraction against 1/2) is always false when whole == INT64_MAX, and stepping never happens on that side. On the other side, step == -1 would need whole == INT64_MIN to overflow, but whole's range is [-INT64_MAX, INT64_MAX] (INT64_MIN is never a valid Rational magnitude), so that direction cannot overflow either. The guard is defensive: reachable only if a future change let a Rational's magnitude exceed INT64_MAX, which the type's invariants currently forbid everywhere else in this file." }, diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index 17671d52..a0ff6931 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -658,13 +658,13 @@ TEST_CASE("NamedQuantity slices to a plain Quantity", "[quantity]") { // // formatRationalDecimal negated the numerator with signed arithmetic, which is // UB for INT64_MIN -- confirmed by UBSan at quantity.hpp:101 before the fix. -// INT64_MIN reaches it because the whole-integer `Rational{value, DecimalPlaces}` -// constructor does not canonicalise (and `numerator` is a public member), so the -// clamp in canonicalise() never runs on this path. +// The ordinary constructors canonicalise INT64_MIN, but `numerator` remains a +// public member for aggregate-like use, so a caller can still create this state +// explicitly. The formatter must remain defined for that poisoned value. TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator renders exactly", "[quantity][rational][morph496]") { - morph::math::Rational const value{std::numeric_limits::min(), morph::math::DecimalPlaces{0}}; - // Precondition: this constructor really does keep the trap value. + morph::math::Rational value{0, morph::math::DecimalPlaces{0}}; + value.numerator = std::numeric_limits::min(); REQUIRE(value.numerator == std::numeric_limits::min()); // Under -fsanitize=undefined this line was the UB report; the magnitude must // survive the unsigned negation intact rather than wrapping. diff --git a/tests/test_rational_checked.cpp b/tests/test_rational_checked.cpp index 25cc9ed0..83592574 100644 --- a/tests/test_rational_checked.cpp +++ b/tests/test_rational_checked.cpp @@ -155,12 +155,11 @@ TEST_CASE("checkedSub mirrors checkedAdd", "[rational][checked]") { REQUIRE(ok.has_value()); CHECK(*ok == whole(2)); - // kMin + 1 (== -INT64_MAX), not kMin: constructing a Rational whose - // numerator is INT64_MIN is itself undefined behaviour -- canonicalise() - // negates the numerator unguarded, and -INT64_MIN is not representable. - // setWire guards that case on the wire path; the public constructor does - // not. Tracked separately; this test stays inside the representable range - // so it measures checkedSub rather than that. + // kMin + 1 (== -INT64_MAX), not kMin: `whole(kMin)` does not construct a + // Rational holding INT64_MIN. canonicalise() clamps such a numerator to + // -INT64_MAX and logs at error (morph#537), so passing kMin here would + // measure that clamp -- and the clamp's log -- rather than checkedSub. + // This test stays inside the representable range on purpose. // // -INT64_MAX - 2 is the first difference that genuinely does not fit // (-INT64_MAX - 1 is exactly INT64_MIN, which still does). @@ -359,6 +358,163 @@ TEST_CASE("An intermediate-only overflow saturates toward the true sign", "[rati CHECK((negLhs + negRhs).numerator == -kMax); } +TEST_CASE("Whole-integer INT64_MIN construction is canonicalised", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + const Rational value{kMin, DecimalPlaces{2}}; + CHECK(value.numerator == -kMax); + CHECK(value.denominator == 1); +} + +TEST_CASE("Public INT64_MIN numerator operations stay defined", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + CHECK((-raw).numerator == kMax); + CHECK(abs(raw).numerator == kMax); + + const auto inverse = raw.reciprocal(); + REQUIRE(inverse.has_value()); + CHECK(inverse->numerator == -1); + CHECK(inverse->denominator == kMax); + + CHECK((raw * whole(2)).numerator == -kMax); + const auto checked = checkedMul(raw, whole(2)); + REQUIRE_FALSE(checked.has_value()); + CHECK(checked.error() == RationalError::Overflow); +} + +// Cross-cancellation takes both gcd operands as *magnitudes*. Reinterpreting a +// negative denominator as uint64_t instead yields 2^64 - |d|, whose gcd with +// the other numerator is a different number -- and one that need not divide the +// denominator at all, so the reduction truncates and the value changes. +// 1/-4 * 3/1 is the shortest witness: gcd(3, 4) == 1, but +// gcd(3, 2^64 - 4) == 3, which divides 3 and not -4. A negative denominator is +// a poisoned state for the same reason an INT64_MIN numerator is -- the members +// are public -- and master handled it correctly, so this guards a regression +// rather than a gap. +TEST_CASE("Multiplication cross-cancels on magnitudes, not a reinterpreted denominator", + "[rational][checked][saturate]") { + Rational poisoned{0, DecimalPlaces{2}}; + poisoned.numerator = 1; + poisoned.denominator = -4; + + const auto product = poisoned * whole(3); + CHECK(product.numerator == -3); + CHECK(product.denominator == 4); + + const auto checked = checkedMul(poisoned, whole(3)); + REQUIRE(checked.has_value()); + CHECK(checked->numerator == -3); + CHECK(checked->denominator == 4); +} + +// reciprocal() used to negate `denominator` directly, which is the very UB this +// change exists to remove -- just in the other component. The value here is +// -1/-2^63, a hair above zero, so its inverse is 2^63 and clamps to INT64_MAX. +// A signed negation wraps to INT64_MIN instead and lands on -INT64_MAX: the +// wrong sign, not merely the wrong magnitude. +TEST_CASE("reciprocal is defined for a poisoned INT64_MIN denominator", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = -1; + raw.denominator = kMin; + + const auto inverse = raw.reciprocal(); + REQUIRE(inverse.has_value()); + CHECK(inverse->numerator == kMax); + CHECK(inverse->denominator == 1); +} + +// compareForSaturation asks `*this <=> -rhs`, so an overflowing addition puts +// the poisoned operand through unary negation. Under a signed negation that +// wraps to INT64_MIN the comparison inverts and the sum saturates toward +// +INT64_MAX -- the opposite end from the true sum. +TEST_CASE("An overflowing addition with a poisoned addend saturates toward the true sign", + "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + const auto sum = whole(-1) + raw; + CHECK(sum.numerator == -kMax); + CHECK(sum.denominator == 1); +} + +// checkedMul promises exact-or-nothing, and INT64_MIN is not an exactness this +// type can keep: canonicalise() clamps such a component to -INT64_MAX. The +// numerator case needs no poisoned operand at all -- (-2^62) * 2 is two +// ordinary canonical values whose product is exactly INT64_MIN. +TEST_CASE("checkedMul reports a reduced product of exactly INT64_MIN as overflow", "[rational][checked]") { + constexpr auto halfMin = kMin / 2; // -2^62, perfectly representable + + const auto numeratorSide = checkedMul(whole(halfMin), whole(2)); + REQUIRE_FALSE(numeratorSide.has_value()); + CHECK(numeratorSide.error() == RationalError::Overflow); + + // The denominator side of the same rule. A negative denominator is a + // poisoned state, so this one has to be built by hand. + Rational poisoned{0, DecimalPlaces{2}}; + poisoned.numerator = 1; + poisoned.denominator = halfMin; + const Rational half{Numerator{1}, Denominator{2}, DecimalPlaces{2}}; + + const auto denominatorSide = checkedMul(poisoned, half); + REQUIRE_FALSE(denominatorSide.has_value()); + CHECK(denominatorSide.error() == RationalError::Overflow); +} + +TEST_CASE("checkedMul reports a poisoned INT64_MIN numerator times one as overflow", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational raw{0, DecimalPlaces{2}}; + raw.numerator = kMin; + + // mulOverflows(INT64_MIN, 1) is correctly false -- the product fits an + // int64_t. It is canonicalisation that changes it afterwards, which is why + // the predicate has to know about representability and not just width. + const auto product = checkedMul(raw, whole(1)); + REQUIRE_FALSE(product.has_value()); + CHECK(product.error() == RationalError::Overflow); +} + +// reciprocal() has to clamp a poisoned component's 2^63 magnitude, so the value +// it returns is not the divisor's inverse and multiplying by it is inexact. +// checkedDiv is the exact-or-nothing form, so it declines instead of handing +// that back as a success. Deliberately conservative: 2 / (INT64_MIN/1) has the +// representable exact quotient -1/2^62. See checkedDiv's own doc comment. +TEST_CASE("checkedDiv declines a divisor whose reciprocal is not representable", "[rational][checked][saturate]") { + const morph::log::ScopedLoggerOverride quiet{[](morph::log::LogLevel, std::string_view) {}, + morph::log::LogLevel::error}; + + Rational poisonedNumerator{0, DecimalPlaces{2}}; + poisonedNumerator.numerator = kMin; + const auto byNumerator = checkedDiv(whole(2), poisonedNumerator); + REQUIRE_FALSE(byNumerator.has_value()); + CHECK(byNumerator.error() == RationalError::Overflow); + + Rational poisonedDenominator{0, DecimalPlaces{2}}; + poisonedDenominator.numerator = -1; + poisonedDenominator.denominator = kMin; + const auto byDenominator = checkedDiv(whole(2), poisonedDenominator); + REQUIRE_FALSE(byDenominator.has_value()); + CHECK(byDenominator.error() == RationalError::Overflow); + + // Still reports the zero divisor ahead of everything else. + const auto byZero = checkedDiv(whole(2), whole(0)); + REQUIRE_FALSE(byZero.has_value()); + CHECK(byZero.error() == RationalError::DivisionByZero); +} + TEST_CASE("A numerator of INT64_MIN is clamped, not undefined", "[rational][checked][saturate]") { std::vector logged; const morph::log::ScopedLoggerOverride capture{