feat(agent_expr): if_goto + goto control-flow opcodes (#25 phase-3)#18
Merged
Conversation
fab5b0a to
75938a9
Compare
Fills the docs/28 0x90-0x9f control-flow range that was reserved in phase-1 (the bytecode VM + evaluator), so #26 phase-2's in-target tracepoint VM can compile predicates with branches instead of filtering on the daemon side. Opcode bytes: 0x90 for if_goto, 0x91 for goto. gdb's spec puts these at 0x20 / 0x21 but 0x20 already maps to LDB's kReg — matching gdb's bytes would either break every phase-1/phase-2 bytecode payload (reassign kReg) or accept a one-byte translation in #26's eventual gdb-remote wire driver. Picked the latter (translation lives in the wire driver, not the VM contract). Compiler grows (if cond then else) and (when cond body) special forms in src/agent_expr/compiler.cpp. Layout: <cond> kLogNot kIfGoto Telse <then> kGoto Tend Telse: <else> Tend:. The "if-not-cond, jump to else" shape lets the single-pass emitter keep forward-branch back-patching local — the alternative gdb layout would parse 'then' second but emit it last, fighting the patch-as-we-go invariant. Extra kLogNot is one byte + one cycle; tracepoints are #26's hot path, not daemon-side compilation. New emit_jump + patch_u16_be helpers on State handle the forward- jump placeholder + back-patch dance. Out-of-range target pc (jump past code.size()) surfaces kBadImmediate; infinite backward loops still trip the existing kMaxInsnCount cap. 18 new test cases (9 evaluator + 9 compiler) cover taken / not-taken branches, backward jumps, short-circuit semantics (unchosen branch's (div 1 0) never executes), nested (if (if ...) ...), arity errors, codegen-shape assertion for (if 1 42 99), out-of-range target detection, infinite-loop cap. docs/28 §2 lists the new opcodes; docs/28 §4 failure-modes table gets the kBadImmediate-on-out-of-range rows; docs/28 §6 phase-3 section moves from "future" to "landed". docs/29 §1 DSL grammar documents (if ...) and (when ...) with worked examples. ctest: 70/70 green. Agent-expression suite: 61 test cases (was 43), 257 assertions, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view) The kIfGoto handler validated `target > prog.code.size()` only inside the `if (cond != 0)` branch. A malformed predicate with a broken jump target therefore passed silently whenever the popped cond happened to be zero — and crashed (kBadImmediate) on the next call with a truthy cond. Classic latent-validation bug: deferring a check until the data is "needed" lets bad bytecode slip past first contact. Fix: validate the target immediately after the immediate read, before popping cond. Matches kGoto's existing unconditional check. Added an invariant comment on both handlers so the next opcode author copying this pattern doesn't reintroduce the asymmetry. Regression test asserts kBadImmediate when the cond is zero AND the target is out of range. Pre-fix that program returned kOk and silently executed the post-branch tail; post-fix it correctly surfaces the bad target. agent_expr unit tests: 62 cases / 258 assertions (was 61 / 257). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two reviewer findings on the phase-3 test surface: 1. Misleading comment block above the (if/when) compiler suite described gdb's "branch to then, else falls through" layout. The compiler actually emits the opposite — "if-not-cond, jump to else" — by inverting the predicate with kLogNot. The byte-shape test at the bottom verifies the real layout correctly; only the comment was wrong. Fixed the comment to match what's emitted, with a pointer to the byte-shape test as the source of truth. 2. The "infinite loop via backward kGoto → kInsnLimitExceeded" test only asserted that the cap fired — it would have passed even on an off-by-one where the cap tripped on iteration 1. Strengthened by exposing insn_count via EvalResult and asserting it equals kMaxInsnCount exactly. Switched the test program from a single kGoto self-loop to a 3-op loop body (const + drop + goto) so the loop genuinely runs ~3333 cycles before tripping the cap. Added a companion test that asserts insn_count on a normal (non-cap) run so a regression in the counter surfaces locally, not only via the loop test. agent_expr unit tests: 63 cases / 262 assertions (was 62 / 258). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
75938a9 to
d33de33
Compare
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
1 participant
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.
Summary
Fills the
docs/280x90-0x9fcontrol-flow range reserved inphase-1 so #26 phase-2's in-target tracepoint VM can compile
predicates with branches instead of filtering daemon-side.
Op::kIfGoto = 0x90+Op::kGoto = 0x91inbytecode.h.gdb's spec puts these at
0x20/0x21, but0x20is alreadyLDB's
kReg. Picked LDB's reserved range; the one-bytetranslation moves to #26's eventual gdb-remote wire driver
(kept out of the VM contract).
code.size()surfaceskBadImmediate; backward-jump infiniteloops still trip the existing
kMaxInsnCountcap.(if cond then else)and(when cond body)special forms. Emission layout is "if-not-cond, jump to else"
(extra
kLogNotbyte) so single-pass codegen emits in sourceorder — alternative gdb layout would need a side-buffer for the
thenbranch.not-taken / backward / out-of-range / underflow / infinite-loop;
short-circuit semantics (unchosen branch's
(div 1 0)neverruns), nested
(if (if ...) ...), arity errors, codegen-shape.docs/28anddocs/29updated; phase-3 sections movefrom "future" to "landed".
Test plan
cmake --build build -j 8— warning-cleanctest --test-dir build --output-on-failure— 70/70 greenldb_unit_tests '[agent_expr]'— 61 test cases / 257assertions (was 43 / 213); +18 test cases delta
ldb_unit_tests '[ctrl]'— 9 evaluator tests passldb_unit_tests '[if]' '[when]'— 9 compiler tests passRequired by #26 phase-2 (in-target tracepoint VM) for efficient
predicate short-circuiting.
🤖 Generated with Claude Code