test_punica_cpu covers AddShrink's out-of-range-slot guard but not the same guard in
BgmvShrink or BgmvExpandSlice. Dropping either one leaves the suite green.
Evidence
Mutation on pinned main, with src/vllm/lora/punica_cpu.cpp:53 changed from
if (s < 0 || s >= num_slots) continue;
to
test_punica_cpu still passes: 8 test cases / 149 assertions / rc=0, under
address,undefined as well as plain.
The reason is in the fixture, not the kernel. The bgmv_shrink case
(tests/vllm/lora/test_punica_cpu.cpp:102) uses
const std::vector<int32_t> idx = {0, 1, 2, -1, 1, 0}; // num_slots = 3
— every index is either valid or -1. Nothing exercises s >= num_slots, so the upper half of
the guard has no test. BgmvExpandSlice's guard at punica_cpu.cpp:82 is uncovered for the same
reason.
By contrast AddShrink's guard is covered: the case at line 291 feeds
idx = {1, -1, 0, num_slots}, and dropping slot >= num_slots there moves an ASan
heap-buffer-overflow into punica_cpu.cpp:129 and fires six
REQUIRE(got == kSentinel) violations in a plain -O2 non-sanitized build. That is the shape the
other two cases should have.
Suggested close
Add an out-of-range index to the existing case rather than a new one, e.g.
const std::vector<int32_t> idx = {0, 1, 2, -1, num_slots, 0};
RefShrink now skips out-of-range slots (#395), so ref for that token is 0 while a kernel
without the guard writes garbage into out — CheckClose fires. The same lever applies to the
expand-side case.
Provenance
Found by the fresh reviewer of PR #399 while mutating the guards that PR's test change depends on.
Pre-existing, not introduced by #399 — the gap has been there since 1d37f152
(feat(lora): W0 spike + W1 CPU punica brick). Filed separately rather than folded into #399, whose
scope is the two sanitizer failures currently red on main.
Row: LORA-RUNTIME.
test_punica_cpucoversAddShrink's out-of-range-slot guard but not the same guard inBgmvShrinkorBgmvExpandSlice. Dropping either one leaves the suite green.Evidence
Mutation on pinned
main, withsrc/vllm/lora/punica_cpu.cpp:53changed fromto
test_punica_cpustill passes: 8 test cases / 149 assertions / rc=0, underaddress,undefinedas well as plain.The reason is in the fixture, not the kernel. The
bgmv_shrinkcase(
tests/vllm/lora/test_punica_cpu.cpp:102) uses— every index is either valid or
-1. Nothing exercisess >= num_slots, so the upper half ofthe guard has no test.
BgmvExpandSlice's guard atpunica_cpu.cpp:82is uncovered for the samereason.
By contrast
AddShrink's guard is covered: the case at line 291 feedsidx = {1, -1, 0, num_slots}, and droppingslot >= num_slotsthere moves an ASanheap-buffer-overflow into
punica_cpu.cpp:129and fires sixREQUIRE(got == kSentinel)violations in a plain-O2non-sanitized build. That is the shape theother two cases should have.
Suggested close
Add an out-of-range index to the existing case rather than a new one, e.g.
RefShrinknow skips out-of-range slots (#395), soreffor that token is0while a kernelwithout the guard writes garbage into
out—CheckClosefires. The same lever applies to theexpand-side case.
Provenance
Found by the fresh reviewer of PR #399 while mutating the guards that PR's test change depends on.
Pre-existing, not introduced by #399 — the gap has been there since
1d37f152(
feat(lora): W0 spike + W1 CPU punica brick). Filed separately rather than folded into #399, whosescope is the two sanitizer failures currently red on
main.Row:
LORA-RUNTIME.