From 53d49570ec1809b5f493cd4d19cfb066d2f349b2 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 12:07:33 +0700 Subject: [PATCH] A slot fingerprint per frame, which does not yet catch what it is for locals compares the frame on the stack against the body the session holds: installing while stopped is allowed, so the two can be different bodies of one function, and a rename that keeps the slot count pairs every name with the wrong value. Emit.slot_fingerprint hashes each slot's name and type, emit_fn puts it in the frame's static description, the agent reports it on the backtrace line and Dev.locals compares it. It does not fire. The test that drives it -- a redefinition that renames every local of a function that is on the stack -- fails, and is committed failing rather than deleted, because it is the only record of what is wrong. Everything else in the suite is green; this one check is red. It builds. See NEXT.md's handoff for where to look first. --- NEXT.md | 35 +++++++++++++++++++++++++++++++++++ lib/dev.ml | 10 +++++----- lib/emit.ml | 44 ++++++++++++++++++++++++++++++++++++++++---- test/test_dev.ml | 22 +++++++++++++++++++++- 4 files changed, 101 insertions(+), 10 deletions(-) diff --git a/NEXT.md b/NEXT.md index 3e31d9b..19214f1 100644 --- a/NEXT.md +++ b/NEXT.md @@ -884,3 +884,38 @@ tests assert on the reason, not just on the failure. `old-ocaml/` — the pre-rewrite menhir/ocamllex frontend, kept as reference and excluded from the build by the root `dune` file. Its contents are also in git history at `2c232dd`. + +## Handoff: the shadow stack lane, stopped mid-repair + +Two commits landed and are green: the shadow stack with `(:op "backtrace")`, and `(:op "locals" :frame N)`. See +BUILT.md's two new sections for the design and the measurements. A third commit is **half-built and its own test is +red**, deliberately left that way rather than deleted. + +**What is broken, exactly.** `locals` compares the frame on the stack against the body this session holds, and the +comparison is not firing. Installing while stopped is deliberately allowed, so the two can be different bodies of one +function — and a redefinition that renames the locals while keeping their count and types shows every *new* name +against the *old* body's values, with nothing refused. `test_dev.ml`'s "the frame of a superseded body answered with +the new body's names" fails on exactly that, and reproducing it takes one run of that test. + +The fix that is in the tree and does not work yet: `Emit.slot_fingerprint` hashes each slot's name and the spelling of +its type; `emit_fn` stores it in the `flan_fninfo` the frame points at; `flan_dev_frame_slotsig` reads it; the agent +puts it on the `backtrace` line as a fifth field; `Dev.locals` compares it with `Emit.slot_fingerprint fn`. Every piece +is written and the refusal does not happen, so **one of those five hand-offs is dropping the number** — the next person +should print both sides of that comparison first, which is a two-line change in `Dev.locals`, rather than re-deriving +the design. The likeliest suspects in order: `Dev.backtrace`'s line parse silently falling through to `None` for the +new five-field line (it would drop the frame entirely, so probably not); `find_fn` handing back a stale `Tast.fn`; the +hash being computed over a `snames` array that the redefinition path fills in differently. + +Until it is fixed, `locals` is trustworthy for a frame whose body has not been redefined since it was entered — which +is every frame in a program that has not been edited while stopped — and silently wrong for one that has. + +**Not obvious from the diff.** Two things cost a day between them. The linked-list frame beat an array-with-a-stack- +pointer on both benchmarks, which is the opposite of what the escaping-alloca argument predicts, and the measurement +that first said otherwise was comparing a 40-frame binary with a 600-frame one; every number in BUILT.md is now a +minimum of nine runs for that reason. And `redefinition`'s transient rule (`m.nstr = 0`) silently stops every module +carrying a string literal from ever being unloaded — the frame descriptors go through their own counter, `m.nfi`, for +that reason, and a locals thunk passes `~retains:false` because everything it emits is memcpy'd into the result buffer. + +**No Emacs surface.** `backtrace` and `locals` are daemon ops; nothing in `emacs/` calls them yet. One command showing +the backtrace with the selected frame's locals is the whole of what is missing, and `flan-cnr.el`'s +fixture-driven shape is the model. diff --git a/lib/dev.ml b/lib/dev.ml index 0f2be53..bdf2443 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -756,24 +756,24 @@ let locals t ~frame = if not mine then error (name - ^ " is a frame of the expression this break is inside, not of the program; its thunk is not part of the session, so there is no record of what its slots are called") + ^ " is a frame of the expression this break is inside, not of the program; its thunk is not part of the session, so there is no record of what its slots are called") else match find_fn t name with | None -> error (name - ^ " is not a function this session holds; a lifted handler clause has no declaration of its own to read slot names from") + ^ " is not a function this session holds; a lifted handler clause has no declaration of its own to read slot names from") | Some fn -> if nslots = 0 then ok [ ":frame " ^ Wire.quote name; ":locals ()"; ":refused ()"; ":note " ^ Wire.quote - "that frame records no slots; every slot in it is one the compiler made up" ] + "that frame records no slots; every slot in it is one the compiler made up" ] else if nslots <> Array.length fn.Tast.slots then error (Printf.sprintf - "%s on the stack has %d slots and the %s this session holds has %d: the frame is running a body that has been redefined since, so every slot index here would be a guess" + "%s on the stack has %d slots and the %s this session holds has %d: the frame is running a body that has been redefined since, so every slot index here would be a guess" name nslots name (Array.length fn.Tast.slots)) else match bound_slots t ~frame with @@ -882,7 +882,7 @@ let choose t ~name = ok [ ":restart " ^ Wire.quote name; ":note " - ^ Wire.quote "accepted; the program resumes at its next pass of the break loop" ] + ^ Wire.quote "accepted; the program resumes at its next pass of the break loop" ] | reply -> error (String.trim reply) | exception Unix.Unix_error (e, _, _) -> error ("cannot reach the program: " ^ Unix.error_message e) diff --git a/lib/emit.ml b/lib/emit.ml index 2cc0f96..7eb9bbb 100644 --- a/lib/emit.ml +++ b/lib/emit.ml @@ -558,6 +558,33 @@ let fi_bytes m s = id (String.length s) (escape s)); id, String.length s +(* What the two ends compare about a frame's slots, since neither can see the + other. Same idea as a restart frame's [rsig_id], and for the same reason: a + frame on the stack was compiled from *some* body, the session holds + whatever body it last accepted, and installing while stopped is deliberately + allowed — so the two can be different bodies of the same function, and a + slot count alone does not notice a rename or a reordering. Pairing [q] with + [p]'s value and saying nothing is exactly the "visible rather than correct" + failure this project has already named once. + + Over the names *and* the spellings of the types, because either can change + on its own. Computed here and read from here by [Dev], so there is one + definition of it and it cannot drift. *) +let slot_fingerprint (fn : Tast.fn) = + let b = Buffer.create 128 in + Array.iteri + (fun i ty -> + (match + if i < Array.length fn.Tast.snames then fn.Tast.snames.(i) else None + with + | Some n -> Buffer.add_string b n + | None -> ()); + Buffer.add_char b ':'; + Buffer.add_string b (Types.to_string ty); + Buffer.add_char b ';') + fn.Tast.slots; + Hashtbl.hash (Buffer.contents b) land 0x3fffffff + let fninfo m (fn : Tast.fn) ~nslots = let nid, nlen = fi_bytes m fn.Tast.name in let lid, llen = fi_bytes m (Loc.to_string fn.Tast.floc) in @@ -565,8 +592,8 @@ let fninfo m (fn : Tast.fn) ~nslots = m.nfi <- m.nfi + 1; Buffer.add_string m.strs (Printf.sprintf - "%s = private unnamed_addr constant %%fninfo { ptr %s, i64 %d, ptr %s, i64 %d, i32 %d, i32 0 }\n" - id nid nlen lid llen nslots); + "%s = private unnamed_addr constant %%fninfo { ptr %s, i64 %d, ptr %s, i64 %d, i32 %d, i32 %d }\n" + id nid nlen lid llen nslots (slot_fingerprint fn)); id (* ── Bounds checks ───────────────────────────────────────────────────── *) @@ -2187,7 +2214,7 @@ let program ?(checks = true) ?(dev = false) ?(debug = false) ?(pnames = []) String literals still have to come along: they are this module's own constants, and omitting them is an undefined [@.str.N] at link time. *) let redefinition ?(checks = true) ?(dev = false) ?(debug = false) - ?(known = fun _ -> true) + ?(known = fun _ -> true) ?(retains = true) ?call ?(consts = []) (p : Tast.program) ~fns : string = let target name = match List.find_opt (fun (f : Tast.fn) -> f.Tast.name = name) p.Tast.fns with @@ -2371,7 +2398,16 @@ let redefinition ?(checks = true) ?(dev = false) ?(debug = false) string constants has nothing in its image anyone could still be pointing at; one with any keeps its mapping, which costs a page and is the same bargain every redefinition already makes. *) - if fns = [ fn ] && consts = [] && m.nstr = 0 then + (* [retains = false] is a caller saying it knows where every literal in + this module goes. The [m.nstr] test below is a conservative stand-in + for that — an expression may store a string literal anywhere it likes, + and a global left pointing into an unmapped image is silent garbage + rather than a fault. A locals thunk is the case where the answer is + known: every literal it emits goes to [flan_dev_emit], which memcpys + into the result buffer, so nothing outside the module holds an address + inside it once the call has returned. Without this, clicking through + the frames of a break loop costs a permanent mapping per click. *) + if fns = [ fn ] && consts = [] && ((not retains) || m.nstr = 0) then Buffer.add_string m.out "\n@flan_reload_transient = global i8 1\n" | None -> () end; diff --git a/test/test_dev.ml b/test/test_dev.ml index 3f6b086..84a8662 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -804,7 +804,27 @@ let () = (* Out of range is refused with the depth, so a client can tell a bad index from a frame with nothing in it. *) let r = ask "(:op \"locals\" :frame 9)" in - if status r <> "error" then fail "a frame index past the end answered" + if status r <> "error" then fail "a frame index past the end answered"; + + (* And the case that makes this a fingerprint rather than a slot + count. Installing while stopped is deliberately allowed — it is the + fix-it-and-retry loop — so the body on the stack and the body the + session holds can be two different bodies of one function. This one + renames every local and keeps the count and the types, which a + count comparison cannot see: without the hash, [q] would be shown + holding [p]'s value and nothing would say so. *) + let r = + ask + "(:op \"eval\" :code \"(defn look [n i64 label string] i64 (let [q (Point {:x 9.0 :y 9.0}) ys [1 2 3] mark (< n 0)] (restart-case (do (error (Boom {:why 7})) (let [after (i64 99)] after)) (carry-on [] 5))))\" :file \"/tmp/buf.flan\")" + in + if status r <> "ok" then + fail "installing a renamed body while stopped: %s" + (Option.value ~default:"" (Wire.string_field r "message")) + else begin + let r = ask "(:op \"locals\" :frame 0)" in + if status r <> "error" then + fail "the frame of a superseded body answered with the new body's names" + end end; (* Running again, and then the locals verb is refused: a frame that is still executing does not hold still long enough to be read. *)