refcount linter - #1059
refcount linter#1059dozreg-toplud wants to merge 62 commits into
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
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.
…rminating +range Folds in the findings from ~dozreg-toplud's refcount linter (#1059) on the rebased #1057 and the follow-up review notes. - `_get_length` and the `mmul` dimension reads used `u3x_atom` products as C integers; an indirect atom in a shape would blow up. Read them through `u3r_cat`, so an indirect dimension bails instead of being truncated. (Also makes the `Na != Nb` check compare values rather than noun words.) - `u3wi_la_dot` kept borrowed references into `x_meta` (`x_shape` &c) across `u3r_sing(x_meta, y_meta)`. Unifying equality rewrites equal subnouns in place, so the borrowed `x_shape` could be freed before its use in the result. The wrapper now reads every field it returns only after the last comparison that could have rewritten it. - `u3qi_la_*` / `u3wi_la_*` are typed `u3_weak` in the definitions and in q.h/w.h, since they return `u3_none` to punt. - `_set_rounding` keeps develop's `u3_atom` parameter (the mode is a noun, and c3_w would truncate it on the 64-bit loom) while yielding c3n on an unrecognized mode so callers punt. - The dot and trace wrappers use manual noun splitting like the rest of the file on develop (#1009) instead of `u3r_mean`. - +range: where the Hoon can never terminate -- a NaN upper bound, or a stalled accumulator (zero/underflowing step, NaN, infinite start) -- the jet bails %fail rather than punting. An infinite loop is a non-deterministic failure, not a deterministic %exit, and punting it to the Nock hangs the ship. The absurd-count cap is still a punt. Checked with the refcount linter against the lagoon translation unit (0 findings). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
Rebasing the math.hoon transcendental jets onto post-vere64 develop: - The four single-argument @r jet helpers (_rd_jet/_rs_jet/_rh_jet/ _rq_jet) bound the u3_weak product of u3r_at(u3x_sam, cor) to a u3_noun before the u3_none/u3ud guard; declare it u3_weak (refcount linter, #1059). - u3r_mean was rewritten on develop (urbit/urbit dozreg/refcount-check lineage) to take {axe, &out} pairs via u3r_vmean; convert the five call sites from the old flat, 0-terminated form to braced pairs, or -Wmissing-braces fails the -Werror build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
Adds the /lib/twoc scalar two's-complement jets (twoc.c/twoc.h) and the %int2 element-wise array path in the lagoon jets, registered in the 135/ 136/137 trees. Rebased onto #1057: the %int2 cases slot into that PR's rewritten +add/+sub/+mul/+div/+mod wrappers, and every jet that declines with u3_none (all the twid ops, the %int2 helper, and the int2 wrapper results) is typed u3_weak, matching the #1057 convention and the refcount linter (#1059). twoc.c is byte-identical to the ship-verified urbit/numerics copy and uses only fixed-width types (c3_b/s/w/d and __int128), so it is loom-agnostic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
Adds the %int2 (signed two's-complement) element-wise comparison (gth/gte/lth/lte) and reduction (cumsum/dot/min/max/argmin/argmax) array jets, on top of the twoc scalar jets, rebased onto #1057. - The %int2 cases slot into #1057's rewritten comparison and reduction wrappers; the dot case is added to #1057's restructured +dot wrapper (its unifying-equality-safe field reads made the auto-merge miss it). - All the new _la_int2_* helpers and the wrapper int2 results are typed u3_weak (they decline with u3_none); _la_int2_box, which transfers its r_data into the boxed ray, is annotated `@Refcount: transfers `r_data`` so the reduction wrappers do not read as leaking it (refcount linter, #1059). - The reduction result shape is the all-ones-of-rank +scalar-to-ray box, matching #1057's i754 _ones_shape; #1048's separate i754 reduction- shape fix is already carried by #1057, so only the %int2 additions land here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
_sip_rub() writes a freshly-decoded, transferred atom into its `val` out-parameter whenever the pointer is non-null. Without an annotation dozreg's refcount checker (#1059) cannot model the pointee protocol, so _sip_rub and both its callers (_sip_gaze, u3qi_sip_grab) come back as [complicated] / [annotation] and are not analyzed. Annotate it `@Refcount: fills transferred `val``. Comment-only; no behavior change. The checker now reports 0 findings on sip.c (also under --strict-weak). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
Every u3qi_unum_* / u3wi_unum_* can return u3_none -- the arm punts on an unsupported bloq (posit64/128, not in SoftUnum), and each wrapper punts when _unum_bloq() cannot read the door sample -- and the unary wrappers read the sample with u3r_at (a u3_weak product). Declaring them u3_noun made dozreg's refcount checker (#1059) flag 109 [u3_none] findings: none-literal returns from a u3_noun function, and u3r_at products stored into u3_noun locals. Retype the wrapper return types and the u3r_at locals (incl. _unum_bloq's) to u3_weak, in the .c definitions and the q.h/w.h declarations. u3_weak is a typedef of u3_noun (pkg/noun/types.h), so this is a type-annotation change only, with no runtime effect. The checker now reports 0 findings on unum.c (also under --strict-weak). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqqco86RgnvtiC4axw8Px3
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