Guard conditional switch discriminants in optimized Ok()#266
Open
AaronWebster wants to merge 3 commits into
Open
Guard conditional switch discriminants in optimized Ok()#266AaronWebster wants to merge 3 commits into
AaronWebster wants to merge 3 commits into
Conversation
The optimized Ok() switch reads its discriminant once and emits `if (!emboss_reserved_switch_discrim.Known()) return false;` as a safety net. When the discriminant is a conditionally-present field read from a buffer too short to contain it, the discriminant is Unknown and that guard makes Ok() reject a message that is actually valid: the fields gated on the (absent) discriminant simply do not exist. The guard is only sound when the group has a "bare" arm whose existence condition is exactly `discrim == K`. Then an Unknown discriminant leaves that field's has_X() Unknown, so the unoptimized Ok() bails for the same reason. When every arm carries a residual, an out-of-bounds discriminant can coexist with a valid message whose residuals are all statically false (so every has_X() is Known-false / absent). For that case emit a guarded switch: dispatch only when the discriminant is in bounds, otherwise fall back to per-field has_X().Known() checks (an Unknown discriminant can never make has_X() Known-true, so the value check is unnecessary). Provably-present and bare-armed switches are unchanged, so every existing golden is byte-for-byte identical. Adds ResidualConditionalDiscriminant (the bug) and BareConditionalDiscriminant (the sound-guard case) to condition.emb with regression tests that pass under both the optimized and unoptimized Ok() implementations.
Adds regression structs that distinguish the guard fix from a
presence-wrapper alternative and exercise the optimization chain:
* DominatedBareDiscriminant: a bare arm on a conditional discriminant
whose field is size-dominated by an always-present field, so
IsComplete() stays Known. Only the discriminant Known() guard
rejects the existence-indeterminate message; a
has_<d>().ValueOrDefault() wrapper would wrongly accept it.
* DisjunctionConditionalDiscriminant: disjunction arms with residuals
-> guarded switch with coalesced case labels.
* SingleEntryConditionalDiscriminant: single-entry group demotes to a
has_X() check, so no discriminant guard is emitted.
* EnumConditionalDiscriminant: enum-typed conditional discriminant.
Only condition.emb.h regenerates; all other goldens are unchanged.
Tagged-union alternatives are mutually exclusive, so placing `a` and `b` at the same offset is a more realistic layout (and avoids miscopying when these structs are reused as templates for new tests). Only the physical field layout changes -- the switch structure and Ok() behavior are identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The optimized
Ok()switch (introduced in #253, extended in #256/#257) reads itsdiscriminant once and emits, as a safety net:
When the discriminant is itself a conditionally-present field and is read from a
buffer too short to contain it, the discriminant reads as
Unknown, so this guardmakes
Ok()returnfalse— even though the message is valid, because the fieldsgated on the (absent) discriminant don't exist.
This is latent on
master: no existing test schema produces the shape, and the onlysurviving guard in the goldens (
parameters.emb'sAxis) is on a bare arm, which issound.
Root cause
The guard is sound iff the group has at least one bare arm (existence condition
exactly
discriminant == K): then anUnknowndiscriminant leaves that field'shas_X()Unknown, so the unoptimizedOk()bails for the same reason. When everyarm carries a residual (extra conjuncts beyond
discriminant == K), a valid message canhave an out-of-bounds (
Unknown) discriminant while every arm's residual is staticallyfalse — making every
has_X()Known-false (absent). The blanket guard wrongly rejects it.Fix
In
_emit_switch_block, classify each group:Known()guard (unchanged — sound).the discriminant is in bounds, otherwise fall back to per-field
has_X().Known()checks.(An
Unknowndiscriminant can never makehas_X()Known-true, so the value check isunnecessary in the fallback.)
Cases 1 & 2 are untouched, so every existing golden is byte-for-byte identical; only the
new structs add codegen.
Tests
Adds two structs to
testdata/condition.emb:ResidualConditionalDiscriminant— the bug (all-residual arms on a conditional discriminant).BareConditionalDiscriminant— the sound-guard case (bare arms).with regression tests in
condition_test.cccovering: discriminant absent (valid, waswrongly rejected), discriminant present-but-truncated (invalid), gated field present/truncated,
and discriminant matching no arm. The tests pass under both the optimized and unoptimized
(
_no_opts)Ok()implementations, proving the optimized path now matches ground truth.The same fix + tests will be propagated to the open stacked PRs (#258, #259) so the bug cannot
be reintroduced as they land.