The slot fingerprint was emitted and never read back
This commit is contained in:
commit
28fb034beb
28
BUILT.md
28
BUILT.md
@ -1214,8 +1214,32 @@ on what it shows rather than on what it saves.
|
||||
**Four refusals, each by name and with its reason.** Three per slot — invented, not yet bound, and no printer for the
|
||||
type (a map, a function value, a type variable; the arm exists, no program the checker accepts has reached it yet, so
|
||||
it is written and untested) — and two whole frames: one belonging to a `C-x C-e` thunk, whose `Tast` the session does
|
||||
not keep, and one whose slot count does not match the body this session holds, which is a frame running a body that has
|
||||
been redefined since and where every slot index would be a guess.
|
||||
not keep, and one whose *body* is not the body this session holds.
|
||||
|
||||
**The superseded frame, and why a count could not find it.** Installing while stopped is deliberately allowed — it is
|
||||
the fix-it-and-retry loop — so the frame on the stack and the body the session holds can be two bodies of one function.
|
||||
A slot count catches a body that gained or lost a binding and nothing else; the case that matters is a **rename**, which
|
||||
changes neither the count nor the types, and which would otherwise show every *new* name against the *old* body's
|
||||
storage with nothing saying so. That is the confident wrong answer, in the one place someone is working out what went
|
||||
wrong.
|
||||
|
||||
So each `%fninfo` carries a **slot fingerprint**: `Emit.slot_fingerprint` over every slot's name together with the
|
||||
spelling of its type, since either can change on its own. The frame carries the value for the body it was compiled from,
|
||||
`flan_dev_frame_slotsig` reads it, the agent snapshots it with the rest of the frame and puts it on the backtrace line,
|
||||
and `Dev.locals` recomputes it from the body it holds and compares. Different means refused by name — *this frame's body
|
||||
was redefined since it was entered, so its names no longer describe its values* — rather than answered. The count check
|
||||
stays in front of it because its message is the more specific one.
|
||||
|
||||
Computed in `emit.ml` and read from there by `dev.ml`, so there is one definition of it and the two ends cannot drift.
|
||||
It stays **off the wire**: `(:op "backtrace")` still answers four fields per frame, because a hash is not something an
|
||||
editor can act on and the refusal says the fact in words instead. The bound is worth stating: it is a 30-bit hash, so a
|
||||
collision is possible in principle, and it would reproduce exactly the silent wrong answer this catches — but only
|
||||
between two *differing* bodies of a function whose qualified name has already matched, since `find_fn` is what gates the
|
||||
comparison at all.
|
||||
|
||||
The first build of this shipped the fingerprint into `%fninfo` and stopped there — no accessor, no wire field, no
|
||||
comparison — and was left in the tree with its own test red. Four of five hand-offs missing looks exactly like one
|
||||
hand-off dropping a number, which is what the note left behind said it was.
|
||||
|
||||
**A `(Vec T)` shows as `<vec>` and a `(Ptr T)` as `<ptr>`**, because that is what `render.ml` already does for them
|
||||
everywhere else: following a pointer a REPL was handed is not a safe thing to do on someone's behalf, and walking a
|
||||
|
||||
41
NEXT.md
41
NEXT.md
@ -410,11 +410,13 @@ forgotten.
|
||||
Agreed at the end of 2026-09-12. Ordered by priority, not by size. Items 1-3 and 5-6 want the compiler core and should
|
||||
run one lane at a time; item 4 is disjoint and runs alongside any of them.
|
||||
|
||||
1. **Fix the one failing test** — `the frame of a superseded body answered with the new body's names`. The slot
|
||||
fingerprint that should refuse a frame whose body was redefined since it was entered does not fire, so a rename
|
||||
keeping the same slot count and types shows new names against old values. The suite has been green all session apart
|
||||
from this, and a red suite stops being a signal quickly. The handoff says to start by printing both sides of the
|
||||
comparison in `Dev.locals`.
|
||||
~~1. **Fix the one failing test** — `the frame of a superseded body answered with the new body's names`.~~ **Done**, and
|
||||
the handoff's diagnosis was wrong. Nothing was dropping the number: the fingerprint was emitted into `%fninfo` and
|
||||
never read back. `flan_dev.c` called the field `spare`, there was no accessor, the agent never snapshotted it, the
|
||||
backtrace line never carried it, and `Dev.locals` compared slot counts and nothing else — four of the five
|
||||
hand-offs were never written, and printing both sides of the comparison could not have found it because there was
|
||||
no comparison. The mechanism was sound and stayed: it hashes slot *names* as well as types, so it does see a
|
||||
rename. See BUILT.md, "Locals of a stopped frame".
|
||||
|
||||
2. **The colon-to-dot change.** Cheap, mechanical, ~284 sites across 45 files — and it must land **before** `Map`, or
|
||||
map literals and struct literals collide and both have to change instead of one. It gets more expensive every day
|
||||
@ -1122,26 +1124,17 @@ tests assert on the reason, not just on the failure.
|
||||
## 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.
|
||||
BUILT.md's two new sections for the design and the measurements. A third commit was **half-built and its own test
|
||||
left red** on purpose; **it is finished now** — see the struck item 1 above — and the rest of this section is kept
|
||||
because the parts of it that were true are still worth having.
|
||||
|
||||
**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.
|
||||
**What is broken, exactly — and this paragraph was wrong; kept for what it cost.** It said `locals` compares the
|
||||
frame on the stack against the body this session holds and the comparison is not firing, that every piece of the
|
||||
fingerprint was written, and that one of five hand-offs was dropping the number. Four of the five were never written at
|
||||
all: `Emit.fninfo` stored the fingerprint and nothing else touched it. The first step it recommended — printing both
|
||||
sides of the comparison in `Dev.locals` — could not have worked, because `Dev.locals` had no comparison to print. The
|
||||
lesson is the ordinary one: a lane that stops mid-repair should say which pieces it *ran*, not which it believes it
|
||||
wrote.
|
||||
|
||||
**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
|
||||
|
||||
69
lib/dev.ml
69
lib/dev.ml
@ -235,7 +235,15 @@ let restarts t =
|
||||
framing [restarts] uses, terminated by a lone dot, because it comes back
|
||||
over the same one-line-out socket.
|
||||
|
||||
Each line is [I ± NSLOTS LOC NAME]. The flag says whether the frame belongs
|
||||
Each line is [I ± NSLOTS SIG LOC NAME]. [SIG] is the slot fingerprint of
|
||||
the body this frame was compiled from — [Emit.slot_fingerprint] over the
|
||||
name and the type of every slot — and it is how [locals] tells a frame whose
|
||||
body has been redefined underneath it from one that still matches. It stays
|
||||
off the wire: a hash is not something a client can act on, and the refusal
|
||||
it produces says the fact in words instead. It sits before [LOC] because
|
||||
[NAME] is the only field that can contain a space and so has to be last.
|
||||
|
||||
The flag says whether the frame belongs
|
||||
to the program or to the C-x C-e thunk the break happens to be inside: a
|
||||
break inside an evaluation has that evaluation's frames on top, and
|
||||
answering "where is my program" with [eval/7] would be true and useless.
|
||||
@ -264,11 +272,14 @@ let backtrace t =
|
||||
end
|
||||
else
|
||||
match String.split_on_char ' ' line with
|
||||
| idx :: flag :: nslots :: loc :: rest when rest <> [] ->
|
||||
(match int_of_string_opt idx, int_of_string_opt nslots with
|
||||
| Some _, Some k ->
|
||||
| idx :: flag :: nslots :: sig_ :: loc :: rest when rest <> [] ->
|
||||
(match
|
||||
int_of_string_opt idx, int_of_string_opt nslots,
|
||||
int_of_string_opt sig_
|
||||
with
|
||||
| Some _, Some k, Some g ->
|
||||
Some (String.concat " " rest, (if loc = "?" then "" else loc),
|
||||
flag = "+", k)
|
||||
flag = "+", k, g)
|
||||
| _ -> None)
|
||||
| _ -> None
|
||||
in
|
||||
@ -700,7 +711,7 @@ let backtrace_op t =
|
||||
[ ":frames "
|
||||
^ Wire.list
|
||||
(List.map
|
||||
(fun (name, loc, mine, nslots) ->
|
||||
(fun (name, loc, mine, nslots, _sig) ->
|
||||
Wire.list
|
||||
[ Wire.quote name; Wire.quote loc;
|
||||
Wire.quote (if mine then "program" else "eval");
|
||||
@ -731,9 +742,17 @@ let backtrace_op t =
|
||||
showed neither would be the same lie twice.
|
||||
|
||||
And two whole frames it refuses: one belonging to a [C-x C-e] thunk, which
|
||||
this session does not keep the [Tast] of, and one whose slot count does not
|
||||
match the body this session holds — which is a frame running a body that
|
||||
has since been redefined, where every slot index would be a guess. *)
|
||||
this session does not keep the [Tast] of, and one whose *body* is not the
|
||||
body this session holds. The second is the one that needed a fingerprint
|
||||
rather than a count: installing while stopped is deliberately allowed — it
|
||||
is the fix-it-and-retry loop — so the frame on the stack and the body here
|
||||
can be two bodies of one function, and a redefinition that renames a local
|
||||
changes neither the count nor the types. [Emit.slot_fingerprint] hashes
|
||||
every slot's name together with the spelling of its type, the frame carries
|
||||
the value for the body it was compiled from, and this end recomputes it
|
||||
from the body it holds. A collision is possible in principle — it is a
|
||||
30-bit hash — but only between two differing bodies of the function whose
|
||||
qualified name already matched, since [find_fn] gates the comparison. *)
|
||||
let locals t ~frame =
|
||||
if not (alive t) then error "the program exited; restart flan dev"
|
||||
else
|
||||
@ -752,7 +771,7 @@ let locals t ~frame =
|
||||
error
|
||||
(Printf.sprintf "there is no frame %d; the backtrace has %d" frame
|
||||
(List.length frames))
|
||||
| Some (name, _, mine, nslots) ->
|
||||
| Some (name, _, mine, nslots, sig_) ->
|
||||
if not mine then
|
||||
error
|
||||
(name
|
||||
@ -764,17 +783,35 @@ let locals t ~frame =
|
||||
(name
|
||||
^ " 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
|
||||
(* The two body checks come first, including for a frame
|
||||
with no slots. "every slot in it is one the compiler made
|
||||
up" is a claim about the body this session holds, and a
|
||||
zero-slot frame whose body has since been replaced by one
|
||||
with slots is a frame that claim is false about. *)
|
||||
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"
|
||||
name nslots name (Array.length fn.Tast.slots))
|
||||
else if sig_ <> Emit.slot_fingerprint fn then
|
||||
(* The count matching is not the same as the body matching.
|
||||
A redefinition that renames a local, or changes its type
|
||||
to one of the same shape, keeps the count — and then
|
||||
every name here would be the new body's read against the
|
||||
old body's storage, which is the "visible rather than
|
||||
correct" answer this project refuses to give. Said by
|
||||
name, because a frame that is missing and a frame that
|
||||
cannot be trusted are different facts. *)
|
||||
error
|
||||
(Printf.sprintf
|
||||
"%s on the stack was compiled from a different body than the %s this session holds: this frame's body was redefined since it was entered, so its names no longer describe its values"
|
||||
name name)
|
||||
else 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" ]
|
||||
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"
|
||||
name nslots name (Array.length fn.Tast.slots))
|
||||
else
|
||||
match bound_slots t ~frame with
|
||||
| Error m -> error ("the program refused to say which slots are bound: " ^ m)
|
||||
|
||||
@ -335,7 +335,13 @@ typedef struct {
|
||||
const char *loc; /* file:line:col, as Loc spells it */
|
||||
int64_t loclen;
|
||||
int32_t nslots;
|
||||
int32_t spare;
|
||||
/* What the two ends compare about a frame's slots, from
|
||||
[Emit.slot_fingerprint]: a hash over every slot's name and the spelling of
|
||||
its type. The frame carries the one belonging to the body it was compiled
|
||||
from; the daemon recomputes it from the body it now holds, and a
|
||||
difference means the frame is running a superseded body. A count alone
|
||||
cannot see a rename, which is the case this exists for. */
|
||||
int32_t slotsig;
|
||||
} flan_fninfo;
|
||||
|
||||
typedef struct flan_frame {
|
||||
@ -390,6 +396,14 @@ int32_t flan_dev_frame_nslots(const void *frame) {
|
||||
return (f == NULL || f->info == NULL) ? 0 : f->info->nslots;
|
||||
}
|
||||
|
||||
/* The fingerprint of the body this frame was compiled from. Zero for a frame
|
||||
* with no description, which is the same "nothing to compare" a zero slot
|
||||
* count already means. */
|
||||
int32_t flan_dev_frame_slotsig(const void *frame) {
|
||||
const flan_frame *f = frame;
|
||||
return (f == NULL || f->info == NULL) ? 0 : f->info->slotsig;
|
||||
}
|
||||
|
||||
/* Where slot [i] of this frame lives, or NULL — which means one of three
|
||||
* things, all of which are "there is nothing to read here": this build records
|
||||
* no slots, the index is not one of them, or the binding that fills it had not
|
||||
|
||||
@ -806,6 +806,24 @@ let () =
|
||||
let r = ask "(:op \"locals\" :frame 9)" in
|
||||
if status r <> "error" then fail "a frame index past the end answered";
|
||||
|
||||
(* The inverse, first, because it is the cheap half of the same
|
||||
claim: a redefinition that really does change the slots must be
|
||||
refused too, and that one a count comparison can see. This body
|
||||
drops [flag] and keeps the signature, so the frame on the stack has
|
||||
one slot more than the body this session now holds. *)
|
||||
let r =
|
||||
ask
|
||||
"(:op \"eval\" :code \"(defn look [n i64 label string] i64 (let [p (Point {:x 1.5 :y 2.5}) xs [10 20 30]] (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 body with fewer slots 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 body whose slots changed answered anyway"
|
||||
end;
|
||||
|
||||
(* 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
|
||||
@ -823,7 +841,16 @@ let () =
|
||||
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"
|
||||
fail "the frame of a superseded body answered with the new body's names";
|
||||
(* And the other half of that claim, which is the one a fingerprint
|
||||
that never matched would fail: redefining [look] says nothing
|
||||
about [main], and its frame must still answer. A refusal that
|
||||
fires for every frame would pass the test above and make the
|
||||
whole verb useless. *)
|
||||
let r = ask "(:op \"locals\" :frame 1)" in
|
||||
if status r <> "ok" then
|
||||
fail "redefining one function refused an untouched frame: %s"
|
||||
(Option.value ~default:(status r) (Wire.string_field r "message"))
|
||||
end
|
||||
end;
|
||||
(* Running again, and then the locals verb is refused: a frame that is
|
||||
|
||||
14
vendor/agent/flan_agent.c
vendored
14
vendor/agent/flan_agent.c
vendored
@ -155,6 +155,7 @@ extern void *flan_dev_frame_at(int32_t i);
|
||||
extern const char *flan_dev_frame_name(const void *frame, int64_t *len);
|
||||
extern const char *flan_dev_frame_loc(const void *frame, int64_t *len);
|
||||
extern int32_t flan_dev_frame_nslots(const void *frame);
|
||||
extern int32_t flan_dev_frame_slotsig(const void *frame);
|
||||
extern void *flan_dev_frame_slot(const void *frame, int32_t i);
|
||||
extern const uint8_t *flan_restart_name(int32_t i, int64_t *len);
|
||||
extern void *flan_restart_frame(int32_t i);
|
||||
@ -259,6 +260,8 @@ typedef struct {
|
||||
int32_t fnoff[FRAME_MAX], fnlen[FRAME_MAX];
|
||||
int32_t floff[FRAME_MAX], fllen[FRAME_MAX];
|
||||
int32_t fslots[FRAME_MAX];
|
||||
int32_t fsig[FRAME_MAX]; /* the slot fingerprint of the body
|
||||
* this frame was compiled from */
|
||||
int32_t fmine[FRAME_MAX]; /* 0 = the evaluation's, not the
|
||||
* program's */
|
||||
char ftext[FRAME_TEXT];
|
||||
@ -363,6 +366,10 @@ static int snap_push(void) {
|
||||
s->fused += (int32_t)ll;
|
||||
s->ftext[s->fused++] = 0;
|
||||
s->fslots[s->fn] = flan_dev_frame_nslots(fr);
|
||||
/* Snapshotted with the rest of the frame rather than read later: the
|
||||
* module this description lives in can be unloaded once the daemon
|
||||
* installs a replacement, and the comparison happens after that. */
|
||||
s->fsig[s->fn] = flan_dev_frame_slotsig(fr);
|
||||
/* The outermost [frame_floor] frames are the program's; anything above
|
||||
* them belongs to the evaluation this break is inside. */
|
||||
s->fmine[s->fn] = (frame_floor < 0) || (i >= fn - frame_floor);
|
||||
@ -667,8 +674,11 @@ static void serve(int fd) {
|
||||
}
|
||||
for (int32_t i = 0; i < s->fn; i++) {
|
||||
char hdr[64];
|
||||
int k = snprintf(hdr, sizeof hdr, "%d %c %d ", i,
|
||||
s->fmine[i] ? '+' : '-', s->fslots[i]);
|
||||
/* The fingerprint goes before the location and the location before
|
||||
* the name, because the name is the one field that can contain a
|
||||
* space and so has to be last. */
|
||||
int k = snprintf(hdr, sizeof hdr, "%d %c %d %d ", i,
|
||||
s->fmine[i] ? '+' : '-', s->fslots[i], s->fsig[i]);
|
||||
if (k > 0) send(fd, hdr, (size_t)k, MSG_NOSIGNAL);
|
||||
if (s->fllen[i] > 0)
|
||||
send(fd, s->ftext + s->floff[i], (size_t)s->fllen[i], MSG_NOSIGNAL);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user