tests: build weak tables in a function that returns before collecting

[dg1sbg]

Fixes the WEAK-KEY-AND-VALUE-WEAKNESS flake tracked in #1814. Supersedes #1815 (closed) — same diagnosis, but with a reproduction that PR did not have, and without the retry loop, which turns out to be unnecessary.

Why this is a test bug, not a Clasp bug

The tests allocate throwaway (list nil) temporaries in the same frame that then calls garbage-collect, and assert an exact hash-table-count. Boehm scans the stack conservatively, so a stale word left in that frame or in a register can still reference a temporary and keep its weak entry alive.

That is the defining trade-off of conservative collection: it guarantees it will never collect live data, not that it will always collect dead data. The comment already above these tests concedes as much:

;;; These tests are pretty strict - they want the garbage to actually be
;;; collected by that garbage-collect call, which may not be the case if we
;;; ever get a more relaxed collector (generational or something)

Three things say stack artifact rather than a weak-table defect:

• the count is off by exactly one, never more — a single cons retained
• the rate tracks stack depth and nothing else
• the entry is collected by a later GC; a real leak would fail every time

The only genuine Clasp-side fix would be precise stack scanning (shadow stacks or stack maps for every C++ and JIT frame), which Clasp has deliberately not adopted — boehmprecise refers to precise heap layout descriptors, not precise stack scanning.

Reproduction

Call the test body directly, under recursion, and iterate. Roughly a minute; no special environment.

weakprobe.lisp — establishes the rate and its dependence on stack depth

(defun trial ()
  (let ((table (make-hash-table :weakness :key-and-value)))
    (setf (gethash (list nil) table) :value
          (gethash :key table) (list nil)
          (gethash (list nil) table) (list nil)
          (gethash :key table) :value)
    (gctools:garbage-collect)
    (hash-table-count table)))

(defun deep-trial (depth)
  (if (<= depth 0)
      (trial)
      (let ((pad (list depth depth depth)))
        (declare (ignorable pad))
        (deep-trial (1- depth)))))

(defun run (label n depth)
  (let ((bad 0) (worst 1))
    (dotimes (i n)
      (let ((c (if (plusp depth) (deep-trial depth) (trial))))
        (unless (eql c 1) (incf bad) (when (> c worst) (setf worst c)))))
    (format t "~&RESULT ~a n=~a depth=~a bad=~a rate=~,4f worst-count=~a~%"
            label n depth bad (/ (float bad) n) worst)))

(run "shallow" 2000 0)
(run "deep50"  2000 50)
(run "deep500" 2000 500)
(core:quit)

x86-64 Linux, LLVM 18, boehmprecise, --build-mode=bytecode, 2000 trials per condition:

stack depth	failure rate
shallow	1.30 %
50 frames	0.05 %
500 frames	16.6 %

worst-count is 2 in every failure — one cons falsely retained, never more.

The rate is build-dependent. It comes from where stale words happen to land in registers and stack slots, so code layout moves it. The same commit rebuilt gave 5.4% instead of 16.5%, and another build 0.1%. Treat the numbers as a range (0.1 %–16.5 % at depth 500), not a constant — which is also why a handful of full-suite runs cannot establish absence: at ~1 % per run, twenty clean runs is the expected outcome of a measurement too weak to see it.

Why the retry from #1815 is not needed

#1815 did two things: had the builder return before collecting, and retried the collection up to ten times. Only the first removes the artifact; the second relaxes what is asserted, from "collected in one GC" to "collected within ten". Measured separately:

weak3.lisp — same-frame vs builder-only vs builder+retry

(defun same-frame ()
  (let ((table (make-hash-table :weakness :key-and-value)))
    (setf (gethash (list nil) table) :value
          (gethash :key table) (list nil)
          (gethash (list nil) table) (list nil)
          (gethash :key table) :value)
    (gctools:garbage-collect)
    (hash-table-count table)))

(defun build-table ()                      ; frame popped on return
  (let ((table (make-hash-table :weakness :key-and-value)))
    (setf (gethash (list nil) table) :value
          (gethash :key table) (list nil)
          (gethash (list nil) table) (list nil)
          (gethash :key table) :value)
    table))

(defun builder-only ()
  (let ((table (build-table)))
    (gctools:garbage-collect)
    (hash-table-count table)))

(defun builder-plus-retry (&optional (tries 10))
  (let ((table (build-table)))
    (loop repeat tries
          do (gctools:garbage-collect)
          when (eql (hash-table-count table) 1) return 1
          finally (return (hash-table-count table)))))

1000 trials per arm at depth 500:

form	5.4 % build	0.1 % build
current (same frame)	54 / 1000	1 / 1000
builder only	0 / 1000	0 / 1000
builder + retry	0 / 1000	0 / 1000

Builder-only is 0 failures in 2000 trials where roughly 55 would be expected. So the retry buys nothing measurable, and this PR omits it — the test asserts exactly what it does today, one collection and an exact count. It simply stops manufacturing the stale reference itself.

Non-vacuity

A fix that made the test unfailable would be worse than the flake. Pinning both key and value in globals — :key-and-value weakness drops an entry if either side dies, so pinning only the key proves nothing, which is a mistake I made on the first attempt at this control:

pinned, builder-only    -> count 2  (still fails, as it must)
pinned, builder + retry -> count 2  (retry does not mask it either)
unpinned                -> count 1  (normal case still reaches the expected count)

Verification

Full build clean; regression suite run three times: 1963 successes, TEST_EXIT=0, Passed WEAK-KEY-AND-VALUE-WEAKNESS each time.

Context

This one test was the sole failure in nine Linux CI jobs across six unrelated PRs in a single day — PRs touching koga scripts, print-object methods, numeric compiler macros and CLOS dispatch guards, none of them near weak tables or the GC. It is also not confined to ubuntu-latest/bytecode as #1814's title suggests: it appears on native too, and on both clasp/ and cando/ variants, which is what you would expect of a stack-scanning artifact.