While working on the arm64 lowering for #1160/#1167 I noticed (*sequenceDecs).decodeSyncSimple in zstd/seqdec_asm.go has:
// FIXME: Using unsafe memory copies leads to rare, random crashes
// with fuzz testing. It is therefore disabled for now.
const useSafe = true
with the real dynamic logic (computing useSafe from buffer slack, same shape as executeSimple's still-active dynamic useSafe) commented out below it. This dates to #644 (2022-07-17), soon after #637 added the faster unsafe copy (up to ~1.46x on some benchmarks) and shortly after #562/#563 (an earlier decodeSync crash, root-caused and fixed). #644 doesn't link an issue or root cause — so this reads as "another crash turned up, we cut losses" rather than a one-off.
Four years later, sequenceDecs_decodeSync_{amd64,bmi2} are still generated and shipped, but useSafe being a compile-time true means nothing ever calls them — no test or fuzz target exercises them. The arm64 avo lowering (#1160) mechanically translates the same IR, so this dead, untested path now exists on a second architecture too (_arm64/_bmi2), doubling the footprint of code nobody runs.
Given the track record (fixed once, still had to be disabled again within months), I don't think restoring the commented-out logic as-is is right either. Two reasonable paths:
- Remove it — drop the unsafe generation from
_generate/gen.go, simplify the safe bool out of decodeSyncAsm's dispatch, drop the dead code in seqdec_asm.go. Mechanical, no behavior change from what ships today.
- Root-cause and re-enable it with real regression coverage, matching how
executeSimple's analogous path stayed dynamically toggled.
Happy to send a PR either way — let me know which you'd prefer.
While working on the arm64 lowering for #1160/#1167 I noticed
(*sequenceDecs).decodeSyncSimpleinzstd/seqdec_asm.gohas:with the real dynamic logic (computing
useSafefrom buffer slack, same shape asexecuteSimple's still-active dynamicuseSafe) commented out below it. This dates to #644 (2022-07-17), soon after #637 added the faster unsafe copy (up to ~1.46x on some benchmarks) and shortly after #562/#563 (an earlier decodeSync crash, root-caused and fixed). #644 doesn't link an issue or root cause — so this reads as "another crash turned up, we cut losses" rather than a one-off.Four years later,
sequenceDecs_decodeSync_{amd64,bmi2}are still generated and shipped, butuseSafebeing a compile-timetruemeans nothing ever calls them — no test or fuzz target exercises them. The arm64 avo lowering (#1160) mechanically translates the same IR, so this dead, untested path now exists on a second architecture too (_arm64/_bmi2), doubling the footprint of code nobody runs.Given the track record (fixed once, still had to be disabled again within months), I don't think restoring the commented-out logic as-is is right either. Two reasonable paths:
_generate/gen.go, simplify thesafe boolout ofdecodeSyncAsm's dispatch, drop the dead code inseqdec_asm.go. Mechanical, no behavior change from what ships today.executeSimple's analogous path stayed dynamically toggled.Happy to send a PR either way — let me know which you'd prefer.