From 542bc6a65cb5aecd8ef236453e18162f03b64ff1 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 13 Sep 2026 08:02:22 +0700 Subject: [PATCH] A bad index stops the program where it stands instead of taking the session with it --- BUILT.md | 83 +++++++++++- NEXT.md | 32 ++++- PORTING.md | 29 ++++- lib/emit.ml | 83 +++++++++--- lib/prelude.ml | 34 +++++ runtime/flan_rt.c | 99 ++++++++++++++- test/programs/bounds-condition.flan | 190 ++++++++++++++++++++++++++++ test/test_acceptance.ml | 38 +++++- 8 files changed, 551 insertions(+), 37 deletions(-) create mode 100644 test/programs/bounds-condition.flan diff --git a/BUILT.md b/BUILT.md index 36772af..2600b4a 100644 --- a/BUILT.md +++ b/BUILT.md @@ -18,7 +18,9 @@ assignable, which makes the generated step its only writer. level of a function body. Each one is checked in place, then registered on the context; it emits nothing where it stands. Function exit runs them innermost-first, and an explicit `return` runs the ones registered *above* it — a defer written below a return has not executed yet and must not fire. A trap runs none of them, which follows from the -bounds-check shape (`noreturn` then `unreachable`) rather than being a separate decision. +bounds-check shape (`noreturn` then `unreachable`) rather than being a separate decision. **Amended** once a bounds +failure became a signal: an *answered* one leaves through the unwind path and runs them like any other transfer, an +unanswered one still runs none. See "An index out of range is a condition" at the foot of this file. `defer` inside a `let`, a loop or a branch is **rejected**, not accepted with function scope. It would run once at function exit rather than once per iteration, and that is the silent-wrongness class the rule below is about. @@ -3933,3 +3935,82 @@ assertable. `raylib-image.flan` carves one 6×3 sheet twice at two different `y` `y`, `width` and `height` are each pinned by an answer that moves if they do, and the sheet surviving both carves is what distinguishes this from `image-crop`. Bind it to `ImageCrop` by mistake and the second carve reads out of a 2×1 image and the case goes red. + +## An index out of range is a condition + +`flan_bounds_fail` printed the source location, the index and the length, and called `exit(134)`. That was defensible +while `flan dev` was two processes. It is not now: the compiler runs **inside the program**, so the trap took the +whole session with it — and the session not having to restart is the project's thesis. `PORTING.md` found the customer +and found it on the most ordinary path there is: a grid indexed straight from a mouse position is out of bounds the +first time the pointer leaves the window, and the author's Common Lisp port had to add an `in-bounds-p` to survive it. + +A failed bounds check now signals **`BoundsError`** with `error`, the same way a failed allocation signals +`StorageExhausted`. `runtime/flan_rt.c` has `flan_bounds_error` and `flan_slice_error`; each walks the handlers, then +offers the break loop, and only if neither transferred does it tail into the `flan_bounds_fail`/`flan_slice_fail` that +were there before — same message, same status 134. Nothing was removed; a die was demoted to a last resort. + +`(defstruct BoundsError [low i64 high i64 length i64])` is in the prelude. Fixed numeric fields and no rendered +message, for `StorageExhausted`'s reason: formatting allocates, and a condition raised on a path that may be out of +storage must not. `low` and `high` are the same index for an `(at xs i)` and the two ends of the range for a `(slice +xs lo hi)`, so **one** condition type covers both and a handler that wants to survive a bad index writes one clause +rather than two. The three `int64_t`s in `flan_rt.c` have to agree with that `defstruct` field for field — the same +hand-kept agreement `flan_name_id` already keeps with `Check.type_id`, and for the same reason: a struct is a layout, +a type is a number, and neither side can see the other. + +### Why no restart is established at the failing index + +This is the decision, and it is a decision rather than an omission. + +`alloc_guard` offers `retry` and `file_guard` offers `retry`/`use-value` **because their attempt is repeatable**. A +handler frees something and the allocation succeeds; a handler supplies another path and the open succeeds. That is +what makes them `plan.org`'s named exceptions to "restarts go at the resync point, once" — a restart at an outer loop +cannot re-attempt an allocation, and only the allocation site can. + +Nothing a handler can do makes index 51 valid for a length-50 array. There is no attempt to re-run, so there is +nothing for a site restart to resume into, and bounds falls on the **default** side of that rule. + +- **`continue` is wrong.** `(at xs i)` has to produce a value of the element type and there is none to produce. It + would mean something in the `(set (at xs i) v)` position and nothing in the other, and `at` is one form. +- **`use-value` for the index is the near miss and is still wrong.** It costs the hot path, not just the cold block: + `idx` is an SSA value feeding the gep, and retrying needs it in an alloca reloaded per attempt, plus a restart frame + pushed and popped on **every** indexing operation. What it buys is a *different element*, silently — the class of + answer this codebase refuses everywhere else. +- **What actually answers a bad index is already on the stack.** A frame loop's `continue` — `sand.flan`'s shape — is + an ordinary `restart-case`, `flan_find_restart` walks to it, and the break loop lists it. Signalling is the whole + fix. A site restart would add nothing the frame loop does not already offer, at a cost on every index in the + program. + +### Release, and what a build without the agent does + +`flan_break_hook` is NULL unless the agent package was imported, so a release build — or any program that did not +import it — signals, finds no handler, finds no hook, and dies with the message and the status it always had. That is +still right: there is nowhere to stand. The change is not "bounds failures stopped being fatal"; it is "a bounds +failure is now answerable, and is fatal when unanswered". + +### Defer, which had to be answered rather than inherited + +The note at the top of this file said *a trap runs no defers, which follows from the bounds-check shape (`noreturn` +then `unreachable`) rather than being a separate decision*. The shape changed, so the consequence could not be +inherited. It now splits: + +- **An answered bounds failure runs the defers.** `guard` routes through `current_pad`, which with no enclosing + `restart-case` sets `f.unwound` and branches to the function's unwind block — the same path `return` uses, which is + where §5's defers already live. So a transfer out of a bad index is an ordinary transfer and runs cleanup + innermost-first, like every other one. +- **An unanswered one still runs none**, because it is still a `rt_die()` inside C with no Flan frame involved. + +`bounds-condition.flan` asserts the first of those directly: five `defer`s across five abandoned and finished frames, +counted. + +### Vec and Map + +`(at v i)` and `(at arr i)` are the same form in the source, so shipping one signalling and the other exiting would +have read as a bug. A `Vec`'s bounds check lives *inside* `flan_vec_at` and `flan_vec_as_slice` rather than in emitted +IR (which is also why `--no-bounds-checks` never reached it), so both grew a trailing transfer-channel parameter and +`Emit`'s `Rt` arm guards those two symbols and no others — they are the only ones in that family that can transfer; +everything else there is arithmetic over a container header. + +**A `Map`'s bounds and a `Vec`'s stale-allocator check still die.** `flan_vec_stale_fail` is a different kind of +failure — the region a container lived in was released, and there is no frame to go back to that would not read freed +memory — and the map path was left alone rather than converted half-way. Written down here so it is a known edge +rather than a discovery. diff --git a/NEXT.md b/NEXT.md index bb777bc..6dd224c 100644 --- a/NEXT.md +++ b/NEXT.md @@ -11,11 +11,33 @@ surface and must not be load-bearing, because the default build has no `FLAN_RAY **2. `Key` has no `left-shift` — DONE.** `left-shift 340`, and nothing else: `PORTING.md` §5 checked every other enum value the game touches and they were all already right. -**3. An out-of-bounds index should signal a condition, not `exit(134)`.** `PORTING.md`'s own first recommendation -after the binding lines. The game indexes grids straight from mouse coordinates — `game.lisp` had to add an -`in-bounds-p` to survive it — and today a trap ends the process *and* the `flan dev` session with it. `flan_rt.c`'s -`flan_bounds_fail` should go through the condition machinery, which would make it a break loop with a restart instead -of a dead session. Note this interacts with the merged one-process build: killing the program now kills the compiler. +**3. An out-of-bounds index should signal a condition, not `exit(134)` — DONE.** A failed bounds check signals +`BoundsError` with `error`; `runtime/flan_rt.c`'s `flan_bounds_error`/`flan_slice_error` walk the handlers, then offer +the break loop, and tail into the old `flan_bounds_fail` message and status only if nothing answered. `Vec`'s two +checks are plumbed the same way, since `(at v i)` and `(at arr i)` are one form in the source. + +**No restart is established at the failing index**, and `BUILT.md` has the argument: `retry` exists for allocation and +for files because those attempts are *repeatable*, and nothing a handler can do makes index 51 valid for a length-50 +array. `use-value` for the index would cost every indexing operation a restart frame and buy a silently different +element. What answers a bad index is the restart the program already had — a frame loop's `continue`, `sand.flan`'s +shape — which is on the restart stack and on the break loop's list without anything being pushed at the site. + +**Defer** had to be answered rather than inherited, since the `noreturn`-then-`unreachable` shape is what the old note +followed from: an answered bounds failure leaves through the function's unwind block, which is `return`'s path, so it +runs the defers; an unanswered one still runs none. `test/programs/bounds-condition.flan` counts them. + +**Still dying, deliberately:** a `Map`'s bounds check and `flan_vec_stale_fail`. The stale-allocator case is a +different kind of failure — the region the container lived in was released — and there is no frame to go back to that +would not read freed memory. The map path was left alone rather than converted half-way. + +### What is left on `PORTING.md`'s list + +Tier 0 is finished. Of Tier 1, items 5 and 6 are the next two and they are both dev-loop work rather than language +work: **a watch for a *running* program** (the stopped-stack inspector is a different tool for a different moment — +`watch.clj` + `spy-num` is the shape, and the numeric accumulator for hot loops is the part that is least obvious), +and **frame rollback as a worked example** — `snapshot`/`restore` callbacks beside the `continue` restart, which is +now genuinely reachable from a bad index and so is worth more than it was yesterday. `bounds-condition.flan` shows an +abandoned frame leaving half-written state behind; rollback is what finishes that thought. **What `PORTING.md` says NOT to build, with evidence:** escaping closures (one capture site, fixed by one parameter), `Handle`/pools, `Result`/`try`, `handler-case`, `loop`/`recur` and tail calls, user allocators, structural typing — diff --git a/PORTING.md b/PORTING.md index 3bb23c0..954651b 100644 --- a/PORTING.md +++ b/PORTING.md @@ -312,6 +312,14 @@ another name. Two things are missing behind it: `lib/dev.ml` answers every subsequent request with "the program exited; restart flan dev". + > **Fixed, 2026-09-13.** It signals `BoundsError` with `error` now, and dies with that + > same message only if nothing answered. The site establishes **no restart** — nothing + > a handler can do makes a bad index good, so there is no attempt to re-run — and what + > answers it is the restart the program already had, which is exactly the frame loop's + > `continue` this section is about. `BUILT.md` has the reasoning and + > `test/programs/bounds-condition.flan` has the worked case. Item 1 below, the + > rollback, is now the whole of what is left here. + That is not theoretical for this game. `game.clj`'s `update-game` computes `row` and `col` straight from the mouse position and indexes the grid with them, with no bounds check anywhere. **`game.lisp` added `in-bounds-p` and calls it in `update-drag`** — the @@ -487,13 +495,20 @@ not compete for the same slot. ### Tier 1 — language and tooling, in the order that unblocks the most of this game -4. **A bounds failure should stop the program, not end it.** Route `flan_bounds_fail` - through the condition machinery so it reaches the break loop with the restarts that are - on the stack, instead of `exit(134)`. This is the single largest gap between what Flan - promises and what this game would experience, and the game reaches it through the most - ordinary path in it — a mouse coordinate outside the window, which `game.lisp` had to - add `in-bounds-p` to survive. Everything else here is a workaround with a known cost; - this one ends the session. +4. ~~**A bounds failure should stop the program, not end it.**~~ **Done, 2026-09-13.** It + signals `BoundsError`, reaches handlers and the break loop, and is fatal only when + nothing answers. `Vec`'s checks went with it, since `(at v i)` and `(at arr i)` are one + form in the source. No restart is established at the site: `retry` exists for + allocation and for files because those attempts are repeatable, and this one is not — + so what answers a bad index is the `continue` the frame loop already offered, which is + the restart this document was pointing at all along. + + Two consequences worth carrying forward. **Defer** had to be decided rather than + inherited — an answered bounds failure runs the function's defers, because it leaves + through the same unwind path a `return` does; an unanswered one still runs none. + And **item 6 below got more valuable**, not less: now that a bad index lands in + `continue` instead of ending the process, the missing half is the rollback, because an + abandoned frame leaves the grid half-written. That is the next thing on this list. 5. **A watch for a running program.** `watch.clj` + `watch.el` + `spy` + `spy-num` is a tool the author built deliberately and uses constantly, and the stopped-stack inspector diff --git a/lib/emit.ml b/lib/emit.ml index f668f2d..67e1fc7 100644 --- a/lib/emit.ml +++ b/lib/emit.ml @@ -707,7 +707,12 @@ let fninfo m (fn : Tast.fn) ~nslots = Indices are i32 in Flan and sign-extended to i64 for the gep, so a negative one arrives here as a huge unsigned value: an unsigned comparison catches - the negative and the too-large case in a single test. *) + the negative and the too-large case in a single test. + + This is the *trapping* shape and it still has four users: the restart + lookups and the unarmed-clause check, none of which is recoverable — there + is nowhere to resume a transfer whose target does not exist. The two bounds + checks moved off it; see [signal_block]. *) let fail_block f (loc : Loc.t) ok emit_call = let good = fresh_label f "inb" and bad = fresh_label f "oob" in term f "br i1 %s, label %%%s, label %%%s" ok good bad; @@ -717,21 +722,44 @@ let fail_block f (loc : Loc.t) ok emit_call = term f "unreachable"; label f good +(* The same branch, for a failure that *signals* rather than dying. The call is + an ordinary one — it returns when a handler or the break loop transferred — + so it is followed by a guard, and the fall-through past the guard is what is + unreachable: nothing answered, so the runtime already died inside the call. + + [guard] is passed in rather than called directly because [guard] is part of + the expression emitter's recursive group and this is defined above it. It is + the same [guard f] every call site emits, so a bounds failure that is + answered leaves the function through the innermost pad — a restart-case's, + or the function's own unwind block, which runs its defers and returns. + **That is the answer to "does a trap run defers": an answered one does, an + unanswered one still does not, because the unanswered one is still a die + inside C.** *) +let signal_block f (loc : Loc.t) ~guard ok emit_call = + let good = fresh_label f "inb" and bad = fresh_label f "oob" in + term f "br i1 %s, label %%%s, label %%%s" ok good bad; + label f bad; + let id, n = string_bytes f.md (Loc.to_string loc) in + emit_call id n; + guard (); + term f "unreachable"; + label f good + (* [at] is strict: the last valid index is len - 1. *) -let check_at f loc idx len = +let check_at f ~guard loc idx len = if f.md.checks then begin let ok = fresh f in ins f "%s = icmp ult i64 %s, %s" ok idx len; - fail_block f loc ok (fun id n -> - ins f "call void @flan_bounds_fail(ptr %s, i64 %d, i64 %s, i64 %s)" - id n idx len) + signal_block f loc ~guard ok (fun id n -> + ins f "call void @flan_bounds_error(ptr %s, i64 %d, i64 %s, i64 %s, ptr %s)" + id n idx len xfer_param) end (* [slice] is not: a slice ending at len — or an empty one at lo = len — is legal, and its one-past-the-end gep is defined. [lo <= hi] is not redundant with it, because a reversed range would otherwise yield hi - lo as a huge unsigned length, which is a worse hole than the missing check. *) -let check_slice f loc lo hi len = +let check_slice f ~guard loc lo hi len = if f.md.checks then begin let a = fresh f in ins f "%s = icmp ule i64 %s, %s" a lo hi; @@ -739,9 +767,11 @@ let check_slice f loc lo hi len = ins f "%s = icmp ule i64 %s, %s" b hi len; let ok = fresh f in ins f "%s = and i1 %s, %s" ok a b; - fail_block f loc ok (fun id n -> - ins f "call void @flan_slice_fail(ptr %s, i64 %d, i64 %s, i64 %s, i64 %s)" - id n lo hi len) + signal_block f loc ~guard ok (fun id n -> + ins f + "call void @flan_slice_error(ptr %s, i64 %d, i64 %s, i64 %s, i64 %s, \ + ptr %s)" + id n lo hi len xfer_param) end (* ── Expressions ───────────────────────────────────────────────────── *) @@ -1026,7 +1056,7 @@ and element_addr f (target : Tast.expr) idx = (match ty with | Types.Array (n, elem) -> (* The bound is static; LLVM folds the check away for a literal index. *) - check_at f i.Tast.loc i64 (Int64.to_string n); + check_at f ~guard:(fun () -> guard f) i.Tast.loc i64 (Int64.to_string n); let p = fresh f in ins f "%s = getelementptr inbounds %s, ptr %s, i64 0, i64 %s" p (ll ty) ptr i64; @@ -1038,7 +1068,7 @@ and element_addr f (target : Tast.expr) idx = ins f "%s = extractvalue %%slice %s, 0" base s; let len = fresh f in ins f "%s = extractvalue %%slice %s, 1" len s; - check_at f i.Tast.loc i64 len; + check_at f ~guard:(fun () -> guard f) i.Tast.loc i64 len; let p = fresh f in ins f "%s = getelementptr inbounds %s, ptr %s, i64 %s" p (ll elem) base i64; go p elem rest @@ -1745,7 +1775,7 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = match target.Tast.ty with | Types.Array (n, _) -> let a = addr f target in - check_slice f e.Tast.loc lo64 hi64 (Int64.to_string n); + check_slice f ~guard:(fun () -> guard f) e.Tast.loc lo64 hi64 (Int64.to_string n); let p = fresh f in ins f "%s = getelementptr inbounds %s, ptr %s, i64 0, i64 %s" p (ll target.Tast.ty) a lo64; @@ -1756,7 +1786,7 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = ins f "%s = extractvalue %%slice %s, 0" q v; let n = fresh f in ins f "%s = extractvalue %%slice %s, 1" n v; - check_slice f e.Tast.loc lo64 hi64 n; + check_slice f ~guard:(fun () -> guard f) e.Tast.loc lo64 hi64 n; let p = fresh f in ins f "%s = getelementptr inbounds %s, ptr %s, i64 %s" p (ll elem) q lo64; p @@ -1766,7 +1796,7 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = ins f "%s = extractvalue %%slice %s, 0" q v; let n = fresh f in ins f "%s = extractvalue %%slice %s, 1" n v; - check_slice f e.Tast.loc lo64 hi64 n; + check_slice f ~guard:(fun () -> guard f) e.Tast.loc lo64 hi64 n; let p = fresh f in ins f "%s = getelementptr inbounds i8, ptr %s, i64 %s" p q lo64; p @@ -1826,13 +1856,25 @@ and prim f (e : Tast.expr) (p : Tast.prim) (args : Tast.expr list) = | t -> [ ll t ^ " " ^ value f a ]) args) in + (* The two runtime entry points whose bounds check signals. They are the + only [Rt] symbols that can transfer, so they are the only ones that take + the channel and the only ones guarded — everything else in this family + is arithmetic over a container header and cannot reach a handler. A Vec + is checked inside the runtime rather than in emitted IR (BUILT.md), so + this is where (at v i) gets what (at arr i) gets from [check_at]. *) + let signals = + String.equal sym "flan_vec_at" || String.equal sym "flan_vec_as_slice" + in + let vs = if signals then vs @ [ "ptr " ^ xfer_param ] else vs in let args' = String.concat ", " vs in if is_void e.Tast.ty then begin ins f "call void @%s(%s)" sym args'; + if signals then guard f; "zeroinitializer" end else begin let t = fresh f in ins f "%s = call %s @%s(%s)" t (ll e.Tast.ty) sym args'; + if signals then guard f; t end | Tast.SizeOf t, [] -> Printf.sprintf "%d" (fst (lay f.md t)) @@ -2286,8 +2328,10 @@ declare void @flan_restart_fail(ptr, i64, ptr, i64) noreturn cold declare void @flan_restart_args_fail(ptr, i64, ptr, i64, ptr, i64, ptr, i64) noreturn cold declare void @flan_restart_unarmed(ptr, i64, ptr, i64, ptr, i64) noreturn cold declare void @flan_transfer_fail(ptr, i64) noreturn cold -declare void @flan_bounds_fail(ptr, i64, i64, i64) noreturn cold -declare void @flan_slice_fail(ptr, i64, i64, i64, i64) noreturn cold +; Not noreturn: each signals BoundsError and returns when something answered +; it, which is the one path out. The trailing ptr is the transfer channel. +declare void @flan_bounds_error(ptr, i64, i64, i64, ptr) cold +declare void @flan_slice_error(ptr, i64, i64, i64, i64, ptr) cold declare ptr @flan_context_allocator() declare ptr @flan_context_temp() declare ptr @flan_heap_allocator() @@ -2311,8 +2355,11 @@ declare i8 @flan_vec_reserve(ptr, i64, i64, i64, ptr, i64) declare i8 @flan_vec_push(ptr, ptr, i64, i64, ptr, i64) declare i8 @flan_vec_clone(ptr, ptr, ptr, i64, i64, ptr, i64) declare i64 @flan_vec_len(ptr, ptr, i64) -declare ptr @flan_vec_at(ptr, i32, i64, ptr, i64) -declare void @flan_vec_as_slice(ptr, ptr, i32, i32, i64, ptr, i64) +; These two take the transfer channel as well, because a Vec's bounds check is +; inside the runtime rather than emitted here and (at v i) has to signal the +; same condition (at arr i) does. +declare ptr @flan_vec_at(ptr, i32, i64, ptr, i64, ptr) +declare void @flan_vec_as_slice(ptr, ptr, i32, i32, i64, ptr, i64, ptr) declare void @flan_vec_free(ptr, i64, i64, ptr, i64) ; (Map K V). The two ptr arguments before the location on put/get/clone are the ; hash and equality pair, which the checker emits per key type and passes here diff --git a/lib/prelude.ml b/lib/prelude.ml index 11eb872..fa56723 100644 --- a/lib/prelude.ml +++ b/lib/prelude.ml @@ -44,6 +44,40 @@ let source = {flan| ;; handler or the break loop, where a working allocator is known. (defstruct StorageExhausted [bytes i64 align i64 allocator i64]) +;; What an out-of-range index signals. Same shape as StorageExhausted and for +;; the same reasons: fixed numeric fields, no rendered message, nothing that +;; allocates — the condition is built on the failing frame's stack and the +;; formatting is the handler's or the break loop's job, where a working +;; allocator is known. +;; +;; It is signalled with `error`, from the runtime rather than from Flan: +;; flan_bounds_error and flan_slice_error in runtime/flan_rt.c, which every +;; bounds check now branches to. **The three fields there are a C struct that +;; has to agree with this one field for field**, the same hand-kept agreement +;; flan_name_id keeps with Check.type_id. +;; +;; `low` and `high` are the same index for an (at xs i), and the two ends of +;; the range for a (slice xs lo hi). One condition type rather than two, +;; because a handler that wants to survive a bad index should not have to +;; write two clauses to cover the two ways of writing one. +;; +;; **Nothing establishes a restart at the failing site**, which is the +;; difference from StorageExhausted and from FileError. Those offer `retry` +;; because their attempt is repeatable: free something, or supply another path, +;; and the same operation succeeds the second time. Nothing a handler can do +;; makes index 51 valid for a length-50 array, so there is no attempt to +;; re-run. `use-value` for the index is the near miss and is not built: it +;; would put an alloca and a restart frame on every indexing operation, and +;; what it buys is a *different element*, silently. +;; +;; The restarts that matter are the ones the program already established — a +;; frame loop's `continue`, sand.flan's shape — and they are on the restart +;; stack and reachable from a handler or from the break loop without anything +;; being pushed here. That is plan.org's "restarts go at the resync point, +;; once", with allocation and file failure as the named exceptions and this on +;; the default side of the rule. +(defstruct BoundsError [low i64 high i64 length i64]) + ;; A breakpoint. (pause) stops the program where it stands and hands it to the ;; break loop, with the whole stack under it readable — C-c C-b lists the ;; frames, TAB opens one, and taking `continue` resumes at the call. diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index 6f38c55..62ed07a 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -466,6 +466,80 @@ _Noreturn void flan_slice_fail(const uint8_t *loc, int64_t loclen, rt_die(); } +/* ── An index out of range is a condition ────────────────────────────── + * + * The two functions above are still here and still die; what changed is that + * they are no longer the *first* thing a bad index reaches. A bounds failure + * now signals BoundsError with `error`, exactly as a failed allocation signals + * StorageExhausted, and only reaches the message above if nothing answered. + * + * Why it had to change. `flan dev` runs the compiler inside the program, in + * one process. exit(134) therefore took the session with it, and the session + * is the thing the project is built around never having to restart. The + * ordinary route into it is not exotic: a grid indexed from a mouse position + * is out of bounds the first time the pointer leaves the window. + * + * **No restart is established here**, and that is a decision rather than an + * omission. flan_alloc_* and the file guards offer `retry` because their + * attempt is repeatable — a handler frees something, or supplies another + * path, and the same operation then succeeds. Nothing a handler can do makes + * index 51 valid for a length-50 array, so there is no attempt to re-run and + * nothing for a site restart to resume into. `use-value` for the index is the + * near miss: it would cost every indexing operation an alloca and a restart + * frame, and what it buys is a *different element*, silently, which is the + * class of answer this codebase refuses everywhere else. The restarts that + * matter are the ones the program already established — a frame loop's + * `continue` — and those are on the restart stack and reachable from the break + * loop without anything being pushed here. + * + * With no break hook — a release build, or any program that did not import the + * agent — flan_break_hook is NULL, nothing transfers, and this falls through + * to the same message and the same status it always had. That is still the + * right answer: there is nowhere to stand. + * + * The condition is three int64s on this frame and it must agree field for + * field with the prelude's (defstruct BoundsError [low i64 high i64 length + * i64]) — the same hand-kept agreement flan_name_id has with Check.type_id, + * and for the same reason: a struct is a layout and a type is a number, and + * neither side can see the other. `low` and `high` are the same index for an + * `at`, and the two ends of the range for a `slice`, so one condition type + * covers both and a handler writes one clause rather than two. */ + +typedef struct { int64_t low, high, length; } flan_bounds_cond; + +static const uint8_t flan_bounds_name[] = "BoundsError"; +#define FLAN_BOUNDS_NAMELEN 11 + +/* Returns nonzero if something transferred, in which case the caller returns + * and its caller's guard carries the transfer out. */ +static int flan_bounds_signal(void *xfer, int64_t low, int64_t high, + int64_t len) { + flan_bounds_cond c; + uint32_t id = flan_name_id(flan_bounds_name, FLAN_BOUNDS_NAMELEN); + c.low = low; + c.high = high; + c.length = len; + flan_signal(id, &c, xfer); + if (*(void **)xfer != NULL) return 1; + if (flan_break_hook != NULL) { + flan_break_hook(flan_bounds_name, FLAN_BOUNDS_NAMELEN, &c, xfer); + if (*(void **)xfer != NULL) return 1; + } + return 0; +} + +void flan_bounds_error(const uint8_t *loc, int64_t loclen, int64_t idx, + int64_t len, void *xfer) { + if (flan_bounds_signal(xfer, idx, idx, len)) return; + flan_bounds_fail(loc, loclen, idx, len); +} + +void flan_slice_error(const uint8_t *loc, int64_t loclen, int64_t lo, + int64_t hi, int64_t len, void *xfer) { + if (flan_bounds_signal(xfer, lo, hi, len)) return; + flan_slice_fail(loc, loclen, lo, hi, len); +} + /* ── Allocators, spec-memory.md ──────────────────────────────────────── * * One type-erased procedure plus an opaque data pointer, which is Odin's @@ -955,23 +1029,40 @@ int64_t flan_vec_len(flan_vec *v, const uint8_t *loc, int64_t loclen) { return v->len; } +/* [xfer] is this operation's end of the transfer channel, so that an index out + * of range signals BoundsError instead of ending the process — see the note + * above flan_bounds_error. (at v i) and (at arr i) are the same form in the + * source and shipping one of them signalling and the other exiting would read + * as a bug, so the two are plumbed together. The channel is the last + * parameter, as it is on every Flan signature; Emit appends it and guards the + * call, and a transfer therefore leaves through the caller's pad with NULL + * here never dereferenced. */ void *flan_vec_at(flan_vec *v, int32_t i, int64_t size, const uint8_t *loc, - int64_t loclen) { + int64_t loclen, void *xfer) { flan_vec_check(v, loc, loclen); /* The same unsigned comparison the fixed-array bounds check uses: a negative * index sign-extends to a huge unsigned and is caught by the one test. */ - if ((uint64_t)(int64_t)i >= (uint64_t)v->len) + if ((uint64_t)(int64_t)i >= (uint64_t)v->len) { + if (flan_bounds_signal(xfer, (int64_t)i, (int64_t)i, v->len)) return NULL; flan_vec_bounds_fail(loc, loclen, (int64_t)i, v->len); + } return (uint8_t *)v->ptr + (int64_t)i * size; } /* [hi] of -1 means "to the end": (as-slice v) has no static length to write. */ void flan_vec_as_slice(flan_vec *v, void *out, int32_t lo, int32_t hi, - int64_t size, const uint8_t *loc, int64_t loclen) { + int64_t size, const uint8_t *loc, int64_t loclen, + void *xfer) { struct { void *p; int64_t n; } s; int64_t l = lo, h = (hi < 0) ? v->len : hi; flan_vec_check(v, loc, loclen); - if (l < 0 || h > v->len || l > h) flan_vec_bounds_fail(loc, loclen, l, v->len); + if (l < 0 || h > v->len || l > h) { + /* Both ends, because both are what went wrong — the fixed-array slice + * check reports the same pair. [out] is left untouched on the transfer + * path; the caller's guard branches before it reads the slice. */ + if (flan_bounds_signal(xfer, l, h, v->len)) return; + flan_vec_bounds_fail(loc, loclen, l, v->len); + } s.p = (uint8_t *)v->ptr + l * size; s.n = h - l; memcpy(out, &s, sizeof s); diff --git a/test/programs/bounds-condition.flan b/test/programs/bounds-condition.flan new file mode 100644 index 0000000..e289c8c --- /dev/null +++ b/test/programs/bounds-condition.flan @@ -0,0 +1,190 @@ +;;;; An index out of range is a condition, not the end of the process. +;;;; +;;;; Until now a bad index printed its source location and called exit(134). +;;;; That was defensible when `flan dev` was two processes; it is not now that +;;;; the compiler runs inside the program, because the trap takes the session +;;;; with it and the session is the thing the project is built around never +;;;; having to restart. And the route in is the most ordinary one there is: a +;;;; grid indexed from a mouse position is out of bounds the first time the +;;;; pointer leaves the window (PORTING.md, §3). +;;;; +;;;; So a failed bounds check signals BoundsError with `error`, the same way a +;;;; failed allocation signals StorageExhausted, and dies with the old message +;;;; only if nothing answered. This program is the "something answered" half — +;;;; the unhandled half is bounds.flan, which still exits 134 with the same +;;;; text it always did. +;;;; +;;;; **No restart is established at the failing index**, and that is the +;;;; decision worth reading this file for. StorageExhausted offers `retry` +;;;; because its attempt is repeatable: free something and the allocation +;;;; succeeds. Nothing a handler can do makes index 7 valid for a length-4 +;;;; array. `use-value` for the index would cost every indexing operation a +;;;; restart frame and buy a *different element*, silently. What answers a bad +;;;; index is the restart the program already had — the frame loop's +;;;; `continue`, which is sand.flan's shape and is what a game wants: abandon +;;;; this frame, keep the window open. +;;;; +;;;; Four things are asserted, and the first two are the ones that matter: +;;;; +;;;; 1. The frame is abandoned and the program carries on. `frames` counts +;;;; the ones that finished and `skipped` the ones that did not. +;;;; 2. **Defers run.** A trap ran none, which BUILT.md recorded as following +;;;; from the noreturn-then-unreachable shape rather than as a decision. +;;;; The shape changed, so the question had to be answered rather than +;;;; inherited: an *answered* bounds failure leaves through the same +;;;; unwind path a `return` uses, and therefore runs the function's +;;;; defers, innermost first. An unanswered one still runs none, because +;;;; it is still a die inside C. +;;;; 3. The condition carries the numbers. `low` and `high` are the same +;;;; index for an `at` and the two ends of the range for a `slice`, which +;;;; is why there is one condition type and not two. +;;;; 4. Every route to a bad index signals: reading a fixed array, writing +;;;; one (a different lowering — place/Pindex, not At), a slice, a Vec +;;;; element, and a Vec's as-slice. A Vec's check lives inside the +;;;; runtime rather than in emitted IR, so those two are plumbed +;;;; separately and are the ones most likely to be left behind. + +(defvar grid [4 i32]) + +;;; Handlers cannot see the locals of the function that established them — +;;; check.ml refuses a capture by name and says to use a global — so +;;; everything this program counts lives up here. +(defvar frames i64) +(defvar skipped i64) +(defvar cleaned i64) +(defvar low i64) +(defvar high i64) +(defvar length i64) + +;;; Two frames deep, with a defer on the way, so the transfer has something to +;;; cross and something to run on its way out. +(defn show [name string n i64] () + (print name) (print " ") (print n) (println "")) + +(defn read-cell [i i32] i32 + (defer (set cleaned (+ cleaned 1))) + (at grid i)) + +(defn write-cell [i i32] () + (defer (set cleaned (+ cleaned 1))) + (set (at grid i) 99)) + +;;; The frame loop's shape: one restart-case around the work, offering +;;; `continue`, which abandons this frame and nothing else. sand.flan's main +;;; loop is this. +(defn read-frame [i i32] () + (restart-case + (do (show "read" (i64 (read-cell i))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1))))) + +(defn write-frame [i i32] () + (restart-case + (do (write-cell i) + (show "wrote" (i64 (at grid i))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1))))) + +(defn slice-frame [s [u8] lo i32 hi i32] () + (restart-case + (do (show "slice" (i64 (len (slice s lo hi)))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1))))) + +;;; The two Vec frames are written inline in main rather than as functions, +;;; because a (Vec T) is move-only: passing one to a helper would hand +;;; ownership over and the caller's binding would be dead afterwards. A +;;; restart-case does not have to be in a different function from the +;;; handler-bind that answers into it — the transfer is by frame address and +;;; the frames here are simply both this one. + +(defn main [] i32 + (set (at grid 0) 10) + (set (at grid 1) 11) + (set (at grid 2) 12) + (set (at grid 3) 13) + + (let [s (bytes "hello") ; len 5 + v (vec-new i32)] + (push v 100) + (push v 200) + + (handler-bind + [(BoundsError [c] + ;; The numbers are here rather than in a message, for the same reason + ;; StorageExhausted has none: formatting allocates, and this is a path + ;; that must be able to run when allocation is what failed. + (set low (.low c)) + (set high (.high c)) + (set length (.length c)) + ;; Abandon the frame. The transfer crosses read-cell (running its + ;; defer) and lands in the clause of the restart-case two frames out. + (invoke-restart 'continue))] + + ;; In bounds: the frame finishes, the handler never runs, and the defer + ;; runs on the ordinary return path. + (read-frame 2) + ;; Past the end, then negative. A negative index sign-extends to a huge + ;; unsigned and is caught by the same single comparison, but the number + ;; the condition carries is the signed one. + (read-frame 7) + (show "low" low) + (show "length" length) + (read-frame -1) + (show "low" low) + + ;; The write path lowers through place/Pindex rather than through At, so + ;; it would be perfectly possible to convert one and not the other. + (write-frame 1) + (write-frame 4) + (show "low" low) + + ;; A slice reports both ends, which is the whole reason low and high are + ;; two fields: [2 9) against a length of 5. + (slice-frame s 1 4) + (slice-frame s 2 9) + (show "low" low) + (show "high" high) + (show "length" length) + ;; A reversed range, which the lo <= hi test is what catches: without it + ;; this builds a slice of length hi - lo as a huge unsigned. + (slice-frame s 3 1) + (show "low" low) + (show "high" high) + + ;; And the Vec pair, whose checks are inside the runtime rather than in + ;; emitted IR — a different code path entirely, and the one most likely + ;; to be left behind by a change made in emit.ml. + (restart-case + (do (show "vec" (i64 (at v 1))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1)))) + (restart-case + (do (show "vec" (i64 (at v 5))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1)))) + (show "low" low) + (show "length" length) + (restart-case + (do (show "vec-slice" (i64 (len (as-slice v 0 2)))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1)))) + (restart-case + (do (show "vec-slice" (i64 (len (as-slice v 0 9)))) + (set frames (+ frames 1))) + (continue [] (set skipped (+ skipped 1)))) + (show "high" high)) + + (free v)) + + ;; Six frames finished, six were abandoned, and every one of the twelve ran + ;; its defer — which is the claim about defer that the old shape could not + ;; make. + (show "frames" frames) + (show "skipped" skipped) + (show "cleaned" cleaned) + ;; The write that did land, and the one that did not: grid[1] is 99 and + ;; nothing else moved. + (print (at grid 0)) (print " ") (print (at grid 1)) (print " ") + (print (at grid 2)) (print " ") (print (at grid 3)) (println "") + 0) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 0cb7ef2..e255953 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -1124,16 +1124,50 @@ let () = let p = Reader.read_file "programs/bounds.flan" |> Parse.program |> Check.program in - if not (contains (Emit.program p) "call void @flan_bounds_fail(") then begin + if not (contains (Emit.program p) "call void @flan_bounds_error(") then begin incr failures; print_endline "FAIL checks on: no bounds call emitted" end; let off = Emit.program ~checks:false p in - if contains off "call void @flan_bounds_fail(" || contains off "call void @flan_slice_fail(" then begin + if contains off "call void @flan_bounds_error(" + || contains off "call void @flan_slice_error(" then begin incr failures; print_endline "FAIL --no-bounds-checks: a check survived" end; + (* The other half of the same change: a bad index that something *answers*. + bounds.flan above is still the unhandled case and still exits 134 with + the same text; this one establishes a frame loop's `continue` and a + handler that takes it, and the program runs to the end. + + Three claims, and the second is the one that had to be decided rather + than inherited. (1) Five frames finish and seven are abandoned, out of + five routes to a bad index — fixed-array read, fixed-array write (a + different lowering), slice, Vec element and Vec as-slice, the last two + checked inside the runtime rather than in emitted IR. (2) `cleaned` is + 5, which is every defer on every one of those paths: an answered bounds + failure leaves through the same unwind path a `return` uses and runs + them, where a trap ran none. (3) The condition's numbers are the real + ones — 7 and -1 for the two bad `at`s, [2 9) and [3 1) for the two bad + slices, with `low` and `high` equal for an index and the two ends for a + range, which is why there is one condition type and not two. + + Also at -O0 and as a dev build: the dev build is the one with the break + loop hook installed, and a handler that transfers must reach the + transfer before the hook rather than after it. *) + let bounds_cond_out = + "read 12\nlow 7\nlength 4\nlow -1\nwrote 99\nlow 4\n\ + slice 3\nlow 2\nhigh 9\nlength 5\nlow 3\nhigh 1\n\ + vec 200\nlow 5\nlength 2\nvec-slice 2\nhigh 9\n\ + frames 5\nskipped 7\ncleaned 5\n10 99 12 13\n" + in + outputs "a bad index is a condition" "programs/bounds-condition.flan" + bounds_cond_out; + outputs ~opt:"-O0" "a bad index is a condition, -O0" + "programs/bounds-condition.flan" bounds_cond_out; + outputs ~dev:true "a bad index is a condition, dev" + "programs/bounds-condition.flan" bounds_cond_out; + (* ── Packages: the link follows the program ──────────────────────── A package's C and linker arguments used to come with the import, whatever [main] did — which is what made sand's two halves two files