From de34fbafdb3afbcc2339aa6c0ee0604066a1fe90 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 20:20:48 -0400 Subject: [PATCH] Close two defeatable ReleaseProof guards and fix the static-lane skip's dead comparison ReleaseProof{} and copy-reuse both compiled under this project's C++17; user-provided ctor, deleted copy ctor and friend close them. The skip now compares stored values, not norms, so it actually fires. Abort downgraded to a debug assert. --- src/core/instrument/engine/live_params.h | 12 ++-- src/shell/instrument/CLAUDE.md | 2 +- src/shell/instrument/automation_channel.h | 81 +++++++++++++++------- src/shell/instrument/instrument_params.cpp | 35 ++++++++-- 4 files changed, 95 insertions(+), 35 deletions(-) diff --git a/src/core/instrument/engine/live_params.h b/src/core/instrument/engine/live_params.h index b8feebb..6b12332 100644 --- a/src/core/instrument/engine/live_params.h +++ b/src/core/instrument/engine/live_params.h @@ -70,11 +70,13 @@ struct LiveValues { // The seqlock copies the block as raw bytes, which is only defensible for a plain value type. static_assert(std::is_trivially_copyable_v, "the live block is copied under a seqlock — it must stay a plain value"); -// Guards operator== against silent staleness: a member added to the struct above changes this -// size, so the assert fails at the new member's own commit instead of leaving a live control -// that never reaches a sounding voice with no compiler or test signal. Confirmed 352 bytes, -// MSVC 19.44 x64, Release (`SizeProbe`, an incomplete-template size probe -// whose error message reports the value). Bump the literal AND operator== together. +// A SIZE-CHANGING edit only: padding can absorb a member added beside an existing one (a bool +// beside splineActive, a fifth FilterSettings float) without moving this literal at all, so this +// assert is NOT the guard against a forgotten operator== field — +// testEveryFieldOfLiveValuesIsCompared (test_live_params.cpp) is that guard, poisoning one leaf +// at a time. This assert only catches an edit that changes sizeof(LiveValues) itself. Confirmed +// 352 bytes, MSVC 19.44 x64, Release (`SizeProbe`, an incomplete-template +// size probe whose error message reports the value). Bump the literal AND operator== together. static_assert(sizeof(LiveValues) == 352, "a member was added or removed — extend operator== in live_params.cpp to match"); diff --git a/src/shell/instrument/CLAUDE.md b/src/shell/instrument/CLAUDE.md index 020e0f9..77c9033 100644 --- a/src/shell/instrument/CLAUDE.md +++ b/src/shell/instrument/CLAUDE.md @@ -132,7 +132,7 @@ the single authority.** Everything else that holds these values is a cache or a Every writer above except the last two writes the model directly, so for those "authority ends" is just "the write happened". Reload seed is not itself a model write — `reloadInstrument` never -touches `params_`; the only write in the tree is `setInstrumentParams`'s, `processor_state.cpp:192` +touches `params_`; the only write in the tree is `setInstrumentParams`'s, `processor_state.cpp:193` — which is why its row states no authority window of its own. The automation lane is the only one that cannot write directly: the SDK delivers it on the audio thread, where the model path allocates (`resolvePlay` copies velocity curves and spline contours). So it patches the diff --git a/src/shell/instrument/automation_channel.h b/src/shell/instrument/automation_channel.h index 5c2deff..b3d8764 100644 --- a/src/shell/instrument/automation_channel.h +++ b/src/shell/instrument/automation_channel.h @@ -2,47 +2,74 @@ // AUTHORITY LIFETIME is mechanised: a point outranks the model from the block it lands in until // the UI thread has folded it back into the model AND republished. This directory's CLAUDE.md // states the model; `core/instrument/param/param_merge` is the pure decision this feeds. -// The release itself is observed the NEXT BLOCK, not the instant it happens — refreshReleases() -// only runs inside process()'s merge branch (reasampler_processor.cpp), and publishLiveParams -// always bumps the generation that branch checks, so the next block is guaranteed to take it. +// The release itself is observed the NEXT BLOCK, not the instant it happens — see +// refreshReleases(). #pragma once #include +#include #include #include -#include #include "core/instrument/param/param_merge.h" namespace reasampler::vst { +class ReaSamplerProcessor; // the one class that legitimately mints a ReleaseProof + // The DeckParam ordinal space — what the automation slots and the notification diff both index. inline constexpr std::size_t kDeckParamSlots = instrument::param::kDeckParamSlots; class AutomationChannel { public: // Evidence release() below requires: the model has actually caught up with the - // point being released. The private constructor means the only way to obtain one - // is through a factory here, and both are named for the case they cover — + // point being released. The two factories are named for the case they cover — // `fromPublish` for an observed model republish, `noRepublishNeeded` for the two // cases drainAutomationToModel finds nothing to publish (the fold left the model - // unchanged, or no engine exists yet to read the live block). A caller that has - // not published cannot spell the argument release() needs, which is what turns - // reordering the release ahead of the publish into a compile error. + // unchanged, or no engine exists yet to read the live block). This is NOT a full + // compile-time proof of ordering: `noRepublishNeeded` still lets `ReaSamplerProcessor` + // claim the exemption with no publish at all. What it enforces structurally is that a + // caller cannot spell release()'s argument without naming, by which factory it called, + // WHICH exemption it is claiming — greppable by a reviewer, not silently inline — and + // both factories are restricted to the one class that legitimately needs either. + // The private, user-provided default constructor plus the deleted copy constructor + // close the two ways `{}` and copy-reuse would otherwise fabricate one for free; see + // the two definitions below for which C++17 rule each closes. class ReleaseProof { - public: + private: + friend class ReaSamplerProcessor; + static ReleaseProof fromPublish(std::uint32_t before, std::uint32_t after) { - // publish() is guaranteed to advance the generation; landing here means - // that guarantee broke, worth crashing on unconditionally rather than - // tolerating silently. - if (after == before) std::abort(); + // publish() advances the generation BY CONSTRUCTION (gen + 2, skipping 0 on wrap — + // live_params.h), so `after == before` is unreachable from this, its one call site, + // short of ~2^31 publishes wrapping exactly onto `before`. It also cannot see the + // actually-reachable failure mode, a caller falsely claiming noRepublishNeeded() — + // that path never runs this function. DEBUG-ONLY on purpose: an unconditional abort + // here would take down a musician's whole REAPER session, unsaved work on every other + // track and plugin included, for a condition this call site cannot produce; a debug + // build (where the test suite runs) is where a future regression in publish()'s + // advance guarantee should be caught. + assert(after != before && "publish() must advance the generation"); return ReleaseProof{}; } static ReleaseProof noRepublishNeeded() { return ReleaseProof{}; } - private: - ReleaseProof() = default; + // User-PROVIDED (a body, not `= default`): under C++17 a class with no data + // members and only a user-DECLARED (not user-provided) default constructor is + // still an aggregate, because C++17's aggregate rule excludes private data + // members only, not private constructors — so `ReleaseProof{}` would perform + // aggregate init and never call this. A user-provided constructor defeats that. + // (C++20's P1008 closes the same hole at the language level; this project is + // pinned to C++17 — CMakeLists.txt:28 — so the class must close it itself.) + ReleaseProof() {} + // Closes the other free-mint path: the implicit copy constructor is public by + // default, so one legitimately-minted proof cached in a member could be replayed + // by any later caller with no new publish behind it. A user-declared copy + // constructor (deleted or not) also suppresses the implicit move constructor, so + // no move-based replay path opens in its place — `release()` below takes this by + // const reference for exactly that reason, rather than needing one back. + ReleaseProof(const ReleaseProof&) = delete; }; // --- AUDIO THREAD ------------------------------------------------------------------- @@ -72,12 +99,16 @@ public: } // Refreshes each held slot's release answer. Runs only inside process()'s merge branch, so a - // release lands the NEXT BLOCK after the UI thread makes it, never the same instant — benign, - // because publishLiveParams always bumps the generation that branch checks, so the next block - // is guaranteed to run this. Must run BEFORE the model block is read: the acquire here - // synchronizes with the UI thread's release store, which it makes only AFTER republishing the - // model — so a slot seen released is one whose value any block read after this point is - // guaranteed to already carry. + // release lands the NEXT BLOCK after the UI thread makes it, never the same instant — benign + // on the `moved` path because publishLiveParams always bumps the generation that branch + // checks, so the next block is guaranteed to run this. The `noRepublishNeeded()` path has no + // publish and no generation bump to guarantee that — land() drops a static lane's repeats, so + // `moved` is false there too — but it is equally benign: the model already carries the value + // (that is why nothing published), so the release is simply observed whenever the generation + // next moves under ANY writer, not specifically this one. Must run BEFORE the model block is + // read: the acquire here synchronizes with the UI thread's release store, which it makes only + // AFTER republishing the model — so a slot seen released is one whose value any block read + // after this point is guaranteed to already carry. void refreshReleases() { for (std::size_t i = 0; i < kDeckParamSlots; ++i) { if (!slots_[i].held) continue; @@ -102,8 +133,10 @@ public: // Releases `slot`'s hold. The `ReleaseProof` argument is the enforcement: it can only be // constructed once the model carrying this point has been republished (or shown not to need - // it), so a caller earlier in that ordering has no value to pass. - void release(std::size_t slot, std::uint32_t seq, ReleaseProof) { + // it), so a caller earlier in that ordering has no value to pass. By const reference, not + // value: the copy constructor is deleted (see ReleaseProof), and the one caller releasing a + // whole fold's worth of slots passes the same proof through this repeatedly. + void release(std::size_t slot, std::uint32_t seq, const ReleaseProof&) { folded_[slot].store(seq, std::memory_order_release); } diff --git a/src/shell/instrument/instrument_params.cpp b/src/shell/instrument/instrument_params.cpp index 50ab140..1aa470f 100644 --- a/src/shell/instrument/instrument_params.cpp +++ b/src/shell/instrument/instrument_params.cpp @@ -295,8 +295,27 @@ void ReaSamplerProcessor::drainAutomationToModel() { // A present-but-static lane resends the SAME point every tick; writing it back would // republish liveParams_ and move paramsGeneration_ — a full model copy, a controller // cache write and an editor repaint, every tick, forever, for a value that never moved. - // The release below still fires: the model already carries the point either way. - if (value == modelParamNormalized(params, row.deck)) continue; + // Compared on the STORED side, never the normalized one: toNormalized(toPlain(n)) == n + // does NOT hold in general (param_taper.h — "no log map satisfies it in double"), so + // comparing `value` to modelParamNormalized(params, ...) skipped almost nothing for the + // 33 of 43 exposed controls whose taper is log/curved rather than identity — the churn + // this comment describes ran unabated for those. hostStoredFromNorm is the exact map + // writeDeckParamToModel below applies, so comparing its answer against the field it would + // land in asks "would this write change anything", not "are two norms equal". The + // release below still fires either way: the model already carries the point regardless + // of whether this write actually runs. + const double newStored = param::hostStoredFromNorm(row.deck, value); + bool unchanged = false; + if (row.deck == DeckParam::kKeyTrack) { + unchanged = newStored == params.keyTrack; + } else if (float* f = instrument::ui::deckFloatField(row.deck, params.play)) { + // The filter's four store a float: compared at that width, since that is what a + // re-write would actually round to, not the double newStored computes before it. + unchanged = static_cast(newStored) == *f; + } else if (double* d = instrument::ui::deckDoubleField(row.deck, params.play)) { + unchanged = newStored == *d; + } + if (unchanged) continue; writeDeckParamToModel(params, row.deck, value); moved = true; } @@ -312,9 +331,15 @@ void ReaSamplerProcessor::drainAutomationToModel() { // CLAUDE.md, THE AUTHORITY MODEL) — `releaseProof` is that ordering enforced structurally: // `automation_.release` below cannot compile without one, and the only ways to obtain one are // `publishLiveParams`'s return (the `moved` branch) or `noRepublishNeeded` (nothing to - // publish because the fold left the model already matching the point). - const AutomationChannel::ReleaseProof releaseProof = - moved ? publishLiveParams() : AutomationChannel::ReleaseProof::noRepublishNeeded(); + // publish because the fold left the model already matching the point). Built via an + // immediately-invoked lambda rather than `moved ? publishLiveParams() : noRepublishNeeded()` + // directly: ReleaseProof's copy constructor is deleted (automation_channel.h), and a ternary + // between two same-type prvalue arms needs it to merge them into one value — each `return` + // below is instead its OWN guaranteed-elided construction of the function's result object. + const AutomationChannel::ReleaseProof releaseProof = [&]() -> AutomationChannel::ReleaseProof { + if (moved) return publishLiveParams(); + return AutomationChannel::ReleaseProof::noRepublishNeeded(); + }(); syncParamsFromModel(); paramNotifySuppressed_ = wasSuppressed; for (std::size_t i = 0; i < foldedCount; ++i) {