dsp::LoopMode has no count sentinel, so five loaders bound it by its last named value #85

New issue

Closed

opened 2026-09-09 22:28:23 +00:00 by bcox · 1 comment

bcox commented

2026-09-09 22:28:23 +00:00

Owner

plugin/patch_format.h reads a dsp::LoopMode byte at five sites — one per envelope-bearing engine — and every one of them bounds it as <= kAlternating rather than against a count sentinel, because dsp/loop_mode.h does not have one.

Found while sweeping that file during ladder-gate. Six other bounds in the same file had the identical shape and were fixable in place, because their enums already carried sentinels: gate_mode and lpg_mode moved from <= kAmplitudeOnly to < kModeCount, and three VactrolGating sites from <= kGate to < kGatingCount. ReadStrip was already reading the sentinels, so that change aligned two readers with a third rather than inventing a convention.

LoopMode cannot be fixed the same way. Closing it means adding kModeCount to dsp/loop_mode.h and reading it at all five sites — a change to another subsystem, which is why ladder-gate reported it rather than widening into it.

Why it matters: appending a fourth loop mode needs five coordinated edits, and any site that is missed silently rejects the new value and falls back. -Werror=switch cannot see a comparison, so nothing catches it at build time — the format simply refuses a mode the engine supports.

Four other enums are read by their last named value for the same reason (no sentinel to read): dsp::VaLadderMode (two sites), dsp::VaShaperInsert, dsp::WaveguideBore, and PartialLayerType::PhaseMode (one each). LoopMode is the one worth acting on because it is the only one with five sites.

For the record, the sweep also established that the "loader ignores a sentinel its enum already has" defect is now gone from plugin/patch_format.h entirely: twelve sites read a sentinel or a single-source predicate, and three two-valued enums are read as equality with the other value as fallback, which is exhaustive by construction.

`plugin/patch_format.h` reads a `dsp::LoopMode` byte at five sites — one per envelope-bearing engine — and every one of them bounds it as `<= kAlternating` rather than against a count sentinel, because `dsp/loop_mode.h` does not have one. Found while sweeping that file during `ladder-gate`. Six *other* bounds in the same file had the identical shape and were fixable in place, because their enums already carried sentinels: `gate_mode` and `lpg_mode` moved from `<= kAmplitudeOnly` to `< kModeCount`, and three `VactrolGating` sites from `<= kGate` to `< kGatingCount`. `ReadStrip` was already reading the sentinels, so that change aligned two readers with a third rather than inventing a convention. `LoopMode` cannot be fixed the same way. Closing it means adding `kModeCount` to `dsp/loop_mode.h` and reading it at all five sites — a change to another subsystem, which is why `ladder-gate` reported it rather than widening into it. **Why it matters:** appending a fourth loop mode needs five coordinated edits, and any site that is missed silently rejects the new value and falls back. `-Werror=switch` cannot see a comparison, so nothing catches it at build time — the format simply refuses a mode the engine supports. Four other enums are read by their last named value for the same reason (no sentinel to read): `dsp::VaLadderMode` (two sites), `dsp::VaShaperInsert`, `dsp::WaveguideBore`, and `PartialLayerType::PhaseMode` (one each). `LoopMode` is the one worth acting on because it is the only one with five sites. For the record, the sweep also established that the "loader ignores a sentinel its enum already has" defect is now gone from `plugin/patch_format.h` entirely: twelve sites read a sentinel or a single-source predicate, and three two-valued enums are read as equality with the other value as fallback, which is exhaustive by construction.

bcox referenced this issue

2026-09-09 23:56:18 +00:00

//patches:audition reports success after writing nothing #69

bcox commented

2026-09-10 01:33:46 +00:00

Author

Owner

Closed by e2c1350 on main.

dsp::LoopMode gains kModeCount, and all five readers now bound the byte with < kModeCount:

reader	what it decodes
`ReadFmPairLayer`	FM pair clock 0
`ReadTimelineRecord`	FM pair clocks 1..N
`ReadVaLayer`	all five VA segment envelopes
`ReadUnvoicedLayer`	unvoiced clock 0
`ReadPartialLayer`	every partial-bank clock

The name is the one four sites in the same file already read -- dsp::LowPassGateMode::kModeCount -- so this finishes the sweep's convention rather than starting one.

Verified by injection

A fourth enumerator appended to dsp::LoopMode, which is the exact change the issue is about, failed the new test at all five readers, once each, and only at ordinal 3. Ordinals 0-2 passed throughout, so the failure was the bound and not the fixture:

ReadFmPairLayer            1
ReadTimelineRecord         1
ReadUnvoicedLayer          1
ReadPartialLayer           1
ReadVaLayer, envelope 0-4  1 each
  loop_mode: Which is: 1-byte object <00>   <- kNone
  mode:      Which is: 1-byte object <03>

With the five bounds moved to kModeCount and the injected mode still in place, the test passes -- which is what says the fix reaches all five rather than four.

The regression instrument

PatchFormat.EveryReaderAcceptsEveryLoopModeTheEnumNames builds one instrument carrying an FM pair with a routed secondary timeline, a VA layer, an unvoiced layer and a partial bank, and round-trips it once per ordinal below kModeCount. It loops to the sentinel rather than over a written-out list of today's three modes, so a fourth extends its reach on its own -- a list would have made the test a sixth site to remember and no safer than the five it guards.

It sits beside AnUnknownLoopModeOrdinalDecodesToNoneAndTheLayerStillEnds, which pins the other half of the same bound: that an ordinal the enum does not name still falls back to kNone.

One thing beyond what was filed

The editor had the same defect in a form the issue did not cover: two loop combos -- the VA envelope's row in editor/va_envelope_view.h and DrawTimelineControlsFor's for the FM pair in editor/timeline_view.h -- each passed a literal 3 beside a three-entry label array. A fourth mode would have loaded and saved correctly and been unreachable in the picker. Both now take the count from IM_ARRAYSIZE under a static_assert against kModeCount, matching what editor/patch_view.h already does for dsp::ShaperShape and dsp::VaFilterTopology. Both static_asserts fired under the injection above.

Two comments that restated how many modes there are lost the count, since kModeCount is itself an enumerator and "LoopMode has exactly three enumerators" had become false as written.

Still open from this issue's tail

The four other enums read by their last named value are untouched: dsp::VaLadderMode (two sites), dsp::VaShaperInsert, dsp::WaveguideBore, and PartialLayerType::PhaseMode. This change did not widen into them, for the reason ladder-gate did not widen into LoopMode.

Lanes

101/101 at fastbuild, ASan and UBSan; //plugin:preset_race_test passes under TSan. No --config=opt run: nothing here touches the renderer or any DSP arithmetic, so no golden is at stake.

Closed by `e2c1350` on `main`. `dsp::LoopMode` gains `kModeCount`, and all five readers now bound the byte with `< kModeCount`: | reader | what it decodes | | --- | --- | | `ReadFmPairLayer` | FM pair clock 0 | | `ReadTimelineRecord` | FM pair clocks 1..N | | `ReadVaLayer` | all five VA segment envelopes | | `ReadUnvoicedLayer` | unvoiced clock 0 | | `ReadPartialLayer` | every partial-bank clock | The name is the one four sites in the same file already read -- `dsp::LowPassGateMode::kModeCount` -- so this finishes the sweep's convention rather than starting one. ### Verified by injection A fourth enumerator appended to `dsp::LoopMode`, which is the exact change the issue is about, failed the new test at **all five readers, once each, and only at ordinal 3**. Ordinals 0-2 passed throughout, so the failure was the bound and not the fixture: ``` ReadFmPairLayer 1 ReadTimelineRecord 1 ReadUnvoicedLayer 1 ReadPartialLayer 1 ReadVaLayer, envelope 0-4 1 each loop_mode: Which is: 1-byte object <00> <- kNone mode: Which is: 1-byte object <03> ``` With the five bounds moved to `kModeCount` and the injected mode still in place, the test passes -- which is what says the fix reaches all five rather than four. ### The regression instrument `PatchFormat.EveryReaderAcceptsEveryLoopModeTheEnumNames` builds one instrument carrying an FM pair with a routed secondary timeline, a VA layer, an unvoiced layer and a partial bank, and round-trips it once per ordinal below `kModeCount`. It loops to the sentinel rather than over a written-out list of today's three modes, so a fourth extends its reach on its own -- a list would have made the test a sixth site to remember and no safer than the five it guards. It sits beside `AnUnknownLoopModeOrdinalDecodesToNoneAndTheLayerStillEnds`, which pins the other half of the same bound: that an ordinal the enum does *not* name still falls back to `kNone`. ### One thing beyond what was filed The editor had the same defect in a form the issue did not cover: two loop combos -- the VA envelope's row in `editor/va_envelope_view.h` and `DrawTimelineControlsFor`'s for the FM pair in `editor/timeline_view.h` -- each passed a literal `3` beside a three-entry label array. A fourth mode would have loaded and saved correctly and been unreachable in the picker. Both now take the count from `IM_ARRAYSIZE` under a `static_assert` against `kModeCount`, matching what `editor/patch_view.h` already does for `dsp::ShaperShape` and `dsp::VaFilterTopology`. Both static_asserts fired under the injection above. Two comments that restated how many modes there are lost the count, since `kModeCount` is itself an enumerator and "LoopMode has exactly three enumerators" had become false as written. ### Still open from this issue's tail The four other enums read by their last named value are untouched: `dsp::VaLadderMode` (two sites), `dsp::VaShaperInsert`, `dsp::WaveguideBore`, and `PartialLayerType::PhaseMode`. This change did not widen into them, for the reason `ladder-gate` did not widen into `LoopMode`. ### Lanes 101/101 at fastbuild, ASan and UBSan; `//plugin:preset_race_test` passes under TSan. No `--config=opt` run: nothing here touches the renderer or any DSP arithmetic, so no golden is at stake.

bcox referenced this issue from a commit

2026-09-10 01:35:50 +00:00

A loop mode is bounded by its enum's count, not by the last one named

bcox closed this issue

2026-09-10 01:35:50 +00:00

bcox referenced this issue from a commit

2026-09-10 05:18:09 +00:00

The last four enums read by their last value learn to count

bcox referenced this issue

2026-09-10 18:52:17 +00:00

Ladder gate execution ledger: 10 rulings, a bug the spec called a characteristic, and five classes of a self-contradicting comment #84