Source audit: 13 defects fixed, SIMD kernels actually built and tested - #53
Merged
Conversation
Eight defects found by a full read of the sources, each reproduced by compiling and running the tree before being fixed. Crashes: * An unsupported (distance, type) pair called a NULL function pointer. The dispatch table only implements HAMMING for BIT vectors, but distance=hamming was accepted for any type. vector_init now rejects the pair up front, and all five dispatch sites go through a bounds-checked lookup so a future gap in the table becomes an error rather than a segfault. * A query vector supplied as a BLOB was never length-checked, so the kernels read v_dim elements from whatever the caller passed. The JSON branch already validated the dimension; the BLOB branch now does too. Memory safety (all four reproduced under ASan): * The U8/I8/1BIT quantized scans decoded rows on trust. vector0_* is an ordinary writable table, so a truncated blob or an inflated counter walked off the end. All four paths now carry the same length guard the TurboQuant paths had, and the non-TurboQuant streaming cursor - which never recorded the buffer length at all - now tracks it. * vector_quantize_preload sized its buffer from SUM(LENGTH(data)) and filled it with no per-row bound and an int offset. No race is needed to trigger it: LENGTH() counts characters on a TEXT value while sqlite3_column_bytes() returns UTF-8 bytes, so multi-byte text in the data column makes the SUM under-report - ASan reports a 2400-byte write past a 1200-byte buffer. Copies are now bounded, offsets are 64-bit, and the loaded rows must hold the number of vectors they claim before the index is published. * The preloaded index was written and freed under qmutex but read without it, and locking the read would not have been enough: a streaming cursor holds the pointer across many xNext calls, so vector_quantize_cleanup() freed it mid-scan on a single thread. The index is now reference counted; scans hold a reference while they walk it and preload/cleanup swap the table's copy without touching what a live cursor is reading. * sqlite3_vector_init freed the context by hand on its error path even though sqlite3_create_function_v2() had already taken ownership - and SQLite invokes the destructor when that call itself fails, so the second free landed immediately. Injecting an allocation failure at each of 400 points inside init crashed at 23 of them; the sweep is now clean. Correctness: * Identifiers were interpolated with %q, which escapes string literals and does nothing for ';' or '"'. A table named x;DROP TABLE victim;-- injected statements through vector_quantize_cleanup, which runs its DROP outside any savepoint. All nine identifier positions now use %w inside explicit double quotes; generate_quant_table_name switched to %s because it builds a bare name that is bound as a parameter. Generated SQL for ordinary names is unchanged, and names like order-items or "my vec" now work instead of failing with a syntax error. * vFullScanBestIndex asserted orderByConsumed unconditionally, so SQLite dropped the sorter and every ORDER BY on the table-valued function was silently ignored - including ORDER BY distance DESC. It is now claimed only for a single ascending ORDER BY on the distance column. Also adds the cosine shortcut behind normalized=1: for unit-length FLOAT32 vectors cosine distance is 1 - dot, which drops two thirds of the arithmetic from the inner loop. The query is normalized once per scan so the result is exact rather than merely rank-equivalent, and quantized scans ignore the flag because the quantized index holds scaled integers whose norm is not 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ve them
Every x86 build was shipping scalar code. The AVX2 and AVX-512 kernels sit
behind #if defined(__AVX2__) / __AVX512F__, but CFLAGS never enabled those ISAs
and CI just runs `make extension`, so both files compiled to nothing and
init_distance_functions_avx2() was an empty stub. The dispatch chain made it
worse: an if/else if ladder meant a CPU reporting AVX2 called the empty stub and
never fell through to the SSE2 kernels that were compiled in. Forcing
cpu_supports_avx2() true on a stock build reported vector_backend() = CPU.
* The Makefile now probes the compiler and gives distance-avx2.c and
distance-avx512.c their own -mavx2 -mfma / -mavx512{f,bw,vl,dq}, leaving every
other translation unit at the baseline target. The probe makes this a no-op on
non-x86 targets and on multi-arch builds.
* Each init_distance_functions_* now returns whether its kernels were compiled
in, and the ladder walks down instead of stopping at an empty stub.
Turning the flags on showed why nobody had noticed: distance-avx512.c had never
been compiled by anyone. It called _mm512_mask_set1_ps and _mm512_mask_set1_pd
in ten places and neither intrinsic exists; the intent - keep where the mask is
set, zero elsewhere - is _mm512_maskz_mov_ps/pd. It also needs AVX512DQ for
_mm512_extractf32x8_ps, which cpu_supports_avx512() did not check for.
On top of that, the f32 kernels kept a single vector accumulator, which makes
the loop one dependency chain: an FMA has roughly four cycles of latency, so it
retires one vector every four cycles however many FMA ports the core has. L2,
dot and L1 now carry four independent accumulators over 16 lanes per iteration
and use true FMA; cosine carries four per quantity on NEON and AVX-512, two on
AVX2 where sixteen YMM registers would spill, and SSE2 stays at two so 32-bit
x86 does not spill either. AVX2 and AVX-512 cosine were also implemented as
three separate calls to the dot kernel - three passes over the data - and are
now one fused pass.
Measured on Apple M-series, dim 768, f32, L2-resident, best of 300 runs
(Mvec/s): DOT 11.1 -> 25.6, SQUARED_L2 11.1 -> 25.6, L1 14.2 -> 28.4,
COSINE 9.8 -> 17.1. End to end through the virtual table a full cosine scan goes
from 4.35 to 7.02 Mvec/s - 1.6x, not 2.3x - because about a third of the time is
SQLite's per-row step and past a few megabytes the scan is bound by memory
bandwidth. Accuracy improved rather than degraded: shorter summation chains cut
the worst error against a double-precision reference (dot at 1536 dims,
4.1e-6 -> 1.8e-6).
Verification: the full 1447-test suite passes with the AVX2 backend active
(Rosetta executes AVX2, so these kernels - never compiled before - are exercised
for the first time), and every kernel is checked against a double-precision
reference across 25 dimensions including tails on CPU, NEON, SSE2 and AVX2.
AVX-512 is COMPILE-VERIFIED ONLY. No AVX-512 hardware or emulator was available:
Docker's x86_64 emulation on Apple Silicon stops at AVX2 and SIGILLs on an
EVEX-encoded vfmadd231ps, and QEMU TCG does not implement AVX-512 either.
Disassembly confirms the intended shape - four distinct zmm accumulators in the
dot loop, six in cosine, no stack spills, no calls back into the dot kernel -
but `make unittest` should be run on real AVX-512 hardware before a release.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
normalized= was parsed and enforced for consistency across vector_init calls but had no effect and appeared in neither README.md nor API.md. It now drives the cosine shortcut, so the contract needs stating: it is an assertion, not a request - if the stored vectors are not unit length the distances will be wrong - and quantized scans ignore it. Also notes that HAMMING is only valid with type=1BIT, which is now enforced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* xFilter returned SQLITE_DONE for k=0. Any non-OK return from xFilter is an
error code to SQLite; this one only looked harmless because 101 happens to be
the value sqlite3_step() reports at end of results. It now sets an empty
result and returns SQLITE_OK, and negative k takes the same path instead of
relying on sqlite3_malloc() rejecting a negative size.
* The 4-wide bodies of quantize_float32_to_{un,}signed8bit cast to int and
clamped afterwards, which is undefined for NaN and for magnitudes past
INT_MAX - the scalar helpers right above them already clamped in float first,
as does every other quantizer in the file. They now call q_round_u8 /
q_round_s8 too. Behaviour is unchanged on arm64 and x86-64 (both happen to
land on 0 for NaN), but -fsanitize=float-cast-overflow reported
"nan is outside the range of representable values of type 'int'" at
sqlite-vector.c:612 before and is clean after.
* sqlite_get_int_prikey_column bound a parameter to a statement that has none
(silently returning SQLITE_RANGE) and read bare type/name columns alongside
COUNT(*), which SQLite takes from an arbitrary row of the group. It now
selects one row per primary-key column and requires exactly one, of INTEGER
affinity, copying the name before the second step invalidates it.
* A BIT column under 8-bit quantization copied (dim+7)/8 bytes into a dim-byte
slot and left the rest uninitialised. On a populated table this failed anyway,
but with the unrelated message "not an error"; on an empty one it silently
recorded qtype=UINT8, which then applied to rows inserted later. qtype=AUTO on
a BIT column now means the identity 1BIT and an explicit 8-bit request is
refused with a message that says so. The three copy sites zero the slot first,
since an index written by an older build can still hold that shape.
The last one also fixes error reporting in vector_quantize: the cleanup label
overwrote whatever the callee had put on the context with sqlite3_errmsg(db),
which reported "not an error" whenever the failure came from a context error
rather than from SQLite. It now keeps the callee's message unless SQLite
actually has one, captured before the rollback resets it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The unittest target builds every source in a single invocation, which leaves __AVX2__ and __AVX512F__ undefined: those kernels compile to nothing and the suite exercises the scalar fallback instead. Every green CI run so far has been testing the SIMD backends by not testing them - the same mechanism that made every shipped x86 build scalar. * unittest-simd compiles per translation unit the way the extension target does, so the SIMD kernels are actually linked in. RUNNER wraps the binaries for an emulator, EXPECT_BACKEND asserts which tier got installed. * test/backend.c prints the installed backends and, given an argument, exits non-zero unless that is the one that was installed. Without this a silent fallback is indistinguishable from a pass: the suite runs, 1447 tests go green, and the kernels under test were never reached. * A new avx512 job runs the suite on those kernels. GitHub's hosted fleet is mixed - some runners have AVX-512, some do not, and there is no way to request one (actions/runner#1069) - so the job uses the hardware when it is there and Intel SDE when it is not, emulating Skylake-X because that is exactly the F/BW/VL/DQ set cpu_supports_avx512() requires. SDE is pulled from Intel's own download mirror with a pinned SHA-256; a mismatch fails the job rather than running an unverified binary. Verified on linux/amd64: unittest-simd installs the AVX2 backend and passes all 1447 tests (the plain unittest target reports CPU there), and EXPECT_BACKEND fails loudly on a mismatch. The SDE path could not be exercised locally - Pin aborts under nested emulation on Apple Silicon - but by construction that path cannot pass without AVX-512 having run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
A full read of
src/turned up thirteen defects and a build problem that made everyx86 release ship scalar code. Everything here was reproduced by compiling and running
the tree before being fixed, and the fixes are verified the same way.
The build problem
The AVX2 and AVX-512 kernels sit behind
#if defined(__AVX2__)/__AVX512F__, butCFLAGSnever enabled those ISAs and CI runs plainmake extension. Both filescompiled to nothing and
init_distance_functions_avx2()was an empty stub — andbecause the dispatch chain is an
if / else ifladder, a CPU reporting AVX2 calledthat stub and never fell through to the SSE2 kernels that were compiled in. Forcing
cpu_supports_avx2()true on a stock build reportedvector_backend() = CPU.Turning the flags on showed why nobody had noticed:
distance-avx512.chad neverbeen compiled by anyone. It called
_mm512_mask_set1_ps/_mm512_mask_set1_pdinten places and neither intrinsic exists.
The same mechanism was hiding in the test path:
make unittestbuilds every source inone invocation, so every green CI run so far has been testing the SIMD backends by not
testing them.
What changed
Crashes — an unsupported
(distance, type)pair called a NULL function pointer;a query vector supplied as a BLOB was never length-checked. Both reproduced as SIGSEGV.
Memory safety, all four reproduced under ASan — quantized scans decoded rows on
trust;
vector_quantize_preloadwrote past its buffer (no race needed:LENGTH()counts characters on TEXT while
sqlite3_column_bytes()returns UTF-8 bytes, somulti-byte text in
datamakes the sizingSUM()under-report — 2400 bytes writtenpast a 1200-byte buffer); the preloaded index was freed while a streaming cursor still
pointed into it;
sqlite3_vector_initdouble-freed its context on the error path.Correctness — identifiers were interpolated with
%q, which escapes stringliterals and does nothing for
;or": a table namedx;DROP TABLE victim;--injected statements through
vector_quantize_cleanup. AndorderByConsumedwasasserted unconditionally, so SQLite dropped the sorter and every
ORDER BYon thetable-valued function was silently ignored, including
ORDER BY distance DESC.Performance — per-unit ISA flags so the SIMD kernels are built; a dispatch ladder
that falls through; four independent accumulators and true FMA in the f32 kernels
(one accumulator made the loop a single dependency chain); AVX2/AVX-512 cosine was
three passes over the data and is now one. Measured on Apple M-series, dim 768,
L2-resident (Mvec/s): DOT 11.1 → 25.6, SQUARED_L2 11.1 → 25.6, L1 14.2 → 28.4,
COSINE 9.8 → 17.1. Accuracy improved rather than degraded — shorter summation chains.
normalized=1now does something: for unit-length FLOAT32 vectors cosine collapses to1 - dot. The query is normalized once per scan so the result is exact, not merelyrank-equivalent, and quantized scans ignore the flag. It was previously parsed,
validated for consistency, and undocumented.
Evidence
including tails, on CPU, NEON, SSE2, AVX2 and AVX-512.
avx512CI joblanded on an AMD EPYC 9V74 with no AVX-512, took the Intel SDE path, and reported
distance backend: AVX512followed by 1447 passing tests. That is the first timethat code has ever run.
distance-neon.ccompiles clean forarmv7ain the android job, so thenon-aarch64 fallbacks hold.
Read before merging
SQLite's per-row step, and past a few megabytes it is bound by memory bandwidth.
normalized=1adds ~2% on top for the same reason. The remaining levers are batchingthe f32 rows, caching statements, a heap for the top-k, and threading.
normalized=1is an assertion, not a request: a table whose vectors are notactually unit length will return wrong distances. Documented, but it is a new way to
be wrong.
shadow table filled with well-sized nonsense still decodes to nonsense, it just stays
inside its buffer.
qtype=AUTOon a BIT column now means1BITinstead of failing, and an explicit8-bit request on one is refused — a behaviour change, though the old path errored out
or silently recorded an unusable state.
Generated SQL for ordinary identifiers is byte-identical, so existing databases need no
migration.
🤖 Generated with Claude Code