refcount linter - #1059
Conversation
Prompted by ~dozreg-toplud's refcount linter, which flagged leaks in _check, plus a hand audit of the surrounding code. The fixes fall into four groups. _check helper: * take [meta data] as two borrowed nouns instead of consing a cell at every call site (the cell leaked, and it silently assumed ownership of the borrowed sample legs); drop the leaked u3qa_dec result * stop punning raw c3_d/c3_w values as nouns: the shape product was truncated through a u3_atom and compared against a noun, so any ray with 2^31 or more elements failed _check spuriously (and a crafted overflowing shape could pass it) * compute the product with 64-bit overflow detection; an overflowed product cannot match the block count of a real atom, mirroring the Hoon exactly * validate the bloq before calling u3r_met, which is UB at or above block size 37 (release builds compile the guard out); at such sizes any nonzero atom is one block * bail %exit only where +check itself crashes (cell dims, improper shape list, zero data underflowing +dec) wrapper/Hoon divergences (jet computed where the Nock crashes, or vice versa, or produced a different noun): * transpose/diag passed the whole core to _check, reading the gate's battery as ray metadata -- those jets unconditionally bailed %exit whenever they fired * the elementwise (+add/+sub/+mul/+div/+mod), comparison (+gth/+gte/+lth/+lte), scalar (+add-scalar &c), +abs, and +trace wrappers never called _check at all, though the Hoon asserts it * +ravel has no +check and never reads the kind, so its wrapper now punts (not bails) on an inconsistent ray * +dot only asserts equal shapes outside the %i754 path; the full meta equality and consistency checks moved inside the %i754 case * +mmul never compared the two rays' bloq/kind, and its gemm switch silently returned a zero matrix for an out-of-range bloq; it now punts on both * reduction results (+cumsum/+min/+max/+dot) hard-coded shapes ~[1] or ~[1 1] or ~[n 1]; +scalar-to-ray gives an all-ones shape of the input's rank * _set_rounding returns c3n on an unrecognized mode and callers punt, instead of bailing %fail kernel bugs (wrong results, previously unobservable because the wrapper bailed or untested): * trace computed (dot d d) -- the sum of SQUARES of the diagonal -- instead of (cumsum (diag a)); identity-matrix tests masked it * transpose read and wrote with rows/columns swapped, correct only for square inputs, and the wrapper did not swap the result shape * argmin/argmax returned (len - i - 1) instead of the ravel index i * diag and abs return elements in natural order, in lockstep with urbit/urbit#7388, which removes the erroneous flops in +diag and +el-wise-op (the old +abs jet already returned natural order, so jetted and unjetted ships disagreed about +abs; the diag jet never returned at all) * trace honors the door rounding mode; linspace fences an indirect count atom before using it as a raw integer * mod (ray and scalar) rounded the quotient with a hardcoded round-toward-zero; +toi rounds in the door mode (7 mod 2 is -1 under %n, not 1). It also returned values where the Hoon crashes: a non-finite quotient (zero or NaN divisor) is (need ~). And mod-scalar/div-scalar multiplied by a rounded 1/n instead of dividing -- wrong even for exact quotients (21 mod 7 gave 7, 21/7 gave 2.9999998). All four now divide directly, round in softfloat_roundingMode, and bail %exit on a non-finite quotient * gth/gte used SoftBLAS's f_gt/f_ge macros, which are (!le)/(!lt) and thus TRUE whenever either operand is NaN; IEEE gt/ge are false. Same disease via f_min/f_max in min/max/argmax. All now use lt/le with swapped operands, which also reproduces the Hoon reel-fold exactly (NaN in the head is sticky, interior NaN is skipped, first-of-ties wins) * range counted elements with one-shot ceil((b-a)/d), but the Hoon iterates x+d in the door mode, testing each sum against b -- for d=.0.1 the shapes disagreed outright (10 vs 11). The kernel now replicates the iteration (values are the accumulated sums) and the wrapper takes the count from the result's block count; it punts on non-finite bounds/step, a zero step, or a stalled accumulator, where the Nock loops forever leaks: * the _check cell at every call site; u3qa_dec inside _check; _get_dims arrays in the diag/dot wrappers and trace kernel (plus raw c3_d dimensions used directly as nouns there); argmin/argmax scan buffers; linspace/range result-shape overkeeps Verified on a fresh fake ship against the base lagoon (with the urbit/urbit#7388 fixes applied), differentially against a de-jetted copy of the same library: 36/36 probes agree, including transpose of non-square, trace, diag, reduction shapes at rank 1, argmin/argmax with NaN at and after the head, mod under %n/%u, mod-scalar and div-scalar on exact quotients, gth/gte/lth against NaN, range with d=.0.1 and a negative step, and add/cumsum/mod-scalar/range under non-default rounding modes; zero-divisor mod crashes on both doors. A 40x40 %i754 mmul runs ~3s jetted vs ~25s interpreted, confirming the jets fire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Notes from core blitz:
I'll describe refcount annotation syntax here:
/* @Refcount: retain
* (header comment)
*/
u3_noun
foo(u3_noun a)
{ ... }
u3_noun bar(u3_noun u3_noun); // @Refcount: transfer (same line for declarations is OK)
if ( condition ) { // @Refcount: assert transfer (this assignement consumes)
*ptr_u = u3k(u3h(list));
}
// @Refcount: assert custom file
// @Refcount: X
// @Refcount: Y
// @Refcount: Z
|
e44669e to
8abe4f3
Compare
43d0e40 to
ff5b07d
Compare
|
extended the checks to pkg/vere, added more kinds of annotations. moar fixes (these are the important ones, others less so):
and some other leak fixes |
Contains refcount fixes found in #1059 separately, so that #1059 only contains only refcount annotations. Depends on #1079 --- List of fixes: - `by_all`/`by_any` jets slammed the gate while keeping uncounted references to `l_a` and `r_a` - `skid`/`skim`/`skip` held uncounted reference to `i` during slam. - `ames.c` missing `u3z` in some branches - `http.c` missing `u3z` in some branches, callbacks retained instead of transferring, which seems to be the protocol for callbacks - `term.c`: leak fixes (`_reck_orchid` retains), direct atom assertion - `unix.c` direct atom assertions - `dawn.c` u3r_string retains, u3nc transfers (although not a big deal right now as dawn_fail is noreturn) - `king.c` helpers transfer - `lord.c` missing break in switch! - `mars.c` `jar` leak - `pier.c` `tag` is transferred (probably left from u3r_string transferring long time ago)
Cherrypicked from #1059. I added u3_none treatment in the linter per @joemfb's request, found some omitted checks. Also extended the linter to check functions that neither take nor return nouns (common for callbacks for external code), found some leaks as well. Refcount action in eval in main.c was very confusing, I refactored it.
This PR contains the refcount linter, comment annotations for the linter as well as some jet fixes that are also PR'd separately in #1058.
The linter itself is fully vibecoded for now and I can't guarantee that it doesn't produce false negatives (it already did on previous iterations). At some point I'll rewrite the abstract interpreter.
What I would like to understand is whether the annotation grammar is uncontroversial. The annotations are placed either on functions, blocks of code (e.g. if we transfer a noun to some persistent structure) or the entire files (e.g. to supress checks for
nock.c)TODO:
Depends on #1057, #1079, #1080