JosephFerano/flan
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Memcheck is told an arena reset happened, and the agentless session is pinned

Browse Source 
Two loose ends.

The arena was invisible to memcheck. free-all is retain-capacity, so from
malloc's point of view nothing died and round two of a reset arena could read
a byte it never wrote, print round one's value, and draw no report.
flan_arena_proc now issues memcheck's MAKE_MEM_UNDEFINED over the whole
capacity beside its registry call. Measured on the same machine: the control
produced ERROR SUMMARY 0 before and 6 errors from 4 contexts after, with
--track-origins naming the client request. It is a control in
test_valgrind.ml now rather than a printed note.

The macro is vendored, not included, and the argument is measurement: the
machine that runs the sweep has valgrind and not valgrind-devel, so a guarded
#include would compile to nothing exactly where it matters and the control
would go quiet with no diagnostic. There is also nowhere to put an -I --
flan_rt.c is cat'd into an OCaml string literal and handed to clang in a
scratch directory. The __x86_64__ guard is load-bearing: the same runtime is
built for wasm32-wasi and emscripten.

Cost outside valgrind: 23 instructions on the free-all path only, about 1ns
per reset over fifty million of them, against a run-to-run spread wider than
the effect. Nothing on alloc, resize or free. valgrind.supp still holds no
suppressions; the corpus stayed clean across the change, which is its own
finding.

merged_serve's warning path deserved a test and has one. The discriminating
fact is not the log line but the policy: two_process kills its child and
fails where merged_serve warns and serves anyway, and nothing held that
second answer in place. dev-noagent.flan plus the last block of test_dev.ml
assert the session still answers describe after the wait runs out. Verified
by reverting the policy: the block reports rather than passing. It costs the
full ten seconds and there is no way to spend less. HANDOFF-f1.md is deleted.
This commit is contained in:
Joseph Ferano 2026-09-13 17:46:58 +07:00
parent 888915402e
commit d1464ee266
8 changed files with 301 additions and 194 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
BUILT.md
View File

@ -760,8 +760,9 @@ binary, and twenty-eight of them twice over is minutes against the suite's secon
## Valgrind, and the two questions a sanitizer cannot be asked
`dune build --root . @valgrind` runs the headless corpus under memcheck: forty-seven programs checked, twelve of them a
second time with `--no-bounds-checks`, in **88 seconds** against `@sanitize`'s nine minutes — and that figure includes
`dune build --root . @valgrind` runs the headless corpus under memcheck: forty-nine programs checked, twelve of them a
second time with `--no-bounds-checks`, in **91 seconds** against `@sanitize`'s nine minutes — warm; a cold object cache
puts it at 162s, which is the compiles. That figure includes
the compiles, because the programs are built once here rather than twice. Memcheck is 20–50x on execution and the
corpus is small; the sanitized build's static link was always the expensive half.
@ -796,13 +797,30 @@ anything through fixed arrays.
The sweep is clean. Enumerated, in the spirit of "three of six is a ceiling, not a measurement of the risk":
- **Globals and stack are outside it.** Measured: 0 of 6 against ASan's 3 of 6.
- **Arena storage reused after `free-all` is not re-poisoned.** Measured, and it is the sharpest hole. Round one
writes four elements; `free-all` resets the offset and keeps the pages; round two allocates the same bytes back and
reads one it never wrote — and prints round one's `44`, with memcheck silent. Nothing told memcheck the storage
died, because from `malloc`'s point of view it did not: an arena is one `malloc(cap)` and `free-all` is an integer
going to zero inside it. So the uninitialised-read coverage the control demonstrates holds for the **heap**
allocator and not for the per-frame pattern the arena exists for. Closing it means `VALGRIND_MAKE_MEM_UNDEFINED`
in `flan_arena_proc`, which is a `runtime/` change.
- ~~**Arena storage reused after `free-all` is not re-poisoned.**~~ **Closed.** It was the sharpest hole and it is
now the fourth control. Round one writes four elements; `free-all` resets the offset and keeps the pages; round two
allocates the same bytes back and reads one it never wrote — and still prints round one's `44`, because nothing
about the program changed, but memcheck now reports the read and `--track-origins` names `flan_arena_proc` under
`flan_alloc_free_all` as where the undefined bytes came from. Measured on the same machine: ERROR SUMMARY **0**
before, **6 errors from 4 contexts** after. `free-all` now issues memcheck's `MAKE_MEM_UNDEFINED` over
`[base, cap)` beside the registry's `flan_dev_reg_dead_range`, so the uninitialised-read coverage the headline
control demonstrates holds for the per-frame pattern the arena exists for as well as for the heap.
The client request is **vendored into `flan_rt.c`, not included**, and the argument is measurement rather than
taste: the machine that runs the sweep has `/usr/bin/valgrind` and no `/usr/include/valgrind` — `valgrind-devel` is
a separate package — so a guarded `#include` would compile to nothing on the one box where it matters and the
control proving it works would go quiet with no diagnostic. There is also nowhere to put an `-I`: `flan_rt.c` is
`cat`'d into an OCaml string literal (`lib/dune`, `runtime_src.ml`) and handed to clang in a scratch directory.
The vendored macro is `#if defined(__x86_64__)`-guarded, load-bearing rather than defensive, because the same
runtime is compiled for wasm32-wasi and emscripten where the inline asm would not assemble.
**Cost, measured.** Outside valgrind the request is four `rolq $n,%rdi` — which leave `%rdi` as they found it — and
`xchgq %rbx,%rbx`; 23 extra instructions on the `FLAN_ALLOC_FREE_ALL` path only, `flan_arena_proc` going from 151
to 174 instructions at `-O2`, and `flan_rt.o` from 53,008 to 53,112 bytes. Nothing on `alloc`, `resize` or `free`.
Fifty million `free-all`s in a loop: best-of-seven **1.19s** before and **1.24s** after, i.e. about **1ns** per
`free-all`, against a run-to-run spread of 0.18s within each series. At one arena reset per frame that is
unmeasurable; it is stated as a per-call number rather than "inside noise" because the noise floor here is wider
than the effect.
- **Interior overruns inside a single allocation are invisible by construction.** The arena's alignment padding and
the gap between its offset and its capacity are one block to memcheck, so a read across a sub-object boundary
crosses nothing. The same holds for the `Map`, whose `data` is *one* allocation laid out `keys | values | hashes |
@ -4768,16 +4786,21 @@ The two callers of `Render` already differ in an emitter record; they now differ
runs, so printing one would make a rendering — and any test that reads one — depend on where the heap landed. It is
the rule `Render` already follows for an allocator.
### The arena hole: answerable, not reported
### The arena hole: answerable *and* reported — two tools, still two claims
`test_valgrind.ml` measures a hole and this does not close it. `free-all` is retain-capacity: the offset goes to zero,
the pages stay mapped, and from `malloc`'s point of view nothing died — so memcheck is never told, and a later read of
stale bytes is a read of memory that is, as far as it knows, perfectly alive.
`free-all` is retain-capacity: the offset goes to zero, the pages stay mapped, and from `malloc`'s point of view
nothing died. Both halves of the answer are about telling something that fact, and they tell different somethings.
The registry **is** told. What that buys is that the same read is *answerable*: a pointer into a released region comes
back dead and names what used to be there. Memcheck still says nothing, and closing that half is
`VALGRIND_MAKE_MEM_UNDEFINED` in `flan_arena_proc`, which `test_valgrind.ml` names as the fix and which is still not
written. **The two must not be blurred into one claim.**
The registry is told by `flan_dev_reg_dead_range`. What that buys is that a later read is *answerable*: a pointer into
a released region comes back dead and names what used to be there. That is for a person at an editor, in a dev build.
Memcheck is told by `FLAN_VG_MAKE_MEM_UNDEFINED` on the line beneath it, added later. What that buys is that the read
is *reported*, in every build, to whoever runs the sweep — the definedness bits are reset, so round two reading a byte
it never wrote is an uninitialised read rather than a silent reprint of round one's value.
**The two still must not be blurred into one claim.** They reach different people through different tools, and
neither is evidence for the other: the registry says nothing under memcheck, and the client request renders nothing at
an editor.
### What a release build actually carries, said honestly

106
HANDOFF-f1.md
View File

@ -1,106 +0,0 @@
# Handoff — the `test_dev.ml` "never bound agent.sock" flake
## Root cause
`test_dev.ml`'s first block checked `Sys.file_exists agent_sock` without having completed a single
round-trip on the daemon socket, so nothing had ordered the check after the bind.
The evidence is in `lib/dev.ml`:
- `merged_setup` (lib/dev.ml:2264) binds and listens on the **dev** socket (`Unix.bind ls` at
lib/dev.ml:2300) and returns. The program's main thread — the thread that calls `agent/start`,
which is what binds `agent.sock` (`vendor/agent/flan_agent.c:1097` `flan_agent_start`) — only runs
*after* `merged_setup` returns.
- So the test's `await (fun () -> Sys.file_exists sock)` (test/test_dev.ml:76) says nothing about
`agent.sock`, and `connect sock` (test/test_dev.ml:82) succeeds off the **listen backlog** without
anyone having accepted: `merged_serve` (lib/dev.ml:2311) does not reach `accept_loop` until after
its own `await ~ms:10000 (fun () -> Sys.file_exists t.agent)` (lib/dev.ml:2322).
- The comment at lib/dev.ml:2314-2320 states the ordering outright.
There is a **second race on the same path**, which the timings confirmed. The test unlinks
`agent.sock` immediately after the check (test/test_dev.ml, `Unix.unlink agent_sock`). Both the test
and `merged_serve` poll that path at 5ms. When the test won and unlinked first, `merged_serve` burned
its full 10s timeout and printed the "does it call (agent/start ...)?" warning before accepting — the
test still passed, ~10s slower. Measured: failing runs clustered at ~24s, passing runs at ~30-36s.
## What changed
One file: `test/test_dev.ml`. The existing `describe` request and its status check were **moved up**
to sit immediately before the `agent_sock` existence check (and removed from their old position a few
lines below). No new sleep, no lengthened timeout, no change to `lib/dev.ml`.
A completed reply proves `merged_serve` got past its `await` — i.e. it has *already observed*
`agent.sock` — which makes the existence check deterministic and puts the unlink safely after that
observation, killing both races at once.
The genuine-failure property is preserved: if a program really never binds, `merged_serve` warns
after 10s and accepts anyway, the reply arrives, and the existence check fails as it should.
## Verified
Run as `./test_dev.exe` from `_build/default/test`, serially, whole binary, detecting the flake by
grepping stdout for `never bound`:
- **Before: 2/8 runs failed (25%)** — matches the rate NEXT.md records. Failing runs 24.7s and 24.0s;
passing runs 30.0-36.0s (the 10s stall described above).
- **After: 0/10 runs failed (0%)**, and every run landed in the fast band, 17.0-26.7s — the 10s stall
band is gone as well, which is the independent confirmation that the second race is also closed.
Run counts are small (8 and 10) because the session was cut short for budget; each run is ~25s. A
longer sweep was **not measured**.
`dune test --root .` was run twice after the change. The second run was green. The first run reported
`1 failure(s)` with `FAIL the daemon never listened` — see item 1 below.
## What remains
1. **Decide whether `merged_serve`'s 10s warning path deserves a test.** Nothing currently exercises
lib/dev.ml:2322-2326 (a program that never calls `agent/start`). It was reachable only by accident
here, via the unlink race that is now closed.
## Closed since: the other "the daemon never listened", which was not a race at all
The previous item 1 sent two lanes after a contention race. **There is none**, and the reading it was
built on was wrong twice over:
- The wording is **not** unique to `test_emacs.ml:50` and `test_repl.ml:71`. `test/test_dev.ml:77` —
the *dev* socket check in the same first block this handoff fixed the *agent* socket check in —
emitted exactly `the daemon never listened` as well. Three sites, not two, which is why a sighting
in a full-run log could not be attributed.
- The cause is a **bound that is too short for a build, not a race**. `flan dev` does an llc-and-link
of the whole program *before* it binds its socket, so the await covers a build. Warm and idle that
is ~600ms; under dune's own parallelism it was measured at 6.5s here and at 6.6s and 6.8s by the
lane that fixed it — against a 5000ms await in `test_dev.ml` and 8000ms in the other two. Nothing
about it is occasional; it reproduces whenever the build is cold, and `Build.cachedir` sits under
dune's per-run `TMPDIR`, so the object cache starts empty on **every** `dune test` invocation.
Fixed on another branch: all three awaits now wait a minute, bounded by the existing watchdog rather
than by a guess, and `test_dev.ml` carries a named `listening` helper so the reason is written once
for its thirteen daemons.
Two measurements from this lane that stand whatever else changes: **30 sequential full `dune test`
runs, 0 failures and no `never bound` in any of them** — the `agent.sock` ordering fix above is
confirmed at a sweep wide enough to close item 3 of the old list, so `NEXT.md`'s "One flaky test,
measured rather than suspected" section is deleted. The reproduction that came after it, on the 31st
run, was the *dev* socket at `test_dev.ml:77`, with `built dev-loop.flan in 6525ms` sitting four lines
below the FAIL in the same log — which is what turned the hypothesis into the finding above.
## Tried and rejected
- **Wrapping the check in the file's `await` helper** (`await (fun () -> Sys.file_exists agent_sock)`).
It looks like the obvious fix and it is wrong: it makes the test observe the file at the *earliest*
possible instant, which makes it *more* likely to win the unlink race against `merged_serve` and
more likely to trigger the silent 10s stall. Do not reintroduce it.
- **Lengthening any timeout** — *for the `agent.sock` check*. Not needed there; the ordering was
missing, not the time. This does **not** generalise, and reading it as a rule cost two lanes: the
socket-appears checks above are a wait for a *build*, and there lengthening the bound was exactly
the fix. See the section before this one.
## Environment note for whoever picks this up
Running `_build/default/test/test_dev.exe` directly requires the dune-copied deps
(`test/programs/*.flan`) to be present in `_build`. A bare `dune build --root .` does **not** copy
them — every daemon then dies with `Sys_error("programs/dev-loop.flan: No such file or directory")`
and all eleven blocks report "never listened". Run `dune build --root . @test/runtest` once first to
populate `_build/default/test/programs/`. Also run the loop **strictly serially**: the socket names
under `$TMPDIR` are fixed (`flan-devtest-*.sock`), so parallel runs collide and manufacture failures.

69
NEXT.md
View File

@ -59,9 +59,13 @@ question 4 seriously now that there are two backends that can disagree.
which removes a class of unbalanced-pair bug from every raylib program.
- **`slice-from-ptr`** — queued below. Blocks three raylib examples and leaves the hand-written `Font`
surface with no example caller.
- **`merged_serve`'s 10s warning path** (`lib/dev.ml:2322-2326`), the last item in `HANDOFF-f1.md`.
- **The memcheck half of the registry** — `VALGRIND_MAKE_MEM_UNDEFINED` in `flan_arena_proc`. The
registry answer and the memcheck answer are different tools and must not be blurred.
- ~~**`merged_serve`'s 10s warning path**~~ — tested. `test/programs/dev-noagent.flan` plus the last
block of `test_dev.ml`: the session still answers `describe` after the wait runs out, which is the
policy that distinguishes it from `two_process`. `HANDOFF-f1.md` is gone with it.
- ~~**The memcheck half of the registry**~~ — done. `flan_arena_proc`'s `free-all` issues memcheck's
`MAKE_MEM_UNDEFINED` over the arena, vendored rather than included; it is a control in
`test_valgrind.ml` now, and `BUILT.md` carries the cost. The registry answer and the memcheck
answer stay two claims.
## Not in the freeze, because it is already decided and unblocked
@ -122,8 +126,6 @@ the Emacs side". **Superseded** — the registry lane built that properly. Delet
leaves the hand-written `Font` surface with no example caller.
3. **Generic structs and `$n` array lengths** — queued below, decided, and now unblocked since the
generics lane has merged.
4. `HANDOFF-f1.md` has one item left: whether `merged_serve`'s 10s warning path
(`lib/dev.ml:2322-2326`) deserves a test.
## For siam-farmer
@ -457,7 +459,7 @@ tip themselves. A lane that did not notice would produce plausible work against
the `emacs/test-flan-dev.el` one. `HANDOFF-f2.md` is deleted; the reasoning is in `BUILT.md`, "A breakpoint is a
function call, and the editor only says where".
### From `HANDOFF-f1.md` — the socket flake is fixed, and two things follow it
### ~~From `HANDOFF-f1.md`~~ — the socket flake is fixed, and everything that followed it is done
The fix is ordering, not timing: the test checked for `agent.sock` before completing any round-trip, and
`merged_serve` does not accept until its own 10s await on that path. Moving the existing `describe` request ahead of
@ -492,8 +494,18 @@ those two carry that wording was false and sent one lane after the wrong file.
**The socket ordering fix is confirmed** at 30 sequential full `dune test` runs, 0 failures, no `never bound` in any
log — so the section this file used to carry about it is gone.
1. **Decide whether `merged_serve`'s 10s warning path deserves a test.** `lib/dev.ml:2322-2326` is exercised by
nothing now that the unlink race is closed.
**It deserved a test, and has one; `HANDOFF-f1.md` is deleted.** The reason is not the log line, it is the policy
behind it: a program that never calls `agent/start` is the one condition on which the two shapes of `flan dev`
deliberately disagree — `two_process` kills its child and `failwith`s, `merged_serve` warns and serves anyway,
because the thing it would have to kill is itself. Nothing held that second answer in place, and copying the
daemon's answer back into the merged path is the obvious tidy-up: it would turn every agentless program into a
session that dies at startup, with no test to notice. `test/programs/dev-noagent.flan` and the last block of
`test_dev.ml` assert the session still answers `describe` after the wait runs out, with the warning text as the
secondary evidence that this is the branch that produced it.
The block costs the full ten seconds and there is no way to spend less — `accept_loop` is not reached until the
await expires, so no reply can arrive sooner. A timeout override in `lib/dev.ml` existing only for the test was the
alternative and is a worse trade.
**The `never listened` message named the wrong thing, and now it names which.** One timer covered two waits — a
build, then a bind — so the message was a wrong diagnosis, which costs more than no message. `listening` in
@ -627,12 +639,12 @@ Items 1 to 5 are built and the test is written. `BUILT.md`'s *"An address answer
table, the note and the inspector's pointer arm; *"An address you have in your hand"* is the account of the reader —
the address root, the breakdown, the leak report, and what "at exit" turned out to mean.
**Left:** **the memcheck half of item 6, and nothing else.** The registry knows an arena's `free-all` killed
everything in the region, so a later read through a pointer into it is *answerable*. Memcheck still says nothing,
because nothing told it: the pages stay mapped and `free-all` is an integer going to zero inside one allocation.
Closing that is `VALGRIND_MAKE_MEM_UNDEFINED` in `flan_arena_proc`, which `test/test_valgrind.ml` already names.
**The two must not be blurred** — the registry answer and the memcheck answer are different tools reaching different
people, and building one is not progress on the other.
**Left: nothing.** The memcheck half of item 6 is written. The registry knows an arena's `free-all` killed
everything in the region, so a later read through a pointer into it is *answerable*; memcheck is now told the same
fact on the next line, by a vendored `MAKE_MEM_UNDEFINED` over `[base, cap)`, so the same read is *reported*.
**The two are still not blurred** — the registry answer and the memcheck answer are different tools reaching
different people, and building one was not progress on the other; they were built separately and neither is evidence
for the other.
**What it does not cover, and does not need to:** stack locals and globals, which the shadow stack and the static type
table already answer by name. A stack address is deliberately not in the table, and a pointer to one still renders
@ -956,8 +968,9 @@ it *would* have written.
of program for a clamp. `escaped[ESCAPE_MAX]` was already covered, because `println.flan`
drives a 1100-character string through it on purpose — 1019 bytes out against a worst case of 1021 into 1024.
`scratch[SCRATCH]` never sees more than 20 characters of 64.
3. **Valgrind over the headless corpus, done.** `dune build --root . @valgrind` runs forty-seven programs under
memcheck, twelve of them again with `--no-bounds-checks`, in 88 seconds including the compiles. Clean. It needs no
3. **Valgrind over the headless corpus, done.** `dune build --root . @valgrind` runs forty-nine programs under
memcheck, twelve of them again with `--no-bounds-checks`, in 91 seconds including the compiles — on a warm object
cache; 162s was measured on a cold one, which is the compiles and not the sweep. Clean. It needs no
instrumentation at all — memcheck works on the binary, so `Emit`'s hand-written IR arrives on the same footing as
clang's C, which is why it was reachable where MSan was not. The uninitialised read ASan is blind to is now a
control that must report: index 3 of a `Vec` with len 2 and cap 4, with `--track-origins` naming the
@ -965,14 +978,22 @@ it *would* have written.
holds **no suppressions** — nothing false came up to suppress. Details, and the measured fact that memcheck catches
0 of `bounds.flan`'s 6 cases where ASan catches 3, in [`BUILT.md`](BUILT.md).
**The hole it leaves, and it is the arena.** `free-all` is retain-capacity, so the pages stay and memcheck is never
told the storage died: round two of a reset arena reads a byte it never wrote, prints round one's value, and
nothing reports. Interior overruns are invisible for the same structural reason — an arena is one `malloc`, and the
`Map`'s `keys | values | hashes | scratch` is one allocation too, so "probe overrun at high load" is not clean, it
is not observable. Closing the arena half means `VALGRIND_MAKE_MEM_UNDEFINED` in `flan_arena_proc`, a `runtime/`
change nobody has made. And both positive controls had to be written by hand: no corpus program reaches an
observable uninitialised read, so the sweep is a regression net from here rather than an audit that found the
runtime sound.
**The arena half of the hole is closed; the interior half cannot be.** `free-all` is retain-capacity, so the pages
stay and `malloc` never learns the storage died — but memcheck can be told directly, and now is: `flan_arena_proc`
issues a vendored `MAKE_MEM_UNDEFINED` over `[base, cap)` beside its registry call, and round two of a reset arena
reading a byte it never wrote reports where it used to print round one's value in silence. Measured: 0 errors
before, 6 from 4 contexts after, with `--track-origins` naming the client request. It is the sweep's fourth
control. Cost outside valgrind is five no-op instructions plus the request block — 23 on the `free-all` path only,
about 1ns per reset — and the macro is vendored rather than `#include`d because the machine that runs the sweep
has `valgrind` and not `valgrind-devel`, so a guarded include would silently compile to nothing exactly here. See
[`BUILT.md`](BUILT.md).
**Interior overruns remain invisible**, for a structural reason no client request fixes — an arena is one `malloc`,
and the `Map`'s `keys | values | hashes | scratch` is one allocation too, so "probe overrun at high load" is not
clean, it is not observable. And all three positive controls had to be written by hand: no corpus program reaches
an observable uninitialised read, so the sweep is a regression net from here rather than an audit that found the
runtime sound. The corpus stayed clean across the arena change, which is its own small finding — nothing in it
reads stale arena storage.
Two things the sweep structurally cannot cover: raylib and libm are uninstrumented, so the windowed examples are noise;
and a redefinition module is built by `llc` and `ld` rather than clang, so the reload path carries no instrumentation

84
runtime/flan_rt.c
View File

@ -689,6 +689,68 @@ static flan_allocator flan_heap = {
0, 0, 0, 0
};
/* -- Telling memcheck an arena reset happened. -----------------------
*
* One valgrind client request, vendored rather than included. The macro in
* <valgrind/memcheck.h> is two dozen lines of inline asm and two integer
* constants; what an #include buys is those lines, and what it costs is a
* dependency this project has already refused twice over — it shells out to
* clang rather than linking libLLVM, and plan.org rejected libclang.
*
* The decisive argument is not taste, it is measurement: the machine that runs
* `dune build --root . @valgrind` has /usr/bin/valgrind and no
* /usr/include/valgrind. valgrind-devel is a separate package almost nobody
* installs. A guarded #include would therefore compile to nothing on the one
* box where the sweep runs, and the control in test_valgrind.ml that proves
* this works would go quiet with no diagnostic. There is also nowhere to put
* an -I: this file is cat'd into an OCaml string literal (lib/dune,
* runtime_src.ml) and handed to clang in a scratch directory, so the include
* path a build system would supply does not exist here.
*
* The sequence is four rotates of a register whose value they leave unchanged,
* followed by `xchg %rbx,%rbx`. Outside valgrind that is five no-op
* instructions; under it the JIT recognises the preamble and reads the request
* block out of %rax. Nothing is linked, nothing is probed, and a binary built
* this way runs identically with no valgrind on the machine at all.
*
* 0x4d43 is 'M','C' — memcheck's tool base, VG_USERREQ_TOOL_BASE('M','C') —
* and +1 is MAKE_MEM_UNDEFINED, +0 being NOACCESS and +2 DEFINED. Being wrong
* about either number is a silent no-op rather than an error, which is exactly
* why test_valgrind.ml asserts the effect instead of trusting the constant.
*
* The asm is amd64-only and the guard is load-bearing rather than defensive:
* this runtime is also compiled for wasm32-wasi and for emscripten, where the
* block would not assemble. Everywhere but x86-64 the request is a cast to
* void, and memcheck does not run there anyway. */
#if defined(__x86_64__) && !defined(__wasm__)
#define FLAN_VG_PREAMBLE \
"rolq $3, %%rdi ; rolq $13, %%rdi\n\t" \
"rolq $61, %%rdi ; rolq $51, %%rdi\n\t"
#define FLAN_VG_REQUEST(dflt, req, a1, a2, a3, a4, a5) \
__extension__({ \
volatile unsigned long long int _vg_args[6]; \
volatile unsigned long long int _vg_result; \
_vg_args[0] = (unsigned long long int)(req); \
_vg_args[1] = (unsigned long long int)(a1); \
_vg_args[2] = (unsigned long long int)(a2); \
_vg_args[3] = (unsigned long long int)(a3); \
_vg_args[4] = (unsigned long long int)(a4); \
_vg_args[5] = (unsigned long long int)(a5); \
__asm__ volatile(FLAN_VG_PREAMBLE \
/* %rdx = client_request ( %rax ) */ \
"xchgq %%rbx,%%rbx" \
: "=d"(_vg_result) \
: "a"(&_vg_args[0]), "0"(dflt) \
: "cc", "memory"); \
_vg_result; \
})
#define FLAN_VG_MAKE_MEM_UNDEFINED(p, n) \
((void)FLAN_VG_REQUEST(0, 0x4d430000u + 1, (p), (n), 0, 0, 0))
#else
#define FLAN_VG_MAKE_MEM_UNDEFINED(p, n) ((void)(p), (void)(n))
#endif
/* -- The arena: one fixed backing buffer and a bump offset. ----------
*
* `free-all` is retain-capacity: offset = 0, the pages stay. That is an
@ -752,12 +814,24 @@ static void *flan_arena_proc(flan_allocator *a, int32_t mode, void *p,
case FLAN_ALLOC_FREE:
return NULL; /* refused by the capability set above */
case FLAN_ALLOC_FREE_ALL:
/* The hole test_valgrind.ml measures. The pages stay mapped and the bytes
stay readable, so memcheck is told nothing and never will be by this
line; what it does is make the *registry* agree that everything in the
region died, so a later read through a pointer into it is answerable
rather than silent. */
/* Two tools told the same fact, and they are not interchangeable. The
registry is the answer for a person at an editor: it makes the *names*
agree that everything in the region died, so a later read through a
pointer into it is answerable rather than silent.
The client request is the answer for memcheck, and it closes the hole
test_valgrind.ml used to only measure. The pages stay mapped and the
bytes stay readable — free-all is retain-capacity and an arena is one
malloc — so nothing about the *addresses* changes and memcheck would
otherwise never learn the storage died. Marking the region undefined
resets its definedness bits, and round two reading a byte it never
wrote now reports instead of quietly printing round one's value.
The whole capacity rather than [0, offset): everything past the offset
is equally reusable and equally stale, and two calls would only be
cheaper if the second could be skipped. */
flan_dev_reg_dead_range(ar->base, ar->cap);
FLAN_VG_MAKE_MEM_UNDEFINED(ar->base, ar->cap);
ar->offset = 0;
a->live_blocks = 0;
a->live_bytes = 0;

15
test/programs/dev-noagent.flan Normal file
View File

@ -0,0 +1,15 @@
;;;; A program [flan dev] can host that never calls (agent/start ...), which
;;;; is the one condition the merged session and the two-process daemon answer
;;;; differently: [two_process] kills the child and fails, [merged_serve]
;;;; warns and serves anyway. See test_dev.ml's last block.
;;;;
;;;; No (import agent ...) at all, because the point is a program that has not
;;;; been told about the agent rather than one that forgot a call. It prints
;;;; and returns: a Flan main that returns under [flan dev] parks instead of
;;;; ending the process (flan_merged_exit), so the session outlives it and the
;;;; accept loop keeps answering.
(defn step [] i64 7)
(defn main [] i32
(print (step)) (println "")
0)

77
test/test_dev.ml
View File

@ -2725,6 +2725,83 @@ let () =
(try ignore (Unix.waitpid [] tpid) with Unix.Unix_error _ -> ());
List.iter (fun f -> try Sys.remove f with Sys_error _ -> ()) [ tsock; tout ];
(* A program that never calls [agent/start], which is the one condition on
which the two shapes of [flan dev] deliberately disagree. [two_process]
kills its child and [failwith]s: the program is a separate process, the
daemon owns it, and a daemon with nothing to deliver to is useless.
[merged_serve] prints a warning and serves anyway, because the thing it
would have to kill is itself — an editor connected to it still deserves
[describe], [defs] and the program's output, and only a *delivery*
needs the agent.
That second policy was held up by nothing at all. Nothing in the suite
reached lib/dev.ml's warning branch, and the shape of the mistake it
guards against is a small one: copying the daemon's answer back into
the merged path is the obvious tidy-up, and it would turn every program
without an agent into a session that dies at startup, silently, because
no test would have noticed.
So what is asserted is the policy and not the sentence: the session is
still answering after the wait ran out. The warning text is checked
second, as the evidence that this is the branch that produced it and
not some other path that happened to work.
The block costs the full ten seconds of [merged_serve]'s [await]
([lib/dev.ml]) and there is no way to spend less: [accept_loop] is not
reached until the wait expires, so the reply cannot arrive sooner.
Shortening it would mean a timeout override in [lib/dev.ml] that exists
for the test and for nothing else, which is a worse trade than ten
seconds in a suite that already takes minutes. *)
let nsock = tmp "noagent.sock" and nlog = tmp "noagent.log" in
(try Sys.remove nsock with Sys_error _ -> ());
(* Its own stderr, unlike every other daemon here: the warning is the
evidence and it is written there. *)
let nfd =
Unix.openfile nlog [ Unix.O_WRONLY; Unix.O_CREAT; Unix.O_TRUNC ] 0o600
in
let npid =
Unix.create_process flan
[| flan; "dev"; "programs/dev-noagent.flan"; "-s"; nsock |]
Unix.stdin nfd nfd
in
Unix.close nfd;
if not (listening ~pid:npid nsock) then
fail "the agentless daemon %s" !listen_why
else begin
let nc = connect nsock in
(* Blocks for the whole of [merged_serve]'s wait, by construction. The
exception arm is not defensive: a session that adopted the daemon's
policy would exit here, and the connection would come back ECONNRESET
rather than with a status. Reported by name because an uncaught
[Unix_error] out of a test binary says nothing about which test. *)
(match Wire.parse (Wire.send nc "(:op \"describe\")"; Wire.recv nc) with
| r when status r = "ok" -> ()
| r ->
fail "a program without (agent/start ...) was not served: describe: %s"
(status r)
| exception e ->
fail
"a program without (agent/start ...) ended the session instead of \
drawing a warning: %s" (Printexc.to_string e));
(try
ignore (Wire.send nc "(:op \"close\")");
ignore (Wire.recv nc)
with _ -> ());
(try Unix.close nc with Unix.Unix_error _ -> ())
end;
(try Unix.kill npid Sys.sigkill with Unix.Unix_error _ -> ());
(try ignore (Unix.waitpid [] npid) with Unix.Unix_error _ -> ());
let nlog_text =
try In_channel.with_open_bin nlog In_channel.input_all
with Sys_error _ -> ""
in
if not (contains_sub nlog_text "does it call (agent/start ...)?") then
fail
"a program without (agent/start ...) drew no warning from flan dev:\n%s"
nlog_text;
List.iter (fun f -> try Sys.remove f with Sys_error _ -> ())
[ nsock; nlog ];
List.iter (fun f -> try Sys.remove f with Sys_error _ -> ())
[ sock; out; bsock; bout ];
if !failures = 0 then print_endline "dev: all tests passed"

68
test/test_valgrind.ml
View File

@ -353,21 +353,30 @@ let padded_key =
\ (free m))\n\
\ 0)\n"
(* Not a control but a measurement, and the ceiling on everything above.
(* The control that used to be a measurement, and the reason this paragraph is
worth reading twice.
Round one writes four elements into arena storage; free-all resets the
offset and keeps the pages; round two allocates the same bytes back and
reads one it never wrote. The value printed is round one's — and memcheck
says nothing, because the definedness bits round one set are still on those
bytes. Nothing told memcheck the storage died, because from malloc's point
of view it did not: an arena is one allocation and free-all is an integer
going to zero inside it.
reads one it never wrote. The value printed is still round one's — nothing
about the *program* changed — but memcheck now reports the read, and
--track-origins names flan_arena_proc under flan_alloc_free_all as where
the undefined bytes came from.
So the uninitialised-read coverage this file's headline control
demonstrates holds for the heap allocator and *not* for arena storage
reused after a free-all, which is the per-frame pattern the arena exists
for. Closing it means VALGRIND_MAKE_MEM_UNDEFINED in flan_arena_proc, which
is a runtime change and a different lane's. It is printed rather than
asserted because it is a fact about the tool, not a regression. *)
Until flan_rt.c grew its FLAN_VG_MAKE_MEM_UNDEFINED, this file printed a
note here instead of asserting anything, because there was nothing to
assert: from malloc's point of view the storage never died — an arena is
one allocation and free-all is an integer going to zero inside it — so the
definedness bits round one set were still on those bytes and memcheck was
silent. Measured on this machine before the change: ERROR SUMMARY 0. After:
6 errors from 4 contexts, the origin line naming the client request.
So the uninitialised-read coverage [heap_uninit] demonstrates for the heap
allocator now holds for arena storage reused after a free-all too, which is
the per-frame pattern the arena exists for. If this control stops
reporting, the client request has been dropped, mis-numbered, or compiled
out by the architecture guard — all three of which are silent by
construction, which is why the effect is asserted and not the constant. *)
let arena_reuse =
"(defvar frame Allocator)\n\
(defn main [] i32\n\
@ -383,31 +392,6 @@ let arena_reuse =
\ (let [t (as-slice w)] (print (at t 3)) (println \"\"))))\n\
\ 0)\n"
let note_arena_reuse () =
let path = Filename.concat scratch "flan-vg-arena.flan" in
Out_channel.with_open_bin path (fun ch ->
Out_channel.output_string ch arena_reuse);
(match compile ~checks:false path with
| exception Failure m -> fail "arena reuse note: build: %s" m
| exe ->
let _, text, log = run ~vg:true exe [] in
(match summary log with
| Some (0, _) ->
Printf.printf
"note arena storage reused after free-all is not re-poisoned: the \
second round read a byte it never wrote, printed %S, and memcheck \
was silent. This is the ceiling on the uninitialised-read coverage \
below — it holds for the heap allocator and not for an arena.\n"
(String.trim text)
| Some (n, _) ->
Printf.printf
"note arena storage reused after free-all now reports (%d): \
something started telling memcheck the region died. Good news; \
this file and BUILT.md both say it does not.\n" n
| None -> fail "arena reuse note: no ERROR SUMMARY\n%s" log);
(try Sys.remove exe with Sys_error _ -> ()));
(try Sys.remove path with Sys_error _ -> ())
let () =
match Sys.command "command -v valgrind > /dev/null 2>&1" with
| 0 when Sys.command "command -v clang > /dev/null 2>&1" = 0 ->
@ -426,7 +410,15 @@ let () =
field by field, so the holes in it are being read. See maps.flan \
comment (2) and flan_key_hash_flat in flan_rt.c: the flat path is \
bytewise and a struct key must not reach it.";
note_arena_reuse ();
control ~expect:true "flan-vg-ctl-arena" arena_reuse
~why:"Arena storage reused after a free-all is not being re-poisoned, \
so a read of a byte the current round never wrote goes unreported \
and prints the previous round's value instead. That was the state \
of the world before flan_rt.c grew FLAN_VG_MAKE_MEM_UNDEFINED in \
flan_arena_proc's FLAN_ALLOC_FREE_ALL; if this is quiet again the \
request has been dropped, mis-numbered, or compiled out by the \
#if defined(__x86_64__) guard. All three are silent, which is why \
this asserts the effect and not the constant.";
List.iter (fun (p, a) -> check "checked" p a ~checks:true) corpus;
List.iter (fun (p, a) -> check "unchecked" p a ~checks:false)
unchecked_subset;

19
test/valgrind.supp
View File

@ -1,7 +1,7 @@
# Memcheck suppressions for the Flan corpus. See test/test_valgrind.ml.
#
# This file is empty of suppressions, and that is a finding rather than an
# oversight. The sweep was run over forty-seven programs with
# oversight. The sweep was run over forty-nine programs with
# --gen-suppressions=all before this file existed, checked and again with
# --no-bounds-checks, and memcheck produced nothing to suppress: no false
# positives, and no true ones either. Every entry below the line would have
@ -17,9 +17,20 @@
# the offset — is one allocation to memcheck. It never learns that a
# sub-object ended, so it has nothing to complain about and equally
# nothing to catch. Recorded as a coverage ceiling in BUILT.md, not as a
# clean bill of health. A read of arena bytes never written *is* caught,
# by definedness rather than addressability; a read of bytes a previous
# round wrote before a free-all is not.
# clean bill of health.
#
# What *is* caught is definedness rather than addressability, and that
# half now covers the arena too: flan_arena_proc's free-all issues
# memcheck's MAKE_MEM_UNDEFINED over the whole capacity, so a read of
# bytes a previous round wrote before the reset reports. test_valgrind.ml
# asserts it as a control — it produced nothing before the runtime change
# and six errors after. The corpus stayed clean across that change, and
# the sharp end of that is stale-region.flan, map-stale-region.flan and
# pool-stale-region.flan: all three read through a pointer into an arena
# that has been reset, all three are now reading bytes memcheck knows are
# undefined, and none of them reports — because the epoch trap fires
# first. The runtime's own guard beats the read. Interior overruns are
# still invisible, for the structural reason above.
#
# 2. Hand-written LLVM IR. Expected to confuse the tool. It does not, and it
# could not: memcheck instruments the binary, so it never sees IR, never