Skip to content

Fix unsound Ok() switch guard by demoting all-residual conditional discriminants#271

Open
AaronWebster wants to merge 2 commits into
masterfrom
emboss/ok-guard-demote
Open

Fix unsound Ok() switch guard by demoting all-residual conditional discriminants#271
AaronWebster wants to merge 2 commits into
masterfrom
emboss/ok-guard-demote

Conversation

@AaronWebster

@AaronWebster AaronWebster commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

The optimized Ok() switch (introduced in #253, extended in #256/#257) reads its
discriminant once and emits, as a safety net:

const auto emboss_reserved_switch_discrim = <discriminant>;
if (!emboss_reserved_switch_discrim.Known()) return false;
switch (emboss_reserved_switch_discrim.ValueOrDefault()) { ... }

When the discriminant is itself a conditionally-present field read from a buffer too
short to contain it, it reads as Unknown, so this guard makes Ok() return false
even though the message is valid, because the fields gated on the (absent) discriminant
simply don't exist. This is latent on master: no existing test schema produces the
shape.

Same bug that #266 fixes, reached by a simpler route.

Fix

Instead of #266's dedicated ok_method_switch_block_guarded template, add one rule to
the demotion pass in _generate_optimized_ok_method_body: a switch group that is not
provably-known
and has no bare arm (every entry carries a residual) is demoted to
per-field has_X() checks — the existing ok_method_test path — instead of being
emitted as a guarded switch. After this, _emit_switch_block only ever sees
provably-known or bare-arm-guarded forms, so the unsound guarded-switch template is
deleted outright.

Soundness is observationally identical to #266's guarded fallback: with the discriminant
Unknown, a statically-false residual makes has_X() Known-false (field absent → valid,
the exact case #266 fixed), otherwise has_X() is UnknownOk() returns false; with
the discriminant Known it computes the full has_X(), same as a dispatched arm minus
the pointless switch.

Verification

  • Proven byte-identical generated code on every existing schema; the golden diff is
    confined to testdata/golden_cpp/condition.emb.h.
  • Keeps Guard conditional switch discriminants in optimized Ok() #266's regression tests (testdata/condition.emb +
    compiler/back_end/cpp/testcode/condition_test.cc); assertions unchanged, comments
    reworded per review.
  • Full bazel test //... green (206/206).
  • Size: ~0 delta on all targets (MicroBlaze LE / Vitis authoritative) — this is a
    correctness fix, not a size optimization.

Supersedes

Supersedes #266 (same bug, simpler fix; #266's regression tests are carried over here).

…scriminants

The optimized Ok() switch reads its discriminant once and guards it with
`if (!discrim.Known()) return false;`. That guard is unsound when the
discriminant is itself a conditionally-present field AND every arm carries
a residual: a valid message can have an out-of-bounds (Unknown) discriminant
while every arm's residual is statically false, so every has_X() is
Known-false (absent) and the message is valid -- but the blanket guard
rejects it.

Fix it in the demotion pass rather than with a bespoke guarded-switch
template: an all-residual, not-provably-present switch group is demoted to
plain per-field has_X() checks, which handle an Unknown discriminant
correctly (an Unknown discriminant can never make has_X() Known-true, so a
Known-false residual simply leaves the field absent). Groups with at least
one bare arm keep the sound Known()-guarded switch.

This is latent on the current corpus -- no existing schema produces the
shape -- so every existing golden is byte-for-byte identical; only the new
condition.emb regression structs add codegen. Supersedes the guarded-switch
approach in the earlier fix-conditional-switch-guard branch with a simpler
one (no new template, no fallback path).

Adds regression structs to condition.emb (ResidualConditionalDiscriminant,
BareConditionalDiscriminant, DominatedBareDiscriminant,
DisjunctionConditionalDiscriminant, SingleEntryConditionalDiscriminant,
EnumConditionalDiscriminant) with tests in condition_test.cc covering the
discriminant absent / present-but-truncated / gated-field / no-arm-match
cases for both the demoted and the still-guarded shapes.
Comment thread testdata/condition.emb Outdated
Comment thread testdata/condition.emb Outdated
Comment thread testdata/condition.emb Outdated
# IsComplete() is true. Ok() must still be false: a/b's existence is
# indeterminate, and the discriminant Known() guard -- not IsComplete() --
# is what enforces that. (A `if (has_tag().ValueOrDefault())` wrapper would
# wrongly accept this message.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last phrase is unclear
"(A if (has_tag().ValueOrDefault()) wrapper would wrongly accept this message.)"

Does that reflect the state before this PR or after? Is that intended behaviour? Some of the explanations in the comments for these tests might be helpful in the language docs (either as clarifications or examples).

Comment thread testdata/condition.emb Outdated
Comment thread compiler/back_end/cpp/testcode/condition_test.cc Outdated
Comment thread compiler/back_end/cpp/header_generator.py Outdated
Comment thread compiler/back_end/cpp/header_generator.py Outdated
…() with conditional fields

- Remove "optimized"/"guarded switch"/demotion codegen internals and the
  confusing hypothetical-wrapper aside from testdata/condition.emb struct
  comments; describe schema shape and required observable behavior instead.
- Drop bare #NNN PR references from condition_test.cc comments.
- Make the demotion-pass comment in header_generator.py timeless (the
  "latent on the current test corpus" claim is untrue once condition.emb
  exercises the shape) and use _ for the unused loop variable.
- Document Ok() semantics for conditional fields in doc/cpp-guide.md:
  absent field != invalid, indeterminate existence -> Ok() false even
  when IsComplete() is true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants