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.
This commit is contained in:
parent
0ff4ce56a5
commit
53d49570ec
35
NEXT.md
35
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
|
`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`.
|
`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.
|
||||||
|
|||||||
44
lib/emit.ml
44
lib/emit.ml
@ -558,6 +558,33 @@ let fi_bytes m s =
|
|||||||
id (String.length s) (escape s));
|
id (String.length s) (escape s));
|
||||||
id, String.length 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 fninfo m (fn : Tast.fn) ~nslots =
|
||||||
let nid, nlen = fi_bytes m fn.Tast.name in
|
let nid, nlen = fi_bytes m fn.Tast.name in
|
||||||
let lid, llen = fi_bytes m (Loc.to_string fn.Tast.floc) 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;
|
m.nfi <- m.nfi + 1;
|
||||||
Buffer.add_string m.strs
|
Buffer.add_string m.strs
|
||||||
(Printf.sprintf
|
(Printf.sprintf
|
||||||
"%s = private unnamed_addr constant %%fninfo { ptr %s, i64 %d, ptr %s, i64 %d, i32 %d, i32 0 }\n"
|
"%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);
|
id nid nlen lid llen nslots (slot_fingerprint fn));
|
||||||
id
|
id
|
||||||
|
|
||||||
(* ── Bounds checks ───────────────────────────────────────────────────── *)
|
(* ── 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
|
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. *)
|
constants, and omitting them is an undefined [@.str.N] at link time. *)
|
||||||
let redefinition ?(checks = true) ?(dev = false) ?(debug = false)
|
let redefinition ?(checks = true) ?(dev = false) ?(debug = false)
|
||||||
?(known = fun _ -> true)
|
?(known = fun _ -> true) ?(retains = true)
|
||||||
?call ?(consts = []) (p : Tast.program) ~fns : string =
|
?call ?(consts = []) (p : Tast.program) ~fns : string =
|
||||||
let target name =
|
let target name =
|
||||||
match List.find_opt (fun (f : Tast.fn) -> f.Tast.name = name) p.Tast.fns with
|
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
|
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
|
pointing at; one with any keeps its mapping, which costs a page and is
|
||||||
the same bargain every redefinition already makes. *)
|
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"
|
Buffer.add_string m.out "\n@flan_reload_transient = global i8 1\n"
|
||||||
| None -> ()
|
| None -> ()
|
||||||
end;
|
end;
|
||||||
|
|||||||
@ -804,7 +804,27 @@ let () =
|
|||||||
(* Out of range is refused with the depth, so a client can tell a bad
|
(* Out of range is refused with the depth, so a client can tell a bad
|
||||||
index from a frame with nothing in it. *)
|
index from a frame with nothing in it. *)
|
||||||
let r = ask "(:op \"locals\" :frame 9)" in
|
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;
|
end;
|
||||||
(* Running again, and then the locals verb is refused: a frame that is
|
(* Running again, and then the locals verb is refused: a frame that is
|
||||||
still executing does not hold still long enough to be read. *)
|
still executing does not hold still long enough to be read. *)
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user