From a9573b031797cd5d10625c62eae644d7591d5377 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 08:41:26 +0200 Subject: [PATCH 1/7] Flatten every walk of the provenance DAG, and settle the default A running total records one `ASTNode` per iteration, chained through `left`, and nothing collapses that chain. Every walk over it was recursive, so a deep enough derivation ran the stack out. Measured on clang 20 and clang 22 at `-O0` with an 8 MiB stack: $ ./prov0 200000 total = 200000EUR (chain of 200000 provenance nodes retained) destroying... $ echo $? 139 # SIGSEGV in the destructor chain Destruction crossed the limit between 20,800 nodes (exit 0) and 21,000 (SIGSEGV). The same binary at `-O2` survived 200,000, because clang rewrites that particular chain into a loop -- crashes in Debug, survives in Release. morph#574 flagged `equation()` as unverified. It overflows too, and with no optimiser escape, since its frames hold live `Rendered` strings across the call. Reproduced at `-O0`: returns normally at 24,000 nodes, SIGSEGV at 25,000, in `EquationRenderer::renderSymbolic`. `~ASTNode` now detaches its children into a local worklist and releases them one at a time, unlinking a node's own children only when that pop holds the last reference -- so every `~ASTNode` the loop reaches has null children and cannot recurse. All four `equation()` traversals run over an explicit stack; the symbolic and substituted renderings, which differed only in how they stop, became one stack machine selected by a `RenderMode`, so there is one traversal to get right instead of two. Output is byte-identical: the existing 62 `[quantity]`/`[render]`/`[locale]` cases pass unchanged. The two regression tests build a 100,000-node chain. What makes them evidence: on unfixed code the destruction case is SIGSEGV at `-O0` (verified -- it is vacuous at `-O2`, and the test says so and says which CI legs are Debug), and the `equation()` case is SIGSEGV at every optimisation level. `MORPH_QUANTITY_PROVENANCE` keeps its default of `1`. The measured cost is real -- 54,056 KB and 0.034 s against 12,236 KB and 0.006 s for a 200,000-iteration total -- but the toggle changes observable behaviour, not just cost: with it `0`, `equation()` collapses to the bare value and `named()` discards the name, so flipping it would silently empty both for every build that never set it. `docs/spec/util/quantity_type.md` promised the default in four places; the spec now carries the measurement, both readings of the argument, and the note that the crash was never the toggle's business, since a stack overflow is not an acceptable failure mode for either setting. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- docs/spec/util/quantity_type.md | 68 +++++- include/morph/detail/quantity_equation.hpp | 260 +++++++++++++++------ include/morph/util/quantity.hpp | 69 ++++++ tests/test_quantity.cpp | 56 +++++ 4 files changed, 375 insertions(+), 78 deletions(-) diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index 38ede6139..ce7577820 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -424,10 +424,11 @@ types: - **`ASTNode`** — a node in the DAG: its own `ASTUnit current` step, an optional symbol `name` (set by `named()` / `NamedQuantity`, which makes the node opaque and stops `equation()` expanding it), and `shared_ptr left` / `right` - handles onto the nodes that fed this one. The struct is a plain aggregate, but - nodes are **never copied** in practice: sharing is done through the `shared_ptr`, - never by duplicating a node — every operation allocates a fresh `ASTNode` and - links the (shared) prior ones. + handles onto the nodes that fed this one. Copy and move are the defaulted + ones, but nodes are **never copied** in practice: sharing is done through the + `shared_ptr`, never by duplicating a node — every operation allocates a fresh + `ASTNode` and links the (shared) prior ones. The destructor is the one member + with a body, and its job is depth (see below), not ownership. - **`Context`** — the per-`Quantity` handle: a single `shared_ptr node` pointing at the root of that value's derivation. Copying a `Quantity` copies its `Context`, which just bumps the node's refcount — the tree itself is never @@ -443,6 +444,32 @@ can be shared across differently typed quantities. It is *not* precision-erased: every value stored in a node is the exact `Rational` as computed, carrying its own runtime `DecimalPlaces` tag. +**Depth is unbounded, so every walk of the DAG is iterative.** The ordinary +running-total pattern — `total = total + one` in a loop — records one node per +iteration, chained through `left`, and nothing collapses that chain: a loop over +*n* wire rows leaves an *n*-deep derivation. A recursive walk of it overflows +the stack, so none of the walks is recursive: + +- **Destruction.** `~ASTNode` detaches its children into a local worklist and + releases them one at a time, unlinking a node's own children only when that + pop holds its last reference. Every `~ASTNode` the loop reaches therefore runs + with both children already null and cannot recurse. Without this the + compiler-generated destructor releases the chain through + `~shared_ptr` → `~ASTNode` → `~shared_ptr` → …, one frame per node. +- **`equation()`.** All four traversals — the reference count, the placeholder + labelling, and the symbolic and substituted renderings — run over an explicit + stack. The two renderings share one stack machine (`EquationRenderer::render`, + selected by `RenderMode`) whose frames resume at the same three points a + recursive call would: render-or-descend-left, take-left-descend-right, + combine. + +Measured (morph#574, clang 20 and clang 22, `-O0`, 8 MiB stack): before this, +destroying a 21,000-node chain segfaulted and `equation()` segfaulted inside +`renderSymbolic` at 25,000 nodes. The destruction case *survived* 200,000 nodes +at `-O2`, because clang rewrites that particular chain into a loop — "crashes in +Debug, survives in Release" — which is precisely why the flattening is a +specified property of the type rather than something left to the optimiser. + **Nodes are immutable once built.** No operation ever mutates an existing `ASTNode` — arithmetic, conversion, and `named()` each allocate a *new* node that points at the (unchanged) prior ones. Immutability is what makes sharing @@ -1219,6 +1246,39 @@ deliberately not attempted): never crosses the wire and has a **single consumer**, `equation()`. For hot paths that never call `equation()`, build with `MORPH_QUANTITY_PROVENANCE=0`: the API stays callable and no nodes are allocated. + + The cost is measured, not estimated. A 200,000-iteration running total + (morph#574, clang 22, `-O2`, Linux): **54,056 KB** max RSS and 0.034 s with + the default, against **12,236 KB** and 0.006 s with `MORPH_QUANTITY_PROVENANCE=0` + — 4.4x the memory and 5.7x the time, for a loop that adds integers. The + retained chain is proportional to the loop bound, so a loop whose bound comes + from wire input (a ledger replay, a batch of rows) allocates in proportion to + that input. **An application that puts `Quantity` on a bulk path, and does not + need `equation()` on it, should build with the macro set to `0`.** + + morph#574 proposed flipping the default to `0` on those numbers. It stays at + `1`, and the reasoning is recorded here rather than left implicit, because the + two readings are both defensible and the disagreement is the interesting part: + + - *For flipping.* Nobody opts into a cost they do not know about, and the + price of the default is paid by every build that never calls `equation()`. + - *Against, and this is the decision.* The toggle **changes observable + behaviour, not just performance** — see the limitation two entries below: + with tracing off `equation()` collapses to the bare value and `named()` + discards the name. Flipping the default would silently empty the output of + both for every existing build that did not set the macro. And provenance is + not an incidental extra: this document opens by naming it as the third of + the three things a `Quantity` knows, and as the reason a domain application + reaches for this type instead of an exact number. A default that turns the + distinguishing feature off, silently, to buy speed on paths that can already + opt out of it with one flag, trades the wrong way round. + + The crash that report also found is a separate matter and was **not** left to + the toggle: a deep chain used to overflow the stack in *either* setting, which + is not an acceptable failure mode for a default, and both the destructor and + every `equation()` traversal are now iterative (see *Provenance*, "Depth is + unbounded"). If the default is ever revisited, it should be revisited on the + behaviour argument above, not on the crash — that is fixed. - **`int64` ratio overflow for wide-range unit systems.** Conversion ratios are exact `Rational`s of 64-bit integers. A unit system spanning many orders of magnitude (pico- to tera-, say) risks overflowing a composed chained ratio — diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index fece47402..dc916a79c 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -10,11 +10,13 @@ /// lines: formula, substitution, result, and a `where` legend for reused /// values. See `docs/spec/util/quantity_type.md` for the output contract. +#include #include #include #include #include #include +#include #include #include "../attributes.hpp" @@ -63,6 +65,42 @@ struct Rendered { int precedence{100}; }; +/// @brief Which of the two renderings `EquationRenderer::render` produces. +enum class RenderMode : std::uint8_t { + /// @brief Symbolic: a named node prints as its name and a reused node as + /// its `cK` placeholder, neither expanded. + symbolic, + + /// @brief Substituted: a named node and a reused node both print as their + /// value. + substituted, +}; + +/// @brief One suspended visit in the iterative renderer — what a recursive +/// `renderSymbolic` call would have kept in its stack frame. +struct RenderFrame { + /// @brief The node being rendered. + const ASTNode* node{nullptr}; + + /// @brief Whether to expand @ref node itself rather than label it. + bool expandThis{false}; + + /// @brief 0 = not started, 1 = left operand rendered, 2 = both rendered. + int stage{0}; + + /// @brief The left operand's rendering, once stage 1 is reached. + Rendered left; +}; + +/// @brief One suspended visit in the iterative `assignLabels` walk. +struct LabelFrame { + /// @brief The node to label or descend through. + const ASTNode* node{nullptr}; + + /// @brief Whether to expand @ref node itself rather than label it. + bool expandThis{false}; +}; + /// @brief Stateful renderer for one `equation()` call. struct EquationRenderer { /// @brief Placeholder number per reused, unnamed node (1-based). @@ -85,53 +123,73 @@ struct EquationRenderer { [[nodiscard]] bool isPlaceholder(const ASTNode* node) const { return refCount.at(node) >= 2; } /// @brief Counts in-edges through displayed paths (stops at opaque nodes). - /// Recurses into children unconditionally; the null guard handles the - /// absent operands of leaf/unary/scalar steps. - /// @param node The current node (may be null). - void countRefs(const ASTNode* node) { - if (node == nullptr) { - return; - } - if (seen.contains(node)) { - return; - } - seen.insert(node); - if (node->name.has_value() || isAtomNode(*node)) { - return; + /// + /// Iterative, over an explicit worklist, for the reason spelled out on + /// `render` below: the derivation of a running total is a linear chain and + /// a recursive walk of it overflows the stack (morph#574). Each node is + /// counted exactly once (the `seen` set), so the worklist order does not + /// affect the counts. + /// @param root The node to start from (may be null). + void countRefs(const ASTNode* root) { + std::vector pending; + if (root != nullptr) { + pending.push_back(root); } - if (node->left) { - ++refCount[node->left.get()]; - } - if (node->right) { - ++refCount[node->right.get()]; + while (!pending.empty()) { + const ASTNode* node = pending.back(); + pending.pop_back(); + if (seen.contains(node)) { + continue; + } + seen.insert(node); + if (node->name.has_value() || isAtomNode(*node)) { + continue; + } + if (node->left) { + ++refCount[node->left.get()]; + pending.push_back(node->left.get()); + } + if (node->right) { + ++refCount[node->right.get()]; + pending.push_back(node->right.get()); + } } - countRefs(node->left.get()); - countRefs(node->right.get()); } /// @brief Assigns placeholder labels in first-appearance order. - /// @param node The current node. - /// @param expandThis Whether to expand @p node itself (rather than label it). - void assignLabels(const ASTNode* node, bool expandThis) { - if (node == nullptr || node->name.has_value()) { - return; - } - if (!expandThis && isPlaceholder(node)) { - if (!labelIndex.contains(node)) { + /// + /// Iterative for the same reason as `countRefs`. First appearance is a + /// left-before-right pre-order, so the right child is pushed first and the + /// left one popped first — the order a recursive walk would have visited + /// them in. + /// @param root The node to start from. + /// @param expandRoot Whether to expand @p root itself (rather than label it). + void assignLabels(const ASTNode* root, bool expandRoot) { + std::vector pending; + pending.push_back(LabelFrame{root, expandRoot}); + while (!pending.empty()) { + LabelFrame const frame = pending.back(); + pending.pop_back(); + const ASTNode* node = frame.node; + if (node == nullptr || node->name.has_value()) { + continue; + } + if (!frame.expandThis && isPlaceholder(node) && !labelIndex.contains(node)) { labelIndex.emplace(node, placeholderOrder.size() + 1); placeholderOrder.push_back(node); } - if (!isAtomNode(*node)) { - assignLabels(node->left.get(), false); - assignLabels(node->right.get(), false); + // Both the labelled and the unlabelled arm descend exactly when the + // node is not an atom, so the two cases share one exit. + if (isAtomNode(*node)) { + continue; + } + if (node->right) { + pending.push_back(LabelFrame{node->right.get(), false}); + } + if (node->left) { + pending.push_back(LabelFrame{node->left.get(), false}); } - return; - } - if (isAtomNode(*node)) { - return; } - assignLabels(node->left.get(), false); - assignLabels(node->right.get(), false); } /// @brief Parenthesises and joins a binary subexpression. @@ -156,16 +214,23 @@ struct EquationRenderer { return Rendered{text, 3}; } - /// @brief Renders a node symbolically (names, placeholders, inlined ops). + /// @brief The rendering of a node that stops the walk — a name, a `cK` + /// placeholder, a leaf value or a conversion result. /// @param node The node. - /// @param expandThis Whether to expand @p node even if it is a placeholder. - /// @return The symbolic rendering. - [[nodiscard]] Rendered renderSymbolic(const ASTNode* node, bool expandThis) { - if (node->name.has_value()) { - return Rendered{"\"" + *node->name + "\"", 100}; - } - if (!expandThis && isPlaceholder(node)) { - return Rendered{"c" + std::to_string(labelIndex.at(node)), 100}; + /// @param expandThis Whether to expand @p node even if named/placeholder. + /// @param mode Symbolic or substituted. + /// @return The atom's rendering, or `std::nullopt` when @p node has + /// operands that must be rendered first. + [[nodiscard]] std::optional atomRendering(const ASTNode* node, bool expandThis, RenderMode mode) const { + if (mode == RenderMode::symbolic) { + if (node->name.has_value()) { + return Rendered{"\"" + *node->name + "\"", 100}; + } + if (!expandThis && isPlaceholder(node)) { + return Rendered{"c" + std::to_string(labelIndex.at(node)), 100}; + } + } else if (!expandThis && (node->name.has_value() || isPlaceholder(node))) { + return Rendered{formatOptional(nodeValue(*node)), 100}; } if (isLeafNode(*node)) { return Rendered{formatOptional(node->current.lhs), 100}; @@ -173,46 +238,93 @@ struct EquationRenderer { if (isConversionNode(*node)) { return Rendered{formatOptional(node->current.result), 100}; } - Rendered const left = - node->left ? renderSymbolic(node->left.get(), false) : Rendered{formatOptional(node->current.lhs), 100}; - bool const hasRight = node->right || node->current.rhs.has_value(); - if (!hasRight) { - return combineUnary(left); + return std::nullopt; + } + + /// @brief Renders a node, iteratively. + /// + /// The walk is an explicit stack rather than recursion because the + /// derivation of a running total (`total = total + x` in a loop) is a + /// linear chain one node deep per iteration, and a recursive walk of it + /// runs the stack out: measured (morph#574, clang 20, `-O0`, 8 MiB stack) + /// `equation()` segfaulted inside the old recursive `renderSymbolic` at a + /// 25,000-node chain and returned normally at 24,000. Unlike the operand + /// destructor chain, no optimiser rewrite hides this one — the frames hold + /// live `Rendered` strings across the call — so the depth limit was real in + /// every build. + /// + /// Each frame resumes where its recursive twin would have: stage 0 renders + /// the node or descends left, stage 1 takes the left result and descends + /// right, stage 2 combines. `finished` carries the rendering of the frame + /// that just popped, in place of a return value. + /// @param root The node to render. + /// @param expandRoot Whether to expand @p root even if named/placeholder. + /// @param mode Symbolic or substituted. + /// @return The rendering of @p root. + [[nodiscard]] Rendered render(const ASTNode* root, bool expandRoot, RenderMode mode) const { + std::vector stack; + stack.push_back(RenderFrame{root, expandRoot, 0, Rendered{}}); + Rendered finished; + while (!stack.empty()) { + RenderFrame& top = stack.back(); + if (top.stage == 0) { + if (std::optional atom = atomRendering(top.node, top.expandThis, mode)) { + finished = *std::move(atom); + stack.pop_back(); + continue; + } + top.stage = 1; + if (top.node->left) { + const ASTNode* child = top.node->left.get(); + stack.push_back(RenderFrame{child, false, 0, Rendered{}}); + continue; + } + finished = Rendered{formatOptional(top.node->current.lhs), 100}; + continue; + } + if (top.stage == 1) { + top.left = std::move(finished); + bool const hasRight = top.node->right || top.node->current.rhs.has_value(); + if (!hasRight) { + finished = combineUnary(top.left); + stack.pop_back(); + continue; + } + top.stage = 2; + if (top.node->right) { + const ASTNode* child = top.node->right.get(); + stack.push_back(RenderFrame{child, false, 0, Rendered{}}); + continue; + } + finished = Rendered{formatOptional(top.node->current.rhs), 100}; + continue; + } + finished = combine(top.node->current.operation, top.left, finished); + stack.pop_back(); } - Rendered const right = - node->right ? renderSymbolic(node->right.get(), false) : Rendered{formatOptional(node->current.rhs), 100}; - return combine(node->current.operation, left, right); + return finished; + } + + /// @brief Renders a node symbolically (names, placeholders, inlined ops). + /// @param node The node. + /// @param expandThis Whether to expand @p node even if it is a placeholder. + /// @return The symbolic rendering. + [[nodiscard]] Rendered renderSymbolic(const ASTNode* node, bool expandThis) const { + return render(node, expandThis, RenderMode::symbolic); } /// @brief Renders a node with values substituted for symbols/placeholders. /// @param node The node. /// @param expandThis Whether to expand @p node even if named/placeholder. /// @return The substituted rendering. - [[nodiscard]] Rendered renderSubstituted(const ASTNode* node, bool expandThis) { - if (!expandThis && (node->name.has_value() || isPlaceholder(node))) { - return Rendered{formatOptional(nodeValue(*node)), 100}; - } - if (isLeafNode(*node)) { - return Rendered{formatOptional(node->current.lhs), 100}; - } - if (isConversionNode(*node)) { - return Rendered{formatOptional(node->current.result), 100}; - } - Rendered const left = - node->left ? renderSubstituted(node->left.get(), false) : Rendered{formatOptional(node->current.lhs), 100}; - bool const hasRight = node->right || node->current.rhs.has_value(); - if (!hasRight) { - return combineUnary(left); - } - Rendered const right = node->right ? renderSubstituted(node->right.get(), false) - : Rendered{formatOptional(node->current.rhs), 100}; - return combine(node->current.operation, left, right); + [[nodiscard]] Rendered renderSubstituted(const ASTNode* node, bool expandThis) const { + return render(node, expandThis, RenderMode::substituted); } /// @brief Builds one `where`-legend line for a placeholder. /// @param node The placeholder node. /// @return The legend body (`cK = ...`). - [[nodiscard]] std::string legendLine(const ASTNode* node) { + [[nodiscard]] std::string legendLine(const ASTNode* node) const { std::string const label = "c" + std::to_string(labelIndex.at(node)); if (isAtomNode(*node)) { return label + " = " + formatOptional(nodeValue(*node)); diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index ed4feffaa..2beb0934c 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -35,6 +35,15 @@ /// @brief Build-wide provenance toggle. Default on; define to `0` to compile /// the derivation DAG out (no `ASTNode` allocations). +/// +/// The default is `1` deliberately: the toggle changes observable behaviour, +/// not just cost — with it `0`, `equation()` collapses to the bare value and +/// `named()` discards the name — so a build that never set it must not have +/// that output emptied underneath it. The cost is real and measured, though: +/// a 200,000-iteration running total took 54,056 KB and 0.034 s with +/// provenance against 12,236 KB and 0.006 s without (morph#574, clang 22, +/// `-O2`). **A bulk path that never calls `equation()` should set this to `0`.** +/// See `docs/spec/util/quantity_type.md`, *Limitations*. #ifndef MORPH_QUANTITY_PROVENANCE #define MORPH_QUANTITY_PROVENANCE 1 #endif @@ -531,6 +540,66 @@ struct ASTNode { /// @brief Right operand's derivation (shared); null for unary/scalar steps. std::shared_ptr right; + + /// @brief Default-constructs an empty node. + ASTNode() = default; + + /// @brief Copies a node (the children stay shared). + ASTNode(const ASTNode&) = default; + + /// @brief Copy-assigns a node (the children stay shared). + /// @return `*this`. + ASTNode& operator=(const ASTNode&) = default; + + /// @brief Moves a node, leaving the source's children null. + ASTNode(ASTNode&&) = default; + + /// @brief Move-assigns a node, leaving the source's children null. + /// @return `*this`. + ASTNode& operator=(ASTNode&&) = default; + + /// @brief Releases the sub-derivation **iteratively**, so chain depth + /// cannot overflow the stack. + /// + /// The running-total pattern (`total = total + x` in a loop) builds a + /// derivation that is a linear chain down `left`, one node per iteration. + /// The compiler-generated destructor releases that chain recursively — + /// `~shared_ptr` -> `~ASTNode` -> `~shared_ptr` -> ... — one stack frame + /// per node, and a long enough chain runs the stack out. Measured + /// (morph#574, clang 20, `-O0`, 8 MiB stack): a 21,000-node chain + /// segfaults on destruction, while an optimised build survives 200,000 + /// because clang turns the same chain into a loop. "Crashes in Debug, + /// survives in Release" is the worst signature a defect can have, so the + /// flattening is written down here rather than left to the optimiser. + /// + /// Children are detached into a local worklist and released one at a time. + /// A node is only unlinked when this pop holds its last reference; when it + /// does not, dropping the handle cannot destroy it and its children stay + /// where they are. Every `~ASTNode` reached from the loop therefore runs + /// with both children already null, so it cannot recurse. + ~ASTNode() { + std::vector> pending; + if (left) { + pending.push_back(std::move(left)); + } + if (right) { + pending.push_back(std::move(right)); + } + while (!pending.empty()) { + std::shared_ptr node = std::move(pending.back()); + pending.pop_back(); + if (node.use_count() == 1) { + if (node->left) { + pending.push_back(std::move(node->left)); + } + if (node->right) { + pending.push_back(std::move(node->right)); + } + } + // `node` goes out of scope here: either it was not the last owner + // (nothing happens) or it was, and its children are already null. + } + } }; /// @brief The per-`Quantity` handle onto the root of its derivation. diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index e84c52b55..b7571e9ac 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -670,3 +670,59 @@ TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator render // survive the unsigned negation intact rather than wrapping. REQUIRE(morph::units::detail::formatRationalDecimal(value) == "-9223372036854775808"); } + +// ── morph#574: a deep derivation chain must not overflow the stack ── +// +// `total = total + one` in a loop records one ASTNode per iteration, chained +// through `left`, and nothing collapses the chain. Both walks over it used to +// be recursive -- the compiler-generated `~ASTNode` and every traversal in +// `equation()` -- so a long enough chain ran the stack out. +// +// **What makes these two cases evidence, and what does not.** The destruction +// case is load-bearing in an *unoptimised* build only: measured on clang 20 and +// clang 22 with an 8 MiB stack, unfixed code segfaults on destruction at 21,000 +// nodes at `-O0` and survives 200,000 at `-O2`, because clang rewrites the +// recursive release into a loop. So this case fails on unfixed code in every +// Debug configuration CI runs (gcc-debug, clang-debug, the three sanitizer +// legs, cl-debug) and passes vacuously in a Release one. The `equation()` case +// has no such escape: its frames hold live `Rendered` strings across the call, +// no optimiser can turn it into a loop, and unfixed code segfaulted inside +// `renderSymbolic` at 25,000 nodes. +namespace { +constexpr int kDeepChainNodes = 100000; +} // namespace + +TEST_CASE("A 100000-node provenance chain is destroyed without overflowing the stack", + "[quantity][provenance][morph574]") { + { + Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; + Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; + for (int i = 0; i < kDeepChainNodes; ++i) { + total = total + one; + } + REQUIRE(total.hasValue()); + REQUIRE(total.value()->toDouble() == static_cast(kDeepChainNodes)); + // The chain is released here. On unfixed code this is a SIGSEGV at -O0, + // which takes the whole test binary with it rather than failing one + // assertion -- the crash *is* the failure signal. + } + SUCCEED("the chain was released without a stack overflow"); +} + +TEST_CASE("equation() walks a 100000-node provenance chain without overflowing the stack", + "[quantity][provenance][equation][morph574]") { + Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; + Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; + for (int i = 0; i < kDeepChainNodes; ++i) { + total = total + one; + } + + auto const lines = total.equation(); + // Formula, substitution, result, and one `where` line: the single `one` + // leaf is referenced 100,000 times, so it earns exactly one placeholder. + REQUIRE(lines.size() == 4); + CHECK(lines[0].starts_with("0 + c1 + c1")); + CHECK(lines[1].starts_with(" = 0 + 1 + 1")); + CHECK(lines[2] == " = 100000"); + CHECK(lines[3] == "where c1 = 1"); +} From dc0ae6ad2b289aa23519a811c26035abae32c271 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 08:48:41 +0200 Subject: [PATCH 2/7] Validate group separators at both control edges instead of stripping them `normalizeLocaleNumber` dropped every occurrence of the group separator unconditionally, before the decimal check and with no check on placement. So a German user typing the US form into a price field submitted ten times what they meant, and nothing downstream could tell -- the result is a perfectly valid number: normalize("1.5", dec=",", grp=".") = "15" normalize("1.50", dec=",", grp=".") = "150" normalize("1.2.3.4", dec=",", grp=".") = "1234" normalize("1,5", dec=".", grp=",") = "15" <-- the en-US mirror normalize("1.5", dec=".", grp=".") = "15" <-- separators equal The QML mirror in src/qt/forms/qml/DynamicForm.qml was flagged as unverified. It is verified now: lifted out and run on the same inputs, it produced those five answers byte for byte. Consistently wrong is still wrong, so both edges are fixed, and a 47-case differential over both implementations confirms they agree on every one. A group separator is now dropped only where one can legally be: preceded by one to three digits, followed by exactly three, never after the decimal separator. All five lines above are `std::nullopt` and the caller can tell the user to fix the entry. Equal separators are rejected too -- one string in both roles has no defensible reading of "1.5" -- through the return value rather than an `assert`, deliberately: an assertion would make a control edge behave differently in Debug and Release, and would be untestable in the build where it fires. normalize("1.050,25", dec=",", grp=".") = "1050.25" normalize("1.000.000,25", dec=",", grp=".") = "1000000.25" normalize("1050,25", dec=",", grp=".") = "1050.25" normalize("1 050,25", dec=",", grp=" ") = "1050.25" (U+202F too) The existing suite had 18 cases, every one of them single-locale, so all 18 passed with the stripping and with the validation alike. The four new cases are cross-locale, equal-separator, group-placement, and a control that every well-formed entry still normalises; three of the four fail on unfixed code (16 assertions). On the QML side `test_foreignDecimalSeparatorIsRejectedNotAbsorbed` fails on the unfixed mirror and passes on the fixed one, verified by reverting the file and rebuilding. examples/ledger/README.md listed this as an open ladder finding; it now records the fix and keeps the separate `double`-division display item. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- docs/spec/forms/forms.md | 27 +++++++++ examples/ledger/README.md | 12 ++-- include/morph/render/locale_format.hpp | 54 +++++++++++++++++- src/qt/forms/qml/DynamicForm.qml | 58 +++++++++++++++---- src/qt/forms/tests/tst_i18n.qml | 26 +++++++++ tests/test_render_locale_format.cpp | 77 ++++++++++++++++++++++++++ 6 files changed, 236 insertions(+), 18 deletions(-) diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 89a005c55..8da1e5dbc 100644 --- a/docs/spec/forms/forms.md +++ b/docs/spec/forms/forms.md @@ -1333,6 +1333,33 @@ Display formatting is the renderer's duty; the wire stays canonical: single byte that never matched, so a perfectly valid `"1 050,25"` typed by a French user normalised to `std::nullopt` and the control reported it malformed. An empty view means "this locale has no such separator". + + **Grouping is validated, never merely stripped.** A group separator is + dropped only where a group separator can legally be: preceded by one to three + digits, followed by exactly three more, and never after the decimal + separator. `"1.050,25"`, `"1.000.000,25"` and an ungrouped `"1050,25"` all + normalise; `"1.5"`, `"1.50"`, `"1.05"` and `"1.2.3.4"` in a de-DE locale are + malformed, and so is the en-US mirror image `"1,5"`. This is not + strictness for its own sake: dropping every occurrence unconditionally, as + both control edges used to, turns a de-DE user's US-style `"1.5"` into `15` — + a perfectly valid number, ten times too large, that no downstream check can + recognise as wrong, so the user is charged ten times with no diagnostic + anywhere (morph#574). The field's job at this edge is to report a fact to the + layer that owns the policy, not to produce a number at any price. + + **The two separators must differ.** A non-empty `groupSeparator` equal to + `decimalSeparator` is rejected like any other malformed entry — with one + string in both roles there is no reading of `"1.5"` the function could + defend, and the old code silently ate the decimal. It is reported through the + return value rather than an assertion, deliberately: an assertion would make + a control edge behave differently in Debug and Release, and would be + untestable in the configuration where it fires. + + **Both edges, or neither.** `src/qt/forms/qml/DynamicForm.qml` carries a + JavaScript mirror of this function, and a divergence between them is a + divergence in what the product accepts. The mirror produced byte-identical + wrong answers on all of the cases above and carries byte-identical + validation now; changing one without the other is the defect, not the fix. - **Timestamps.** The wire value is strict UTC ISO-8601 ([datetime.md](../util/datetime.md)); a renderer displays and edits in the user's zone by shifting a `morph::time::DateTime` with its existing diff --git a/examples/ledger/README.md b/examples/ledger/README.md index 4f0a1a3af..30f65077e 100644 --- a/examples/ledger/README.md +++ b/examples/ledger/README.md @@ -388,11 +388,13 @@ data; the submit→poll job idiom. unit at `dp=0` and the type system carries it natively. Named test: a JPY leg stores and displays as a true integer, with no `x-rules` gate required. -- **Locale entry**: in de-DE the group separator is "." and the shipped - normalizer strips it anywhere — typing `1.5` submits **15**, a silent 10× - money error. Pin the behavior, fix (positional grouping validation or - reject), and mirror the vectors through `normalizeLocaleNumber` (D5). - Related: result *display* in the shipped forms renderer goes through +- **Locale entry** — *fixed, morph#574*: in de-DE the group separator is "." + and the shipped normalizer stripped it anywhere, so typing `1.5` submitted + **15**, a silent 10× money error. `normalizeLocaleNumber` and its QML mirror + now validate group placement (one to three digits before, exactly three + after, never past the decimal separator) and report a malformed entry + instead; `"1.050,25"` still normalises. Remaining from this item: + result *display* in the shipped forms renderer goes through `double` division — balances beyond 2^53 drift on readback while the payload is exact. This rung's own views do not: every money label binds text the bridge pre-rendered through `ledger::formatMoney`, which is exact diff --git a/include/morph/render/locale_format.hpp b/include/morph/render/locale_format.hpp index 3e4a12332..fcacf495d 100644 --- a/include/morph/render/locale_format.hpp +++ b/include/morph/render/locale_format.hpp @@ -33,7 +33,7 @@ namespace morph::render { /// @brief Converts a locale-formatted numeric string to canonical /// (`-?[0-9]+(\.[0-9]+)?`) text. /// -/// Strips every occurrence of @p groupSeparator, then replaces every +/// Drops @p groupSeparator where it is correctly placed, and replaces every /// occurrence of @p decimalSeparator with `.`. Passing `decimalSeparator == /// "."` and an empty @p groupSeparator is the identity transform (the /// locale-free behavior). Malformed input (a second decimal separator, a @@ -43,6 +43,25 @@ namespace morph::render { /// ("`,-5`" in a de-DE locale) is rejected -- matching the QML mirror in /// `src/qt/forms/qml/DynamicForm.qml`, which has always rejected it (morph#497). /// +/// @par Grouping is validated, not stripped (morph#574) +/// A group separator is only dropped where a group separator can legally be: +/// preceded by one to three digits, followed by exactly three more, and never +/// after the decimal separator. Anything else is malformed and reported as +/// such. Stripping unconditionally instead is a wrong *value*, not a rejected +/// one: a de-DE user typing the US form `"1.5"` into a price field submitted +/// `15`, and nothing downstream could tell -- the result is a perfectly valid +/// number, ten times too large. `"1.50"` gave `150`, `"1.2.3.4"` gave `1234`, +/// and the en-US mirror image `"1,5"` gave `15`. +/// +/// @par The two separators must differ +/// When @p groupSeparator is non-empty and equal to @p decimalSeparator the +/// entry is rejected: with one string in both roles there is no reading of +/// `"1.5"` the function could defend. This is a caller (locale-configuration) +/// error rather than a user one, but it is reported through the return value +/// like any other malformed entry, deliberately not through an assertion -- +/// an assertion would make the two build configurations behave differently at +/// a control edge, and would be untestable in the one where it fires. +/// /// The result is `.`-decimal and digit-only, but is **not** narrowed to /// `-?[0-9]+(\.[0-9]+)?`: a bare "`.`", a leading "`.5`" and a trailing "`5.`" /// are passed through, exactly as that same QML mirror passes them. Tightening @@ -61,21 +80,45 @@ namespace morph::render { [[nodiscard]] inline std::optional normalizeLocaleNumber(std::string_view text, std::string_view decimalSeparator, std::string_view groupSeparator) { + if (!groupSeparator.empty() && groupSeparator == decimalSeparator) { + return std::nullopt; // one string cannot play both roles: see above + } + std::string canonical; canonical.reserve(text.size()); bool sawDecimal = false; bool sawAnyOutput = false; + // Grouping state: how many digits since the last group separator (or since + // the start), and whether any group separator has been seen at all. + std::size_t digitsInGroup = 0; + bool sawGroup = false; + constexpr std::size_t kGroupSize = 3; for (std::size_t i = 0; i < text.size();) { const std::string_view rest = text.substr(i); if (!groupSeparator.empty() && rest.starts_with(groupSeparator)) { + if (sawDecimal) { + return std::nullopt; // grouping belongs to the integer part only + } + // The first group is one to three digits ("1.050", "12.050", + // "123.050"); every later one is exactly three. + bool const wellPlaced = + sawGroup ? digitsInGroup == kGroupSize : (digitsInGroup >= 1 && digitsInGroup <= kGroupSize); + if (!wellPlaced) { + return std::nullopt; // not a group boundary: malformed + } + sawGroup = true; + digitsInGroup = 0; i += groupSeparator.size(); - continue; // grouping is display-only; never accepted back on entry + continue; // grouping is display-only; never carried into the output } if (!decimalSeparator.empty() && rest.starts_with(decimalSeparator)) { if (sawDecimal) { return std::nullopt; // a second decimal separator: malformed } + if (sawGroup && digitsInGroup != kGroupSize) { + return std::nullopt; // the last group is short: "1.5" in de-DE + } sawDecimal = true; canonical += '.'; // The decimal point *is* output: without this, the `chr == '-'` @@ -99,6 +142,7 @@ namespace morph::render { canonical += chr; } else if (chr >= '0' && chr <= '9') { canonical += chr; + ++digitsInGroup; } else { return std::nullopt; // any other character is malformed } @@ -106,6 +150,12 @@ namespace morph::render { ++i; } + // A grouped integer part has to end on a group boundary too: "1.05" and + // "1.050." are as malformed as "1.5" is. + if (sawGroup && !sawDecimal && digitsInGroup != kGroupSize) { + return std::nullopt; + } + if (canonical.empty() || canonical == "-") { return std::nullopt; } diff --git a/src/qt/forms/qml/DynamicForm.qml b/src/qt/forms/qml/DynamicForm.qml index 9a317719c..52ecab810 100644 --- a/src/qt/forms/qml/DynamicForm.qml +++ b/src/qt/forms/qml/DynamicForm.qml @@ -807,32 +807,68 @@ Frame { // The payload's exact digit routines below stay entirely locale-free — // this is the one control-edge conversion step, applied once per entry. + // Grouping is *validated*, not stripped (morph#574). A group separator is + // only dropped where one can legally be -- preceded by one to three digits, + // followed by exactly three more, never after the decimal separator. + // Stripping it unconditionally, which both this function and its C++ twin + // used to do, turns a de-DE user's US-style "1.5" into 15: a valid number, + // ten times too large, that nothing downstream can recognise as wrong. + // Verified against the C++ side on the same inputs before and after; the + // two edges agreed on every wrong answer and now agree on every rejection. function normalizeLocaleNumber(text, decimalSeparator, groupSeparator) { - let stripped = "" - for (let i = 0; i < text.length; ++i) { - if (groupSeparator !== "" && text[i] === groupSeparator) - continue - stripped += text[i] - } + // One string cannot play both roles: there is no reading of "1.5" this + // function could defend, so it reports rather than guesses. + if (groupSeparator !== "" && groupSeparator === decimalSeparator) + return null + + const groupSize = 3 let canonical = "" let sawDecimal = false - for (let i = 0; i < stripped.length; ++i) { - const ch = stripped[i] - if (ch === decimalSeparator) { + let sawAnyOutput = false + let digitsInGroup = 0 + let sawGroup = false + for (let i = 0; i < text.length; ++i) { + const ch = text[i] + if (groupSeparator !== "" && ch === groupSeparator) { if (sawDecimal) + return null // grouping belongs to the integer part only + // The first group is one to three digits; every later one is + // exactly three. + const wellPlaced = sawGroup ? digitsInGroup === groupSize + : (digitsInGroup >= 1 && digitsInGroup <= groupSize) + if (!wellPlaced) return null + sawGroup = true + digitsInGroup = 0 + continue + } + if (decimalSeparator !== "" && ch === decimalSeparator) { + if (sawDecimal) + return null + if (sawGroup && digitsInGroup !== groupSize) + return null // the last group is short: "1.5" in de-DE sawDecimal = true canonical += "." - } else if (ch === "-") { - if (i !== 0) + // The decimal point is output, so a sign straight after it is + // not leading (morph#497). + sawAnyOutput = true + continue + } + if (ch === "-") { + if (sawAnyOutput) return null canonical += ch } else if (ch >= "0" && ch <= "9") { canonical += ch + ++digitsInGroup } else { return null } + sawAnyOutput = true } + // A grouped integer part has to end on a group boundary too. + if (sawGroup && !sawDecimal && digitsInGroup !== groupSize) + return null if (canonical === "" || canonical === "-") return null return canonical diff --git a/src/qt/forms/tests/tst_i18n.qml b/src/qt/forms/tests/tst_i18n.qml index cdac769be..655be2e60 100644 --- a/src/qt/forms/tests/tst_i18n.qml +++ b/src/qt/forms/tests/tst_i18n.qml @@ -137,6 +137,32 @@ Item { compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') } + // morph#574. The de-DE locale groups with "." and this form's user + // typed the US decimal form. Stripping the group separator + // unconditionally -- which this mirror did, byte for byte in step with + // its C++ twin -- submitted 1.5 as 15: a valid payload, ten times too + // large, with nothing downstream able to tell. The field is now + // reported malformed, which is what the user can act on. + function test_foreignDecimalSeparatorIsRejectedNotAbsorbed() { + localeForm.setFieldValue("mass", "1.5") + verify(!localeForm.ready) + compare(localeForm.previewLine, "") + + localeForm.setFieldValue("mass", "1.50") + verify(!localeForm.ready) + + // A group separator off a group boundary is malformed wherever it + // sits, not only at the end. + localeForm.setFieldValue("mass", "1234.050,25") + verify(!localeForm.ready) + + // Control: the well-formed entry still goes through, so the + // rejection above is about placement and not about "." at all. + localeForm.setFieldValue("mass", "1.050,25") + verify(localeForm.ready) + compare(localeForm.previewLine, '{"mass":{"num":1050250,"den":1000,"dp":3}}') + } + function test_zonedTimestampRoundTripsToUtc() { zonedForm.setFieldValue("when", "2026-07-05T16:30:00") // 16:30 in UTC+2 verify(zonedForm.ready) diff --git a/tests/test_render_locale_format.cpp b/tests/test_render_locale_format.cpp index cdd4c3d6a..8ba72d190 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -178,3 +178,80 @@ TEST_CASE("normalizeLocaleNumber: the loose shapes stay accepted, in step with t REQUIRE(morph::render::normalizeLocaleNumber("5.", ".", ",") == "5."); REQUIRE(morph::render::normalizeLocaleNumber(".", ".", ",") == "."); } + +// ── morph#574: a group separator is validated, not stripped ────────────────── +// +// Before this, every occurrence of the group separator was dropped +// unconditionally, so a de-DE user typing the US form "1.5" into a price field +// submitted 15 -- a valid-looking number, ten times too large, with no +// diagnostic anywhere. The suite above has 18 cases and not one of them was +// cross-locale: every single-locale case passes with the stripping or with the +// validation, which is exactly the check that would still pass if the feature +// did nothing. + +TEST_CASE("normalizeLocaleNumber: the decimal separator of another locale is rejected, not absorbed", + "[render][locale][morph574]") { + // THE case. de-DE locale, US-style decimal typed: this returned "15". + CHECK(normalizeLocaleNumber("1.5", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.50", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.2.3.4", ",", ".") == std::nullopt); + + // The mirror image: en-US locale, EU-style decimal typed. Returned "15". + CHECK(normalizeLocaleNumber("1,5", ".", ",") == std::nullopt); + + // And with a multi-byte group separator, where the same mistake is a + // narrow no-break space away from a well-formed entry. + CHECK(normalizeLocaleNumber(std::string{"1"} + std::string{kNarrowNbsp} + "5", ",", kNarrowNbsp) == std::nullopt); +} + +TEST_CASE("normalizeLocaleNumber: equal decimal and group separators are rejected rather than guessed", + "[render][locale][morph574]") { + // One string in both roles has no defensible reading, and the old code + // silently ate the decimal: this returned "15". + CHECK(normalizeLocaleNumber("1.5", ".", ".") == std::nullopt); + // Not even the shapes that would be unambiguous if you squinted: the + // rejection is on the configuration, not on the text. + CHECK(normalizeLocaleNumber("1.050", ".", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1", ".", ".") == std::nullopt); + // Control: an empty group separator is "this locale does not group", which + // is a different statement and stays legal. + CHECK(normalizeLocaleNumber("1.5", ".", "") == "1.5"); +} + +TEST_CASE("normalizeLocaleNumber: a group separator must sit on a group boundary", "[render][locale][morph574]") { + // Preceded by one to three digits... + CHECK(normalizeLocaleNumber("1.050", "", ".") == "1050"); + CHECK(normalizeLocaleNumber("12.050", "", ".") == "12050"); + CHECK(normalizeLocaleNumber("123.050", "", ".") == "123050"); + CHECK(normalizeLocaleNumber("1234.050", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber(".050", "", ".") == std::nullopt); + + // ...followed by exactly three, at every group and at the end of the + // integer part. + CHECK(normalizeLocaleNumber("1.05", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.0500", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.050.", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000.00", "", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000.000", "", ".") == "1000000"); + + // ...and never after the decimal separator. + CHECK(normalizeLocaleNumber("1,050.25", ",", ".") == std::nullopt); + CHECK(normalizeLocaleNumber("1.000,250.25", ",", ".") == std::nullopt); +} + +TEST_CASE("normalizeLocaleNumber: every well-formed locale entry still normalises", "[render][locale][morph574]") { + // The validation must not cost a single legitimate entry -- this is the + // half of the change that the rejection cases cannot show. + CHECK(normalizeLocaleNumber("1.050,25", ",", ".") == "1050.25"); + CHECK(normalizeLocaleNumber("-1.050,25", ",", ".") == "-1050.25"); + CHECK(normalizeLocaleNumber("1.000.000,25", ",", ".") == "1000000.25"); + CHECK(normalizeLocaleNumber("1050,25", ",", ".") == "1050.25"); // ungrouped + CHECK(normalizeLocaleNumber("1,050.25", ".", ",") == "1050.25"); // en-US + CHECK(normalizeLocaleNumber(std::string{"1"} + std::string{kNarrowNbsp} + "050,25", ",", kNarrowNbsp) == + "1050.25"); // fr-FR + + // A grouped entry round-trips through the display direction unchanged. + auto const canonical = normalizeLocaleNumber("1.000.000,25", ",", "."); + REQUIRE(canonical.has_value()); + CHECK(formatCanonicalNumber(*canonical, ",", ".") == "1.000.000,25"); +} From 93b19aa56d09d5ea18767efd594a33db15323e39 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 08:48:59 +0200 Subject: [PATCH 3/7] Record both defects in the changelog Two entries under Fixed: the locale group-separator validation, with the wrong values it used to produce, and the provenance-chain stack overflow, with the measurement behind leaving MORPH_QUANTITY_PROVENANCE at 1. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9b8dba4..4d1329568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -157,6 +157,36 @@ API surface). ### Fixed +- **A locale-formatted entry could submit ten times what the user typed.** + `morph::render::normalizeLocaleNumber` dropped every occurrence of the group + separator unconditionally, with no check on placement, so a de-DE user typing + the US form `"1.5"` into a price field submitted `15` — a perfectly valid + number that nothing downstream could recognise as wrong. `"1.50"` gave `150`, + `"1.2.3.4"` gave `1234`, the en-US mirror image `"1,5"` gave `15`, and two + equal separators silently ate the decimal. A group separator is now dropped + only where one can legally be — preceded by one to three digits, followed by + exactly three, never after the decimal separator — and everything else is + reported as malformed; equal separators are rejected outright. Every + well-formed entry (`"1.050,25"`, `"1.000.000,25"`, an ungrouped `"1050,25"`, + fr-FR's U+202F grouping) normalises exactly as before. The JavaScript mirror + in `src/qt/forms/qml/DynamicForm.qml` produced byte-identical wrong answers + and carries the identical validation now. See `docs/spec/forms/forms.md`, + "Locale data formatting"; morph#574. +- **A deep `Quantity` derivation overflowed the stack.** A running total + (`total = total + one` in a loop) records one `ASTNode` per iteration chained + through `left`, and every walk over that chain was recursive: destroying a + 21,000-node chain segfaulted at `-O0` while surviving 200,000 at `-O2`, where + clang rewrites the release into a loop, and `equation()` segfaulted at 25,000 + nodes at every optimisation level. `~ASTNode` now releases the chain through + a local worklist and all four `equation()` traversals run over an explicit + stack, with the symbolic and substituted renderings unified into one stack + machine; `equation()` output is unchanged. `MORPH_QUANTITY_PROVENANCE` keeps + its default of `1` — the measured cost is real (54,056 KB and 0.034 s against + 12,236 KB and 0.006 s for a 200,000-iteration total) but the toggle changes + observable behaviour, not just cost, so a bulk path that never calls + `equation()` should set it to `0` rather than have it flipped underneath every + build that did not. See `docs/spec/util/quantity_type.md`, *Provenance* and + *Limitations*; morph#574. - **Both of the cross-field rule vocabulary's safety checks were bypassed by wrapping a rule in one combinator.** Unsatisfiability detection stopped at the first compound node, because it skipped any node without a `fields` key From ec03d51d49759250a748eddb6a6fa96d0acdf42b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 08:55:37 +0200 Subject: [PATCH 4/7] Split the grouping rule out, and make the equation header self-contained Three things clang-tidy-diff would have reported on the changed lines, all worth doing on their own terms: - `normalizeLocaleNumber` reached a cognitive complexity of 38 against a threshold of 25, because the grouping rule was extra state threaded through the normalising scan. "The grouping is well placed" and "the digits convert" are two separate statements about an entry, and reading them as one made neither clear. The placement rule is now `detail::groupingIsWellPlaced`, a pass of its own that can be read without the scan around it. Verified behaviour-preserving: the 47-case differential over both control edges is byte-identical before and after, and still identical to the QML mirror. - `detail/quantity_equation.hpp` was not self-contained -- it is included from `util/quantity.hpp` after `ASTNode` exists, so a tool that opens it on its own saw `unknown type name 'ASTNode'` on every line. Local clang-tidy on the unmodified file reports 20 such errors and a cascade of bogus findings ("method 'countRefs' can be made static" -- it accesses three members). It now includes `util/quantity.hpp` back, which `#pragma once` makes free and which nothing in that header past the include point can notice. Checked with `clang++ -fsyntax-only -x c++-header` on the file alone. - `misc-const-correctness` on the worklist handle in `~ASTNode`. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- include/morph/detail/quantity_equation.hpp | 41 +++++--- include/morph/render/locale_format.hpp | 104 +++++++++++++++------ include/morph/util/quantity.hpp | 2 +- 3 files changed, 103 insertions(+), 44 deletions(-) diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index dc916a79c..0ef492fd8 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -20,6 +20,14 @@ #include #include "../attributes.hpp" +// Included back deliberately, and not circular: `util/quantity.hpp` includes +// this file at the very end, `#pragma once` makes the inner visit a no-op, and +// nothing in that header past the include point needs anything defined here. +// It is what makes this file self-contained, which matters because a tool that +// opens a header on its own -- clang-tidy analysing a changed header, an IDE, +// include-what-you-use -- otherwise sees `unknown type name 'ASTNode'` on +// every line and reports a cascade of findings about code that compiles fine. +#include "../util/quantity.hpp" #include "../util/rational.hpp" namespace morph::units::detail { @@ -166,7 +174,7 @@ struct EquationRenderer { /// @param expandRoot Whether to expand @p root itself (rather than label it). void assignLabels(const ASTNode* root, bool expandRoot) { std::vector pending; - pending.push_back(LabelFrame{root, expandRoot}); + pending.push_back(LabelFrame{.node = root, .expandThis = expandRoot}); while (!pending.empty()) { LabelFrame const frame = pending.back(); pending.pop_back(); @@ -184,10 +192,10 @@ struct EquationRenderer { continue; } if (node->right) { - pending.push_back(LabelFrame{node->right.get(), false}); + pending.push_back(LabelFrame{.node = node->right.get(), .expandThis = false}); } if (node->left) { - pending.push_back(LabelFrame{node->left.get(), false}); + pending.push_back(LabelFrame{.node = node->left.get(), .expandThis = false}); } } } @@ -203,7 +211,7 @@ struct EquationRenderer { bool const rightNeedsParens = (right.precedence < precedence) || (right.precedence == precedence && (op == "-" || op == "/")); std::string const rightText = rightNeedsParens ? "(" + right.text + ")" : right.text; - return Rendered{leftText + " " + op + " " + rightText, precedence}; + return Rendered{.text = leftText + " " + op + " " + rightText, .precedence = precedence}; } /// @brief Renders a unary-negation subexpression. @@ -211,7 +219,7 @@ struct EquationRenderer { /// @return The combined rendering. [[nodiscard]] static Rendered combineUnary(const Rendered& operand) { std::string const text = (operand.precedence <= 1) ? "-(" + operand.text + ")" : "-" + operand.text; - return Rendered{text, 3}; + return Rendered{.text = text, .precedence = 3}; } /// @brief The rendering of a node that stops the walk — a name, a `cK` @@ -224,19 +232,19 @@ struct EquationRenderer { [[nodiscard]] std::optional atomRendering(const ASTNode* node, bool expandThis, RenderMode mode) const { if (mode == RenderMode::symbolic) { if (node->name.has_value()) { - return Rendered{"\"" + *node->name + "\"", 100}; + return Rendered{.text = "\"" + *node->name + "\"", .precedence = 100}; } if (!expandThis && isPlaceholder(node)) { - return Rendered{"c" + std::to_string(labelIndex.at(node)), 100}; + return Rendered{.text = "c" + std::to_string(labelIndex.at(node)), .precedence = 100}; } } else if (!expandThis && (node->name.has_value() || isPlaceholder(node))) { - return Rendered{formatOptional(nodeValue(*node)), 100}; + return Rendered{.text = formatOptional(nodeValue(*node)), .precedence = 100}; } if (isLeafNode(*node)) { - return Rendered{formatOptional(node->current.lhs), 100}; + return Rendered{.text = formatOptional(node->current.lhs), .precedence = 100}; } if (isConversionNode(*node)) { - return Rendered{formatOptional(node->current.result), 100}; + return Rendered{.text = formatOptional(node->current.result), .precedence = 100}; } return std::nullopt; } @@ -263,7 +271,7 @@ struct EquationRenderer { /// @return The rendering of @p root. [[nodiscard]] Rendered render(const ASTNode* root, bool expandRoot, RenderMode mode) const { std::vector stack; - stack.push_back(RenderFrame{root, expandRoot, 0, Rendered{}}); + stack.push_back(RenderFrame{.node = root, .expandThis = expandRoot}); Rendered finished; while (!stack.empty()) { RenderFrame& top = stack.back(); @@ -276,14 +284,17 @@ struct EquationRenderer { top.stage = 1; if (top.node->left) { const ASTNode* child = top.node->left.get(); - stack.push_back(RenderFrame{child, false, 0, Rendered{}}); + stack.push_back(RenderFrame{.node = child, .expandThis = false}); continue; } - finished = Rendered{formatOptional(top.node->current.lhs), 100}; + finished = Rendered{.text = formatOptional(top.node->current.lhs), .precedence = 100}; continue; } if (top.stage == 1) { top.left = std::move(finished); + // Put the slot back into a known state rather than leaving it + // moved-from: it is read again below, on the pop that follows. + finished = Rendered{}; bool const hasRight = top.node->right || top.node->current.rhs.has_value(); if (!hasRight) { finished = combineUnary(top.left); @@ -293,10 +304,10 @@ struct EquationRenderer { top.stage = 2; if (top.node->right) { const ASTNode* child = top.node->right.get(); - stack.push_back(RenderFrame{child, false, 0, Rendered{}}); + stack.push_back(RenderFrame{.node = child, .expandThis = false}); continue; } - finished = Rendered{formatOptional(top.node->current.rhs), 100}; + finished = Rendered{.text = formatOptional(top.node->current.rhs), .precedence = 100}; continue; } finished = combine(top.node->current.operation, top.left, finished); diff --git a/include/morph/render/locale_format.hpp b/include/morph/render/locale_format.hpp index fcacf495d..a23fb37ea 100644 --- a/include/morph/render/locale_format.hpp +++ b/include/morph/render/locale_format.hpp @@ -30,6 +30,76 @@ namespace morph::render { +namespace detail { + +/// @brief Whether every group separator in @p text sits where a group +/// separator can legally sit. +/// +/// The rule, in one place so it can be read on its own: a group separator is +/// preceded by one to three digits (the first group), or by exactly three +/// (every later one); it is followed by exactly three more digits, at each +/// group and at the end of the integer part; and it never appears after the +/// decimal separator. A locale with no grouping (@p groupSeparator empty) has +/// nothing to place, so it trivially passes. +/// +/// This is a pass of its own rather than extra state inside the normalising +/// scan below: "the grouping is well placed" and "the digits convert" are two +/// separate statements about the entry, and reading them as one made neither +/// clear. +/// +/// Characters this function does not recognise are simply not digits — the +/// normalising scan is what rejects them, and it rejects them whatever this +/// pass concludes. +/// @param text The locale-formatted entry. +/// @param decimalSeparator The locale's decimal-point string; may be empty. +/// @param groupSeparator The locale's digit-grouping string; empty means the +/// locale does not group. +/// @return `true` when the grouping is well placed (or absent). +[[nodiscard]] inline bool groupingIsWellPlaced(std::string_view text, std::string_view decimalSeparator, + std::string_view groupSeparator) { + if (groupSeparator.empty()) { + return true; + } + constexpr std::size_t kGroupSize = 3; + std::size_t digits = 0; + bool sawGroup = false; + bool sawDecimal = false; + + for (std::size_t i = 0; i < text.size();) { + const std::string_view rest = text.substr(i); + if (rest.starts_with(groupSeparator)) { + bool const opensAGroup = sawGroup ? digits == kGroupSize : (digits >= 1 && digits <= kGroupSize); + if (sawDecimal || !opensAGroup) { + return false; + } + sawGroup = true; + digits = 0; + i += groupSeparator.size(); + continue; + } + if (!decimalSeparator.empty() && rest.starts_with(decimalSeparator)) { + if (sawGroup && digits != kGroupSize) { + return false; // the last group of the integer part is short + } + sawDecimal = true; + digits = 0; + i += decimalSeparator.size(); + continue; + } + // i is bounded by the loop condition. + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) + char const chr = text[i]; + digits = (chr >= '0' && chr <= '9') ? digits + 1 : 0; + ++i; + } + + // An ungrouped fractional part ends the number, so the trailing check only + // applies when the integer part was the last thing scanned. + return !sawGroup || sawDecimal || digits == kGroupSize; +} + +} // namespace detail + /// @brief Converts a locale-formatted numeric string to canonical /// (`-?[0-9]+(\.[0-9]+)?`) text. /// @@ -83,42 +153,27 @@ namespace morph::render { if (!groupSeparator.empty() && groupSeparator == decimalSeparator) { return std::nullopt; // one string cannot play both roles: see above } + if (!detail::groupingIsWellPlaced(text, decimalSeparator, groupSeparator)) { + return std::nullopt; // a separator off a group boundary: see above + } std::string canonical; canonical.reserve(text.size()); bool sawDecimal = false; bool sawAnyOutput = false; - // Grouping state: how many digits since the last group separator (or since - // the start), and whether any group separator has been seen at all. - std::size_t digitsInGroup = 0; - bool sawGroup = false; - constexpr std::size_t kGroupSize = 3; for (std::size_t i = 0; i < text.size();) { const std::string_view rest = text.substr(i); if (!groupSeparator.empty() && rest.starts_with(groupSeparator)) { - if (sawDecimal) { - return std::nullopt; // grouping belongs to the integer part only - } - // The first group is one to three digits ("1.050", "12.050", - // "123.050"); every later one is exactly three. - bool const wellPlaced = - sawGroup ? digitsInGroup == kGroupSize : (digitsInGroup >= 1 && digitsInGroup <= kGroupSize); - if (!wellPlaced) { - return std::nullopt; // not a group boundary: malformed - } - sawGroup = true; - digitsInGroup = 0; + // Placement was settled above, so by here the separator is display + // only and is never carried into the output. i += groupSeparator.size(); - continue; // grouping is display-only; never carried into the output + continue; } if (!decimalSeparator.empty() && rest.starts_with(decimalSeparator)) { if (sawDecimal) { return std::nullopt; // a second decimal separator: malformed } - if (sawGroup && digitsInGroup != kGroupSize) { - return std::nullopt; // the last group is short: "1.5" in de-DE - } sawDecimal = true; canonical += '.'; // The decimal point *is* output: without this, the `chr == '-'` @@ -142,7 +197,6 @@ namespace morph::render { canonical += chr; } else if (chr >= '0' && chr <= '9') { canonical += chr; - ++digitsInGroup; } else { return std::nullopt; // any other character is malformed } @@ -150,12 +204,6 @@ namespace morph::render { ++i; } - // A grouped integer part has to end on a group boundary too: "1.05" and - // "1.050." are as malformed as "1.5" is. - if (sawGroup && !sawDecimal && digitsInGroup != kGroupSize) { - return std::nullopt; - } - if (canonical.empty() || canonical == "-") { return std::nullopt; } diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index 2beb0934c..e28b46338 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -586,7 +586,7 @@ struct ASTNode { pending.push_back(std::move(right)); } while (!pending.empty()) { - std::shared_ptr node = std::move(pending.back()); + std::shared_ptr const node = std::move(pending.back()); pending.pop_back(); if (node.use_count() == 1) { if (node->left) { From f72fd829a2aecc1a0c106d999cdb5f4a3b9b3d50 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 09:07:19 +0200 Subject: [PATCH 5/7] Price the deep-chain tests against the depths where each one actually bites The first version of these two cases used 100,000 nodes for both, on the ticket's wording rather than on a measurement. Two things came out of measuring: - `equation()` renders the whole chain into one string by repeated concatenation, so its cost is quadratic in the depth: 83 s under ASan at 100,000 against 32 s at 70,000, for the same evidence. - The claim that the `equation()` case "has no optimiser escape" was too strong. Optimisation shrinks the frames rather than eliminating the recursion: unfixed code returns normally at 50,000 and SIGSEGVs at 60,000 under clang 22 `-O2`, where at `-O0` it SIGSEGVs at 25,000. gcc 16 `-O2` SIGSEGVs already at 40,000. So a 40,000-node case would have passed vacuously in a clang Release leg -- the exact shape of check this repository keeps finding. 70,000 sits above every measured survival depth and keeps the cost down. The destruction case stays at 100,000: it is linear, and the ticket asked for that depth. The comment now carries all five measurements and says plainly that the destruction case is load-bearing in an unoptimised build only, naming the CI legs where that is true. Verified after the change: both cases still SIGSEGV on unfixed headers in the clang-debug suite, and both pass on fixed ones. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- tests/test_quantity.cpp | 66 +++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/tests/test_quantity.cpp b/tests/test_quantity.cpp index b7571e9ac..932954810 100644 --- a/tests/test_quantity.cpp +++ b/tests/test_quantity.cpp @@ -678,28 +678,52 @@ TEST_CASE("formatRationalDecimal: an un-canonicalised INT64_MIN numerator render // be recursive -- the compiler-generated `~ASTNode` and every traversal in // `equation()` -- so a long enough chain ran the stack out. // -// **What makes these two cases evidence, and what does not.** The destruction -// case is load-bearing in an *unoptimised* build only: measured on clang 20 and -// clang 22 with an 8 MiB stack, unfixed code segfaults on destruction at 21,000 -// nodes at `-O0` and survives 200,000 at `-O2`, because clang rewrites the -// recursive release into a loop. So this case fails on unfixed code in every -// Debug configuration CI runs (gcc-debug, clang-debug, the three sanitizer -// legs, cl-debug) and passes vacuously in a Release one. The `equation()` case -// has no such escape: its frames hold live `Rendered` strings across the call, -// no optimiser can turn it into a loop, and unfixed code segfaulted inside -// `renderSymbolic` at 25,000 nodes. +// **What makes each case evidence, and where each one is vacuous.** Both +// depths below were measured against unfixed code with an 8 MiB stack, not +// guessed: +// +// destruction, clang -O0 returns at 20,800 nodes, SIGSEGV at 21,000 +// destruction, clang -O2 returns at 200,000 -- clang rewrites the +// recursive release into a loop, so no depth fails +// equation(), clang -O0 returns at 24,000, SIGSEGV at 25,000 +// equation(), clang -O2 returns at 50,000, SIGSEGV at 60,000 +// equation(), gcc 16 -O2 SIGSEGV already at 40,000 +// +// So the destruction case is load-bearing in an unoptimised build only, and +// passes vacuously in a Release one -- "crashes in Debug, survives in Release" +// is the defect's own signature, and it is not reproducible any other way. CI +// runs it where it bites: gcc-debug, clang-debug, the three sanitizer legs and +// cl-debug are all -O0. The `equation()` case has no such escape: optimisation +// shrinks its frames but cannot eliminate the recursion, because they hold +// live `Rendered` strings across the call, so its depth is chosen to fail in +// every configuration rather than only the unoptimised ones. namespace { +// Destruction is linear in the chain, so the depth the ticket asked for costs +// nothing to run. constexpr int kDeepChainNodes = 100000; + +// equation() renders the whole chain into one string by repeated +// concatenation, which is quadratic in the depth, so this one is priced: 70,000 +// nodes cost 32 s under ASan where 100,000 cost 83 s. It stays above the +// highest measured survival depth (50,000, clang -O2) with margin, which is +// what keeps it from passing vacuously in an optimised build. +constexpr int kDeepEquationNodes = 70000; + +// Builds `0 + one + one + ...`, one retained ASTNode per term. +[[nodiscard]] Euro runningTotal(int terms) { + Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; + Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; + for (int i = 0; i < terms; ++i) { + total = total + one; + } + return total; +} } // namespace TEST_CASE("A 100000-node provenance chain is destroyed without overflowing the stack", "[quantity][provenance][morph574]") { { - Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; - Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; - for (int i = 0; i < kDeepChainNodes; ++i) { - total = total + one; - } + Euro const total = runningTotal(kDeepChainNodes); REQUIRE(total.hasValue()); REQUIRE(total.value()->toDouble() == static_cast(kDeepChainNodes)); // The chain is released here. On unfixed code this is a SIGSEGV at -O0, @@ -709,20 +733,16 @@ TEST_CASE("A 100000-node provenance chain is destroyed without overflowing the s SUCCEED("the chain was released without a stack overflow"); } -TEST_CASE("equation() walks a 100000-node provenance chain without overflowing the stack", +TEST_CASE("equation() walks a 70000-node provenance chain without overflowing the stack", "[quantity][provenance][equation][morph574]") { - Euro total{Rational{Numerator{0}, Denominator{1}, DecimalPlaces{2}}}; - Euro const one{Rational{Numerator{1}, Denominator{1}, DecimalPlaces{2}}}; - for (int i = 0; i < kDeepChainNodes; ++i) { - total = total + one; - } + Euro const total = runningTotal(kDeepEquationNodes); auto const lines = total.equation(); // Formula, substitution, result, and one `where` line: the single `one` - // leaf is referenced 100,000 times, so it earns exactly one placeholder. + // leaf is referenced 70,000 times, so it earns exactly one placeholder. REQUIRE(lines.size() == 4); CHECK(lines[0].starts_with("0 + c1 + c1")); CHECK(lines[1].starts_with(" = 0 + 1 + 1")); - CHECK(lines[2] == " = 100000"); + CHECK(lines[2] == " = 70000"); CHECK(lines[3] == "where c1 = 1"); } From 5e72f143510b28cd17fa01494b0c423a63f1b895 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 09:08:06 +0200 Subject: [PATCH 6/7] Replace the depth claims with the measurements behind them The spec and the two header comments carried a single -O0 number and an assertion that equation()'s recursion had no optimiser escape. Both are now the measured table: destruction has no failing depth at all under clang -O2, while equation()'s limit only moves with optimisation (25,000 at clang -O0, 60,000 at clang -O2, 40,000 under gcc 16 -O2). Also corrects the compiler these were measured on -- clang 22.1.8 and gcc 16.2.1, not clang 20. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- docs/spec/util/quantity_type.md | 25 ++++++++++++++++------ include/morph/detail/quantity_equation.hpp | 13 +++++------ include/morph/util/quantity.hpp | 2 +- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/docs/spec/util/quantity_type.md b/docs/spec/util/quantity_type.md index ce7577820..d0db6bae7 100644 --- a/docs/spec/util/quantity_type.md +++ b/docs/spec/util/quantity_type.md @@ -463,12 +463,25 @@ the stack, so none of the walks is recursive: recursive call would: render-or-descend-left, take-left-descend-right, combine. -Measured (morph#574, clang 20 and clang 22, `-O0`, 8 MiB stack): before this, -destroying a 21,000-node chain segfaulted and `equation()` segfaulted inside -`renderSymbolic` at 25,000 nodes. The destruction case *survived* 200,000 nodes -at `-O2`, because clang rewrites that particular chain into a loop — "crashes in -Debug, survives in Release" — which is precisely why the flattening is a -specified property of the type rather than something left to the optimiser. +Measured against the recursive code (morph#574, 8 MiB stack, clang 22.1.8 and +gcc 16.2.1): + +| walk | build | last depth that returned | first that segfaulted | +|---|---|---|---| +| destruction | clang `-O0` | 20,800 | 21,000 | +| destruction | clang `-O2` | 200,000 | — none found | +| `equation()` | clang `-O0` | 24,000 | 25,000 | +| `equation()` | clang `-O2` | 50,000 | 60,000 | +| `equation()` | gcc `-O2` | — | 40,000 | + +Two things follow, and both are why the flattening is a specified property of +the type rather than something left to the optimiser. Destruction *had no +failing depth at all* under `-O2`, because clang rewrites that particular +`shared_ptr` chain into a loop — so the defect crashed in Debug and survived in +Release, the worst signature a defect can have. `equation()`, whose frames hold +live `Rendered` strings across the call, could not be rewritten that way: +optimisation only moved its limit, and gcc's limit was lower than clang's +unoptimised one. **Nodes are immutable once built.** No operation ever mutates an existing `ASTNode` — arithmetic, conversion, and `named()` each allocate a *new* node diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 0ef492fd8..1fe710b7a 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -254,12 +254,13 @@ struct EquationRenderer { /// The walk is an explicit stack rather than recursion because the /// derivation of a running total (`total = total + x` in a loop) is a /// linear chain one node deep per iteration, and a recursive walk of it - /// runs the stack out: measured (morph#574, clang 20, `-O0`, 8 MiB stack) - /// `equation()` segfaulted inside the old recursive `renderSymbolic` at a - /// 25,000-node chain and returned normally at 24,000. Unlike the operand - /// destructor chain, no optimiser rewrite hides this one — the frames hold - /// live `Rendered` strings across the call — so the depth limit was real in - /// every build. + /// runs the stack out. Measured on the recursive code (morph#574, 8 MiB + /// stack): `equation()` returned at 24,000 nodes and segfaulted inside + /// `renderSymbolic` at 25,000 under clang `-O0`, returned at 50,000 and + /// segfaulted at 60,000 under clang `-O2`, and segfaulted already at + /// 40,000 under gcc `-O2`. Optimisation only moved the limit: unlike the + /// operand destructor chain, this recursion cannot be rewritten into a + /// loop, because the frames hold live `Rendered` strings across the call. /// /// Each frame resumes where its recursive twin would have: stage 0 renders /// the node or descends left, stage 1 takes the left result and descends diff --git a/include/morph/util/quantity.hpp b/include/morph/util/quantity.hpp index e28b46338..a4585a877 100644 --- a/include/morph/util/quantity.hpp +++ b/include/morph/util/quantity.hpp @@ -566,7 +566,7 @@ struct ASTNode { /// The compiler-generated destructor releases that chain recursively — /// `~shared_ptr` -> `~ASTNode` -> `~shared_ptr` -> ... — one stack frame /// per node, and a long enough chain runs the stack out. Measured - /// (morph#574, clang 20, `-O0`, 8 MiB stack): a 21,000-node chain + /// (morph#574, clang 22, `-O0`, 8 MiB stack): a 21,000-node chain /// segfaults on destruction, while an optimised build survives 200,000 /// because clang turns the same chain into a loop. "Crashes in Debug, /// survives in Release" is the worst signature a defect can have, so the From 3a1effd0e9565e22fcd9a6d4fa0801f57b99d3be Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sat, 19 Sep 2026 09:47:38 +0200 Subject: [PATCH 7/7] Give the install/export gate a fixture that cannot be repaired away CI caught this, which is the gate working. `test_check_install_export.sh`'s "the verified header sets widened back to include detail/" case proves `INTERFACE_HEADER_SETS_TO_VERIFY HEADERS` is load-bearing by deleting it and requiring the build to break. It broke because `detail/quantity_equation.hpp` was not self-contained. This branch made it self-contained, every other detail/ header already was, and with no broken header left in the tree the mutation stopped breaking anything: error: NOT caught: the verified header sets widened back to include detail/ -- the gate passed a tree it should reject Borrowing a real header's brokenness made that case depend on it staying broken, so the next person to fix one would have hit this wall too. The mutation now plants its own `detail/selftest_not_standalone.hpp` -- a header calling an undeclared `morphSelfTestUndeclaredHelper()` -- and adds it to the detail/ FILE_SET, so the unmutated property keeps it out of the verified set and deleting the property pulls it in. Same three needles as before (the checker's own wording, the header path, the identifier), all still toolchain-independent. The two paths go on one line because a portable `sed` replacement cannot insert a newline, and `FILES` accepts that. The CMakeLists comment justified the exclusion by quantity_equation.hpp being non-self-contained, which is now false. The rule it encodes is not "these happen to be broken" but "standalone compilation is a promise about public headers", so it says that instead, and notes that every detail/ header compiling standalone today is a convenience rather than the rule. Also here, two branch-coverage repairs for the same reason (`include/morph/detail` and `include/morph/render` both carry a 97% branch floor in scripts/check_branch_coverage.py, measured at 100%): - `countRefs` and `assignLabels` push children unconditionally and handle null on pop, as the recursive form did on entry. Guarding the push instead left `root != nullptr` with an arm nothing could take, since `equation()` checks for a null root before calling. - Two locale cases for arms nothing else reaches: a grouping locale whose entry carries no separator at all, and a non-digit inside one. Refs morph#574. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6 --- CMakeLists.txt | 18 ++++++---- include/morph/detail/quantity_equation.hpp | 21 +++++------- scripts/test_check_install_export.sh | 40 +++++++++++++++++----- tests/test_render_locale_format.cpp | 8 +++++ 4 files changed, 61 insertions(+), 26 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 82a05bf2a..7ebc6a13c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -238,15 +238,21 @@ target_sources(morph ) # The detail/ headers that public headers include. They are not public surface -# and are deliberately outside the verified set -- quantity_equation.hpp is -# included from partway down quantity.hpp and is not self-contained, so -# VERIFY_INTERFACE_HEADER_SETS would fail on it -- but they must still ship, -# or an installed morph/util/quantity.hpp cannot find fixed_string.hpp and the -# install compiles nowhere (morph#232). A second FILE_SET gets them installed -# without adding them to what is standalone-compiled; +# and are deliberately outside the verified set -- standalone compilation is a +# promise made about morph's public headers, and a detail/ header is free to be +# a fragment included partway down the one that owns it -- but they must still +# ship, or an installed morph/util/quantity.hpp cannot find fixed_string.hpp +# and the install compiles nowhere (morph#232). A second FILE_SET gets them +# installed without adding them to what is standalone-compiled; # INTERFACE_HEADER_SETS_TO_VERIFY, which otherwise defaults to *every* # interface header set, keeps the verification list exactly as it was. # +# As it happens every detail/ header compiles standalone today (morph#574 made +# the last one that did not, quantity_equation.hpp, self-contained). That is a +# convenience, not the rule this exclusion encodes, which is why +# scripts/test_check_install_export.sh plants its own broken header rather than +# leaning on one of these staying broken. +# # Adding a detail/ header here is easy to forget, and forgetting is silent: # `cmake --install` still exits 0, and the drift guard above skips detail/ by # design. That is how morph#540 shipped -- `forms/detail/session_common.hpp` diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 1fe710b7a..7d6445bda 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -140,13 +140,14 @@ struct EquationRenderer { /// @param root The node to start from (may be null). void countRefs(const ASTNode* root) { std::vector pending; - if (root != nullptr) { - pending.push_back(root); - } + pending.push_back(root); while (!pending.empty()) { const ASTNode* node = pending.back(); pending.pop_back(); - if (seen.contains(node)) { + // Children are pushed unconditionally and the null case handled + // here, exactly as the recursive form handled it on entry: the + // absent operand of a leaf, unary or scalar step is a null child. + if (node == nullptr || seen.contains(node)) { continue; } seen.insert(node); @@ -155,12 +156,12 @@ struct EquationRenderer { } if (node->left) { ++refCount[node->left.get()]; - pending.push_back(node->left.get()); } if (node->right) { ++refCount[node->right.get()]; - pending.push_back(node->right.get()); } + pending.push_back(node->left.get()); + pending.push_back(node->right.get()); } } @@ -191,12 +192,8 @@ struct EquationRenderer { if (isAtomNode(*node)) { continue; } - if (node->right) { - pending.push_back(LabelFrame{.node = node->right.get(), .expandThis = false}); - } - if (node->left) { - pending.push_back(LabelFrame{.node = node->left.get(), .expandThis = false}); - } + pending.push_back(LabelFrame{.node = node->right.get(), .expandThis = false}); + pending.push_back(LabelFrame{.node = node->left.get(), .expandThis = false}); } } diff --git a/scripts/test_check_install_export.sh b/scripts/test_check_install_export.sh index 37ef80467..9ff216e4e 100755 --- a/scripts/test_check_install_export.sh +++ b/scripts/test_check_install_export.sh @@ -185,21 +185,45 @@ expect_caught "the detail/ header set dropped from the install" \ "detail/instance_directory.hpp" # Bug 2: INTERFACE_HEADER_SETS_TO_VERIFY defaults to *every* interface header -# set, and quantity_equation.hpp is included partway down quantity.hpp and is -# not self-contained. This is the one case the slow phase exists for, so it is -# the one case that runs it. +# set, including the detail/ one, which is deliberately not held to compiling +# standalone. This is the one case the slow phase exists for, so it is the one +# case that runs it. # -# `formatOptionalDecimal` is the identifier the header uses before quantity.hpp -# has declared it. Both compilers name it -- clang as "use of undeclared +# The mutation plants its own non-self-contained detail header rather than +# relying on a real one being broken. It used to point at +# morph/detail/quantity_equation.hpp, which was included partway down +# quantity.hpp and used `formatOptionalDecimal` before that header declared it. +# morph#574 made it self-contained -- a good change for clang-tidy and for any +# tool that opens a header on its own -- and this case went red, correctly: no +# detail/ header was left that VERIFY_INTERFACE_HEADER_SETS would reject, so +# deleting the property no longer broke anything and the mutation could not be +# caught. That is a gate resting on an accident of the tree. Borrowing a real +# header's brokenness meant this case silently depended on it staying broken, +# and the next person to fix one would have hit the same wall. A planted +# fixture cannot be repaired out from under the gate. +# +# The header goes into the detail/ FILE_SET (two paths on one line, which +# `FILES` accepts, because a portable `sed` replacement cannot insert a +# newline), so the unmutated property keeps it out of the verified set and +# deleting that property pulls it in. +# +# `morphSelfTestUndeclaredHelper` is the identifier the planted header calls +# without declaring. Both compilers name it -- clang as "use of undeclared # identifier", GCC as "was not declared in this scope" -- and it appears # nowhere else in the output, which the file name alone cannot promise (ninja # prints that on the progress line for a unit that compiled fine). expect_caught "the verified header sets widened back to include detail/" \ - "edit CMakeLists.txt -e '/INTERFACE_HEADER_SETS_TO_VERIFY HEADERS/d'" \ + "edit CMakeLists.txt \ + -e '/INTERFACE_HEADER_SETS_TO_VERIFY HEADERS/d' \ + -e 's@include/morph/detail/fixed_string.hpp@include/morph/detail/fixed_string.hpp include/morph/detail/selftest_not_standalone.hpp@' \ + && printf '%s\n' \ + '#pragma once' \ + 'inline int morphSelfTestNotStandalone() { return morphSelfTestUndeclaredHelper(); }' \ + > include/morph/detail/selftest_not_standalone.hpp" \ full \ "morph's interface header sets do not all compile standalone" \ - "morph/detail/quantity_equation.hpp" \ - "formatOptionalDecimal" + "morph/detail/selftest_not_standalone.hpp" \ + "morphSelfTestUndeclaredHelper" # Bug 3: install(EXPORT NAMESPACE morph::) prefixes the target name, so # morph_net is exported as morph::morph_net while every in-tree alias, the diff --git a/tests/test_render_locale_format.cpp b/tests/test_render_locale_format.cpp index 8ba72d190..73b59f38d 100644 --- a/tests/test_render_locale_format.cpp +++ b/tests/test_render_locale_format.cpp @@ -237,6 +237,14 @@ TEST_CASE("normalizeLocaleNumber: a group separator must sit on a group boundary // ...and never after the decimal separator. CHECK(normalizeLocaleNumber("1,050.25", ",", ".") == std::nullopt); CHECK(normalizeLocaleNumber("1.000,250.25", ",", ".") == std::nullopt); + + // A grouping locale with no separator in the entry at all: there is no + // placement to be wrong about, and the digits pass through. + CHECK(normalizeLocaleNumber("1050", ",", ".") == "1050"); + + // A non-digit inside a grouping locale restarts the digit run rather than + // being counted into it -- and is malformed for the ordinary reason. + CHECK(normalizeLocaleNumber("1.0x0", "", ".") == std::nullopt); } TEST_CASE("normalizeLocaleNumber: every well-formed locale entry still normalises", "[render][locale][morph574]") {