v4.4.1 — Correctness patch: ten defensive-review fixes - #102
Merged
Conversation
SSE2 log_pd lacked the subnormal prescale that the AVX-512/AVX2/NEON
log_pd overloads already have: below DBL_MIN, the exponent-extraction
path treated the biased exponent 0 as a normal number, compressing the
entire subnormal range into a single wrong result (log_pd(5e-324) was
landing at -709.09 instead of -744.44). Ports the 5-line prescale from
the AVX2 variant using the SSE2 section's existing sse2_blend helper:
scale by 2^54 when denormal, subtract 54 back out of the exponent.
Adds a regression gate (test_transcendental_kernels.cpp,
LogSubnormalPrescale suite) that calls each compiled-in tier's
log_batch_<tier> symbol directly on {5e-324, 1e-310,
2.2250738585072014e-308, 1.0} and asserts <= 1 ULP against std::log,
so a future per-tier asymmetry of this shape cannot recur silently.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sin_pd(-0.0) returned +0.0 on every tier: in the clean-room quadrant-reduction kernel (#74), x = -0.0 reduces to r = -0.0, rlo = +0.0, and the sin core's final add (-0) + (+0) rounds to +0 under IEEE 754, dropping the sign before quadrant recombination ever runs. cos(+/-0) = 1 is unaffected and untouched. Fix: one blend per tier's sin_pd, after quadrant recombination, selecting x itself back in where x == 0.0 (the compare matches both zeros and no other double) -- mirrors libstats' fix (issue #98, commit 7dbb211): blendv/mask_blend/vbslq per tier. Adds explicit std::signbit assertions (test_trig_ulp_gates.cpp, TrigSignOfZero suite) per tier, since the existing ULP-lattice gate maps +0/-0 to the same ordinal and is blind to this class of defect. Evidence on this machine (Windows/MSVC/Zen4): pre-fix, only the SSE2 tier showed red (AVX2/AVX-512 already emitted the correctly-signed zero here -- MSVC's FMA codegen for the reduction happens to produce +0.0 before the final combine, unlike the issue's general 'every tier' claim, presumably observed under a different compiler). The fix is applied uniformly to all four tiers regardless, since the defect is compiler/codegen-dependent and the universal fix is cheap and correct everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion (#83) detect_avx512() checked only leaf-7 EBX bit 16 (AVX-512F), while simd_double_ops_avx512.cpp is compiled -mavx512f -mavx512dq (/arch:AVX512 on MSVC) and both that TU and this header's 8-wide section use AVX-512DQ intrinsics (_mm512_cvtepi64_pd, _mm512_andnot_pd). On an F-without-DQ part (e.g. Knights Landing/Mill, or a hypervisor CPU model masking DQ) the dispatcher would install a tier the CPU cannot execute and the first batch call SIGILLs. detect_avx2() likewise checked only leaf-7 EBX bit 5 (AVX2), never leaf-1 ECX bit 12 (FMA3), while the AVX2 TU is built -mavx2 -mfma and executes FMA intrinsics directly. Fix: - detect_avx512() now requires F + DQ + BW + VL (kAvx512RequiredMask, exposed in cpu_detection.h) -- exactly what /arch:AVX512 licenses. - detect_avx2() now additionally requires leaf-1 ECX bit 12 (FMA3) before the leaf-7 AVX2 test. - cmake/SimdDispatch.cmake's non-MSVC compiler-support probes now test the exact flag sets passed to the per-ISA TUs (-mavx512f -mavx512dq, -mavx2 -mfma), not just the bare ISA flag -- hygiene only, cannot fire on any compiler at or above this project's GCC 12 / Clang 14 floor. Test (test_simd_platform.cpp): when supports_avx512()/supports_avx2() report true, independently re-checks CPUID leaf 7 EBX bits 17/30/31 and leaf 1 ECX bit 12 directly -- true on this machine (Zen 4) and every CI runner both pre- and post-fix, so these two document the contract rather than catching a regression. The regression-guard is Avx512RequiredMaskExactValue, which pins kAvx512RequiredMask's exact value; verified red under a deliberate one-bit perturbation (dropping VL, mask 0xC0030000 -> 0x40030000) and green again after restoring it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
json::Reader::read_double() called std::strtod() directly on src_.data() + pos_, a NUL-terminated-string API with no length parameter, against a caller-supplied string_view that is not guaranteed to be NUL-terminated (a substring, a memory-mapped file, a network buffer). The scan could run past src_.size(), and pos_ could then advance beyond the view's bound. Copy the candidate token into a small NUL-terminated stack buffer bounded by min(63, src_.size() - pos_) and parse that instead; 64 bytes covers any double a writer can emit (max_digits10 = 17 digits plus sign, decimal point, and exponent). Advancing pos_ by the parsed length from that bounded buffer means it can never exceed src_.size(). Adds a JsonReader.ReadDoubleDoesNotReadPastViewBounds test that exercises the bug directly, and the issue's suggested from_json() probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
from_json() and from_json_mv() bounded array/matrix *sizes* via checked_size() but never inspected the *values* read into pi or trans: NaN, infinity, and negative weights were silently accepted, letting the model later score NaN log-likelihoods or drive draw_categorical's inverse CDF outside its documented precondition. Add check_weights(const std::vector<double>&, const char*) in the anonymous namespace beside checked_size: every entry must be finite and non-negative, else throws std::runtime_error naming the field. Called for pi and for each trans row in both from_json and from_json_mv. Normalisation is deliberately not enforced (trainers renormalise). Tests: five scalar-path throw cases (pi=[nan], pi=[inf], pi=[-1.0], trans=[[nan]], trans=[[-5.0]]) plus a valid-baseline no-throw case in test_hmm_json.cpp, and NaN/negative throw cases plus a valid baseline on the MV path in test_hmm_json_mv.cpp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
operator>>(std::istream&, Hmm&) parsed States: with std::stoull and checked only states == 0. std::stoull accepts a leading '-' (States: -1 wraps to ULLONG_MAX and passes that check), a non-numeric token raises std::invalid_argument rather than a documented exception type, and an in-range but oversized count (e.g. 200000) attempts a 200000x200000 trans_ matrix before anything validates it. Add kMaxLegacyStates = 4096 (kept in step with kMaxHmmStates in src/io/hmm_json.cpp via a comment; promoting it to a shared header would be clean if a third format ever needs the same bound) and reject states == 0 || states > kMaxLegacyStates before any allocation. Wrap the std::stoull call so a non-numeric token also throws std::runtime_error rather than an undocumented exception type. Also widen the catch in XMLFileReader::read and XMLFileReader::readFromStream to translate std::bad_alloc and std::logic_error (invalid_argument/out_of_range/length_error) into the documented std::runtime_error, since operator>> can now raise those through the legacy XML path too. Tests: States: = 4294967296, -1, and abc against operator>> (red pre-fix: bad_alloc, length_error, invalid_argument respectively). States: = 200000 is included as a fourth case but was NOT executed pre-fix here — it would attempt an unbounded ~320 GB allocation on this machine; reasoned instead per the issue's guidance, and verified green post-fix where the bound check makes it cheap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#86) getBatchLogProbabilities(observations, out) ignored out.size(), so a caller passing a shorter out span got an out-of-bounds heap write instead of an error. Add a protected static checkBatchSpans(n_obs, n_out) helper on BasicEmissionDistribution that throws std::invalid_argument when out is shorter, call it at the head of all 17 concrete overrides (16 distributions + the CRTP scalar-loop default in DistributionBase), and document the contract in the base-class Doxygen. Verified pre-fix behaviour crashes with STATUS_HEAP_CORRUPTION (0xC0000374) on this machine when GaussianDistribution's batch path is exercised with a shorter out span, matching the issue's own guard-page probe. Tests: one tier-2 (Gaussian) and one tier-1 (Poisson) EXPECT_THROW(..., std::invalid_argument) with obs(100)/out(10). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Discrete/Poisson/Binomial/NegativeBinomial validated isnan/isinf/<0 and then cast a finite double of any magnitude to int/size_t. Casting a value outside the target type's representable range is undefined behaviour ([conv.fpint]) and is ISA-dependent: x86 CVTTSD2SI yields INT_MIN (which existing range checks happen to reject), AArch64 FCVTZS saturates to INT_MAX and returns a large finite log-probability instead. Fix: compare against the distribution's own domain bound before casting -- Poisson/NegBin k <= double(INT_MAX), Binomial value <= double(n_), Discrete x < double(numSymbols_) -- returning the existing zero/-inf result, with the same guard applied in the fit() loops (Binomial, Discrete) and getCumulativeProbability (Poisson, Binomial, NegativeBinomial). Deviations from the issue's line list: - NegativeBinomialDistribution::fit() no longer casts to int at all (k is kept as a double for the Newton-Raphson MLE solver), so no guard was needed there; the fit() test is a defensive regression guard rather than a UB reproduction. - Also fixed DiscreteDistribution::setProbability() in the header (same UB pattern, not one of the issue's listed .cpp sites but directly adjacent to the sites that are). Confirmed the ISA-dependence empirically on this x86_64/MSVC machine: reverting Poisson's isValidCount() upper bound and rerunning CastOverflowGuard still passes, because CVTTSD2SI's INT_MIN result routes through the existing negative-k rejection path and happens to produce the same -inf/0.0 answer. True red evidence for this issue requires an AArch64 leg, unavailable here; this is the "clearly reasoned argument" exception for genuine UB with no visible failure on this ISA. Tests: per distribution, getLogProbability(1e15) == -inf, getProbability(1e12) == 0.0, one batch call, and one fit() call containing such an element. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StudentTDistribution validated degrees_of_freedom and scale but never
location (mu): the 3-arg constructor skipped it and setLocation() was
a bare inline assignment. A NaN/inf mu silently made every
probability NaN instead of throwing, including via from_json (every
other distribution's from_json rejects a NaN parameter; StudentT was
the exception).
Throw std::invalid_argument("Location parameter must be a finite
number") for non-finite mu in the constructor and in setLocation();
move setLocation() from the header to the .cpp beside setScale(). No
invalidateCache() call: none of updateCache()'s cached fields depend
on location_ (verified by reading it).
Also added, per the issue's note that the metrics sweep flagged these
as untested: basic coverage for StudentT::setScale, VonMises::setMu,
and VonMises::setKappa.
Tests: ctor with NAN and INFINITY, setLocation(NAN), and from_json of
{"type":"StudentT","df":3.0,"mu":nan,"sigma":1.0} all throw
std::invalid_argument; one basic test each for setScale/setMu/setKappa.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kappa_from_r_bar's Newton loop formed A = I1(kappa)/I0(kappa) as a direct ratio. I0(kappa) overflows to +inf above kappa ~= 713.99, so for R_bar >= ~0.99930 (ordinary angular dispersion, ~2 degrees, not an edge case) the Mardia-Jupp seed already starts above that threshold, A becomes inf/inf = NaN, and the loop's `kappa > 0.0` guard doesn't catch a NaN kappa -- it gets stored by fit() and every log-probability of the state becomes NaN. Fix: form A via `1.0 - detail::one_minus_bessel_ratio(kappa)`, which switches to a tier-independent asymptotic series above the overflow threshold with no I0 evaluation at all (the helper #73 already added for getCircularVariance(), just not wired into this solver). Add a belt-and-suspenders `if (!std::isfinite(kappa))` check after the Newton update that saturates to MAX_DISTRIBUTION_PARAMETER instead of storing NaN. A'(kappa) = 1 - A^2 - A/kappa is unchanged. Reference kappa values for the regression test solve I1(kappa)/I0(kappa) = R_bar via mpmath (dps=80, findroot from the Mardia-Jupp seed); the generating script and resulting values are recorded in the test file comment (not checked in as a scripts/ file, per the issue's "record the essentials" instruction). Tests assert on getKappa()/getLogProbability(), not getCircularVariance() -- one_minus_bessel_ratio(NaN) deliberately returns 1.0, which would hide the NaN this test exists to catch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… tier note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MSVC and AppleClang pull it in transitively; libstdc++ does not, which failed all four Linux CI legs at compile. All other changed TUs verified syntax-clean under g++/libstdc++. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Version bump, CHANGELOG 4.4.1 section, PLAN.md milestone record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 27, 2026
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements all ten issues on milestone v4.4.1 — Correctness patch, from the 2026-08-21 defensive review. Bug fixes only — no API surface change beyond the documented new throws.
Memory safety
getBatchLogProbabilitiesnow throwsstd::invalid_argumentwhenoutis shorter thanobservations(previously an OOB heap write; reproduced asSTATUS_HEAP_CORRUPTIONpre-fix). All 16 concrete overrides plus the CRTP fallback (whose Release-modeassertwas a no-op).json::Reader::read_doubleparses from a bounded NUL-terminated buffer instead of runningstrtodpast a non-NUL-terminatedstring_view.Numerical correctness
fit()no longer stores κ = NaN for R̄ ≥ 0.9993: Newton solver forms A viaone_minus_bessel_ratio(no I₀ overflow), plus a finite-guard. κ verified against mpmath references.log_pdgets the subnormal 2⁵⁴ prescale the other tiers already had; per-tier regression test runs the same vector through every tier.sin_pd(−0.0)returns −0.0 on all four tiers (libstats#98 port); ULP gates gain explicitsignbitassertions on the ±0 specials.Dispatch safety
Input validation
setLocation, JSON).pi/transentries (both scalar and MV paths).States:count — unbounded allocation, wrong exception types,-1→ ULLONG_MAX #89 — legacyStates:bounded at 4096 before allocation;XMLFileReadertranslatesbad_alloc/logic_errorto its documentedruntime_error.Every fix carries its issue-named regression test, demonstrated red-before-green where reachable (exceptions recorded on the issues/PLAN). Local: Zen 4 / MSVC, full suite 51/51 on the merged tree. The macOS/AArch64 leg is the decisive one for #88 (cast parity) and #81 (NEON); ASan for #86/#87.
Issues intentionally carry no closing keywords — they will be closed with comments at release per project convention.
🤖 Generated with Claude Code