diff --git a/BUILT.md b/BUILT.md index 2a14e59..76de99a 100644 --- a/BUILT.md +++ b/BUILT.md @@ -795,8 +795,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. @@ -831,13 +832,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 | @@ -4824,16 +4842,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 diff --git a/HANDOFF-f1.md b/HANDOFF-f1.md deleted file mode 100644 index 952b802..0000000 --- a/HANDOFF-f1.md +++ /dev/null @@ -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. diff --git a/NEXT.md b/NEXT.md index 50670e0..5c2e39b 100644 --- a/NEXT.md +++ b/NEXT.md @@ -80,9 +80,13 @@ question 4 seriously now that there are two backends that can disagree. - ~~**`slice-from-ptr`**~~ — **built**. `examples/text-rectangle-bounds.flan` is the port it unblocked, and the first example to call the hand-written `Font` surface. The other two it named — `text_codepoints_loading` and `textures_image_processing` — are now ordinary porting work. -- **`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 @@ -157,8 +161,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 @@ -524,7 +526,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 @@ -559,8 +561,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 @@ -694,12 +706,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 @@ -1023,8 +1035,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 @@ -1032,14 +1045,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 diff --git a/runtime/flan_rt.c b/runtime/flan_rt.c index 8aec1fe..eeaa806 100644 --- a/runtime/flan_rt.c +++ b/runtime/flan_rt.c @@ -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 + * 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; diff --git a/test/programs/dev-noagent.flan b/test/programs/dev-noagent.flan new file mode 100644 index 0000000..df9fedc --- /dev/null +++ b/test/programs/dev-noagent.flan @@ -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) diff --git a/test/test_dev.ml b/test/test_dev.ml index 7ed5f32..d81d604 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -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" diff --git a/test/test_valgrind.ml b/test/test_valgrind.ml index 3c9d59e..2983f17 100644 --- a/test/test_valgrind.ml +++ b/test/test_valgrind.ml @@ -354,21 +354,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\ @@ -384,31 +393,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 -> @@ -427,7 +411,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; diff --git a/test/valgrind.supp b/test/valgrind.supp index 07d060c..573e44e 100644 --- a/test/valgrind.supp +++ b/test/valgrind.supp @@ -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