fix(FIX-UNALIGNED-CONSUMERS-2540): read a borrowed 16-bit weight off a byte cursor, so the odd base costs nothing - #2581
Conversation
… safetensors bytes, not the borrow A safetensors payload starts at `8 + <JSON header length>`, and a header length is arbitrary, so a 16-bit weight begins on an odd byte in roughly half of all checkpoints. That is an ordinary file. Three CPU consumers read those bytes through a `const uint16_t*` and `-fsanitize=alignment` aborts on the load, which is the second and third findings holding the `sanitize-cpu` lane red (#2540, #2558). The spec records why the fix goes in the consumer. Refusing to borrow a misaligned tensor also takes the sanitizer green, and it switches direct upload off for every tensor in half of all checkpoints: recomputed here from the direct-upload fixture, whose 171-byte header puts `w` at 179, `v` at 195 and `f` at 211, all odd. `vt::LoadUnaligned` costs nothing on that path, so the borrow survives. The bullet this row takes over is struck from the DEBTFIX spec's `## Owed`, together with the guess it recorded. The root cause is not "wherever `ResidentWeight` produced an odd `bytes.data()`"; it is the file, and a file is not a defect. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
…a byte cursor, so the odd base costs nothing Closes #2540. Closes #2558. Closes #2578. `WidenRowToF32`, `HadRowBlock`, `TileWord32` and `WeightF32` each formed a `const uint16_t*` over bytes the direct-upload borrow handed them straight out of an mmap. A safetensors payload starts at `8 + <JSON header length>`, a header length is arbitrary, and `vt::Exl3Gemm` types the trellis kI8, so an odd base is the expected case rather than an exotic one. The load is undefined even where x86 executes it, and the `sanitize-cpu` lane aborts on it before reporting a single assertion. All four now read through `vt::LoadUnaligned` off an `unsigned char` cursor, which is the shape `vt::cpu::LoadF32` and `cpu_layernorm.cpp` already use and which compiles to the same instruction at `-O2`. The trellis walk moves to byte arithmetic for the same reason: advancing a misaligned `uint16_t*` is undefined in its own right, so `Exl3DecodeTile`, `Exl3ReconstructInner`, `Exl3DequantLinear` and `MoeGemm` take a `const void*` and the stride carries the `sizeof(uint16_t)` the pointer type used to supply. Every existing caller passes a `uint16_t*`, which converts implicitly, so no call site changed. `in`, `out` and the activation keep their typed pointers. They are engine-allocated through `std::aligned_alloc(64, ...)`, never a borrowed file mapping, which is the polarity `cpu_layernorm.cpp` states. The alternative fix refuses the borrow. It works, and it turns direct upload off for every tensor in half of all checkpoints -- the direct-upload fixture's 171-byte header puts all three of its tensors on odd addresses -- so it reds `test_load_direct_upload` and `test_qwen3_5_dense_load_residency`. Both stay green here, which is what separates the two fixes. No alignment `VT_CHECK` is added to `vt::Exl3Gemm` for the same reason: after this change a misaligned operand is representable, and a refusal must name something the kernel cannot do. `WeightF32` was not in the dispatch. It is the SAME defect one frame further on, and it only became visible because `-fno-sanitize-recover=all` stops at the first report: repairing the EXL3 operands made `test_qwen35_exl3` abort in `WeightF32` on the very next run. It is filed as #2578 and fixed here, because the target state is not reached without it. A grep finds 20 further sites of the shape in ten model files; whether any is reached with an odd base is unmeasured, so that is filed as #2579 and named under the spec's `## Owed` rather than swept blind. Two cases gate it, each in the suite that already owns its subject, and each `REQUIRE`s the address parity it depends on before it asserts anything. They compare byte-for-byte against the same operands at even addresses, because a byte cursor that lost its factor of two decodes a different weight and a tolerance over a random-bit trellis could absorb that. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
|
Operator static review, recorded before the gate is rerun. I did not write this change. Producer untouched — the criterion that matters
Pointer arithmetic verified by handThe risk in this shape of change is a stride that looks right and is off by a factor of two. I re-derived every converted site against the arithmetic the
On widening four signatures to
|
2 participants
fix(FIX-UNALIGNED-CONSUMERS-2540): read a borrowed 16-bit weight off a byte cursor, so the odd base costs nothing
Closes #2540. Closes #2558. Closes #2578.
A safetensors payload starts at
8 + <JSON header length>and a header length is arbitrary, so a 16-bit weight begins on an ODD byte in roughly half of all checkpoints. That is an ordinary file.BorrowStTensorByteshands those bytes to the kernel verbatim, which is what direct upload IS, and four CPU consumers then formed aconst uint16_t*over the odd address:vt::cpu::WidenRowToF32cpu_matmul_elem.cpp:577dots3_noteandmuse_glimmer(#2540)HadRowBlockcpu_exl3_kernels.cpp:130suh/svh(#2558)TileWord32cpu_exl3_dequant.cpp:64uint16_t(#2558)WeightF32qwen3_5.cpp:1028The load is undefined even where x86 executes it, and
sanitize-cpu (address,undefined)aborts on it before reporting a single doctest assertion. All four now read throughvt::LoadUnalignedoff anunsigned charcursor — the shapevt::cpu::LoadF32andcpu_layernorm.cppalready use, which at-O2compiles to the load the raw index compiled to.The trellis walk moves to byte arithmetic for the same reason: advancing a misaligned
uint16_t*is undefined in its own right.Exl3TileCodeword,Exl3DecodeTile,Exl3ReconstructInner,Exl3DequantLinearandMoeGemmtake aconst void*, and the stride carries thesizeof(uint16_t)the pointer type used to supply. Every existing caller passes auint16_t*, which converts implicitly, so no call site outside the repaired files changed.in,outand the activation keep their typed pointers: they are engine-allocated throughstd::aligned_alloc(64, ...), never a borrowed mapping, which is the polaritycpu_layernorm.cppstates.Why the consumer and not the borrow
The alternative fix refuses to borrow a misaligned tensor, so the caller's
MakeOwned+memcpyfallback copies into aligned memory. It works, and it switches direct upload off for every tensor in half of all checkpoints. Recomputed here from the direct-upload fixture at the base SHA: its header is 171 bytes, so the payload base is 179, andw(179),v(195) andf(211) are all odd. A producer-side gate therefore redstests/vllm/test_load_direct_upload.cpp:142(CHECK(w.bytes.borrowed())) andtests/vllm/models/test_qwen3_5_dense_load_residency.cpp:296. Both stay green here, and that is what separates the two fixes.No alignment
VT_CHECKis added tovt::Exl3Gemmeither. After this change a misaligned operand is representable, so such a check would be the producer gate relocated one frame up: it would refuse the borrow path this change exists to preserve. A refusal must name something the kernel cannot do.WeightF32was not in scope when the work started. It is the same defect one frame further on, and it only became visible because-fno-sanitize-recover=allstops at the first report — repairing the EXL3 operands madetest_qwen35_exl3abort inWeightF32on the very next run. Filed as #2578 and fixed here, because the target state is not reached without it. A grep finds 20 further sites of the shape in ten model files; whether any is reached with an odd base is UNMEASURED, so that is filed as #2579 and named under the spec's## Owedrather than swept blind.Evidence
Build:
cmake -S . -B build-sanitize -DVLLM_CPP_BUILD_TESTS=ON -DVLLM_CPP_CUDA=OFF -DVLLM_CPP_SANITIZE='address,undefined',-j 4. Run:UBSAN_OPTIONS=print_stacktrace=1 ASAN_OPTIONS=detect_leaks=1:strict_string_checks=1 VT_POOL_BYPASS=1 ctest.RED before, one site at a time. Each mutation restores only the pre-fix LOAD, leaving signatures and strides alone, so the red is the defect's and nothing else's; each asserts its own application, because a mutation that never applied reads as a pass.
WidenRowToF32test_muse_glimmer_text,test_dots3_note_attn,test_ops_rmsnorm_weight_dtypecpu_matmul_elem.cpp:598: load of misaligned address ...907 / ...7cd / ...081HadRowBlocktest_exl3_gemm,test_qwen35_exl3cpu_exl3_kernels.cpp:142: load of misaligned address ...6efTileWord32test_exl3_gemm,test_qwen35_exl3cpu_exl3_dequant.cpp:78: load of misaligned address ...76bWeightF32test_qwen35_exl3qwen3_5.cpp:1043: load of misaligned address ...cd3GREEN after, on the restored tree (verified byte-identical to the fix): 11 of 11 pass, 0
misaligned addressreports, 0runtime errorlines.scripts/agent-preflight.shreports 0 gate failures with the same 5 environmental SKIPs the untouched base reports, andcheck-tree-compilescompiled 596 of 596 translation units in scope.Tests
Two cases, each added to the suite that already owns its subject, so no
tests/CMakeLists.txtrow moves. EachREQUIREs the address parity it depends on before it asserts anything, so a case that quietly landed on an even address cannot read as a pass. Both compare BYTE-FOR-BYTE against the same operands at even addresses rather than within a tolerance: a byte cursor that lost its factor of two decodes a different weight, and a tolerance over a random-bit trellis could absorb that.Spec:
.agents/specs/unaligned-safetensors-consumers.md, committed before the implementation.FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5-1m [claude-code]