From a615e4ecfcf0167061999f20b0be3b8bb8ce0c03 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Fri, 25 Sep 2026 15:38:02 +0700 Subject: [PATCH] The leak sweeps have switches, a C source that includes the dyn header is recompiled when the header changes, and the map-remove and map-keys programs free what they allocate --- TODO.org | 12 +++++++----- lib/build.ml | 14 +++++++++++++- test/dune | 6 +++++- test/programs/map-keys.flan | 3 ++- test/programs/map-remove.flan | 3 ++- test/test_acceptance.ml | 2 +- test/test_valgrind.ml | 23 +++++++++++++++-------- 7 files changed, 45 insertions(+), 18 deletions(-) diff --git a/TODO.org b/TODO.org index e096ce4a..7e22ee01 100644 --- a/TODO.org +++ b/TODO.org @@ -1431,12 +1431,14 @@ through a pointer into it is answerable; memcheck is told the same fact, so the same read is reported. The two stay two claims — different tools reaching different people. -** NEXT The leak question across the corpus +** WAIT The leak question across the corpus Decided 2026-09-25: one pass over the whole corpus with LeakSanitizer and memcheck's leak check on. Memory an allocator holds by design is set aside; memory nothing owns is a leak and is fixed. The sweeps' default stays leak-checking off. -Both sweeps run with leak checking off, because allocate-once-never-free is this -runtime's design and a leak check produces a suppression list. A green sweep -therefore says nothing about who frees the newly allocating =(bytes s)=. Worth -asking on purpose one day, across the whole corpus and not one program. +WAIT on the between-batches sweep slot: the switches are +=ASAN_OPTIONS=detect_leaks=1 dune build @sanitize= and =FLAN_LEAKS=1 dune build @valgrind=. +A 22-program LSan sample found no runtime leak; program leaks in map-remove and +map-keys are fixed. Needs a decision: =(bytes s)= and =(clone slice)= with no +allocator answer a =[T]= over a heap block nothing can free (bytes-copy.flan). +Temp-allocator by default, as =i64->bytes= is, or a =(Vec T)= the caller frees? ** DONE trap_oom has no site CLOSED: [2026-09-25] diff --git a/lib/build.ml b/lib/build.ml index 70e94449..9b25b1d9 100644 --- a/lib/build.ml +++ b/lib/build.ml @@ -690,11 +690,23 @@ let compile_c ~opts ?tflags ?(warn = []) ~src ~name () = built against, so repointing either must not serve a stale .o. *) let tflags = match tflags with Some f -> f | None -> target_flags opts in let cc = compiler opts in + (* A source that includes the dyn header was compiled against it, so the + header is part of what the object depends on. Without it a change to a + struct the header declares served an object built against the old layout + — test/dyn_ops.c read a descriptor's new fields past the end of its own. *) + let header = + let needle = "flan_dyn.h" in + let n = String.length needle and m = String.length src in + let rec has i = + i + n <= m && (String.sub src i n = needle || has (i + 1)) + in + if has 0 then Runtime_src.dyn_header else "" + in let key = Digest.to_hex (Digest.string (String.concat "\000" - [ name; src; stamp_of cc; opts.opt; + [ name; src; header; stamp_of cc; opts.opt; String.concat " " (cflags opts); String.concat " " tflags; String.concat " " warn ])) diff --git a/test/dune b/test/dune index 6fa1957b..e5fba609 100644 --- a/test/dune +++ b/test/dune @@ -118,6 +118,8 @@ (deps (alias corpus) test_sanitize.exe + ; ASAN_OPTIONS=detect_leaks=1 asks the leak question; see test_sanitize.ml. + (env_var ASAN_OPTIONS) ; The dyn runtime's C main, which is the one thing in this sweep that is not ; a Flan program: flan_dyn.c has no Flan spelling yet. It is also the one ; translation unit here that frees the most, which is what makes it worth a @@ -149,7 +151,9 @@ test_valgrind.exe ; The suppression file, which is all reasons and no suppressions; its own ; header says why that is the finding rather than an oversight. - (file valgrind.supp)) + (file valgrind.supp) + ; FLAN_LEAKS=1 turns memcheck's leak check on; see test_valgrind.ml. + (env_var FLAN_LEAKS)) (action (run ./test_valgrind.exe))) ; The corpus a fourth time, through the hand-written x86-64 backend, compared diff --git a/test/programs/map-keys.flan b/test/programs/map-keys.flan index 7e9baf81..c204325f 100644 --- a/test/programs/map-keys.flan +++ b/test/programs/map-keys.flan @@ -22,7 +22,8 @@ (dotimes [i (length vs)] (print (at vs i)) (print " ")) (println "") (free ks) - (free vs))) + (free vs)) + (free m)) ;; A string key, and a map that never allocated. (let [names (map-new string i32) none (map-new string i32)] diff --git a/test/programs/map-remove.flan b/test/programs/map-remove.flan index 349eafd2..cecaee24 100644 --- a/test/programs/map-remove.flan +++ b/test/programs/map-remove.flan @@ -123,7 +123,8 @@ (print (length t)) (println "") ; 150 (match (get t 299) (Some v) (do (print v) (println "")) None (println "?")) ; 598 (print (has-key? t 298)) (println ""))) ; false - (free-all ar)) + (free-all ar) + (arena-destroy ar)) ;; (7) Churn at a steady size, checked against a plain array. Keys are drawn ;; from 512 and the map holds about two thirds of them, so removals leave diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 1a47a9f6..34242856 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -5659,7 +5659,7 @@ level "1" (List.filter (fun line -> contains line prefix - && contains line "constant { i64, i64, ptr, i64, ptr, i64, ptr }") + && contains line "constant { i64, i64, ptr, i64, ptr, i64, ptr, i64, ptr }") (String.split_on_char '\n' ir)) in if n <> want then begin diff --git a/test/test_valgrind.ml b/test/test_valgrind.ml index 7d95d908..3c09d02a 100644 --- a/test/test_valgrind.ml +++ b/test/test_valgrind.ml @@ -41,15 +41,22 @@ let fail fmt = Test_support.fail fmt let scratch = Test_support.scratch let supp = "valgrind.supp" -(* Leak checking is off, and the reason is [test_sanitize.ml]'s reason for - detect_leaks=0 unchanged: allocate-once-never-free is this runtime's design, - not an accident — rt_args says so in its own comment, an arena hands back - nothing before arena-destroy, and the context temp arena is made on first - use and never released. LeakSanitizer produced a suppression list and no - information; memcheck would produce the same list. The question is worth - asking on purpose one day, and this is not that run. *) +(* Leak checking is off by default, and the reason is [test_sanitize.ml]'s + reason for detect_leaks=0 unchanged: allocate-once-never-free is this + runtime's design — rt_args says so in its own comment, and the context + temp arena is made on first use and never released. FLAN_LEAKS=1 asks the + leak question on purpose: definite leaks only, which is memory nothing + points at any more, counted as errors. What an allocator holds by design is + still reachable and is not reported. The sanitize sweep's switch is + ASAN_OPTIONS=detect_leaks=1. *) +let leaks = Sys.getenv_opt "FLAN_LEAKS" = Some "1" + let vg_flags = - [ "--leak-check=no"; "--error-exitcode=0"; "--track-origins=yes"; + (if leaks then + [ "--leak-check=full"; "--show-leak-kinds=definite"; + "--errors-for-leak-kinds=definite" ] + else [ "--leak-check=no" ]) + @ [ "--error-exitcode=0"; "--track-origins=yes"; (* Origins are what turn "uninitialised value" into a line naming the allocation it came from. They cost roughly 2x on top of memcheck and are worth every bit of it: without them an uninitialised-read report