ReviewOS

also looking at this

zig-utils/zig-js

fix(lexer): size the SIMD identifier scan by lane count, not byte size

#625
Merged glennmichael123 wants to merge fix/vector-lane-count-not-sizeof into main
1 file +10 -3
9 days ago

consumeAsciiIdentPart drove its block loop off @sizeOf(Block), where Block = @Vector(8, u8). A vector is padded up to its alignment, so that value is 8 on aarch64 and 16 on x86_64 — it agreed with the lane count on one target and not the other.

On x86_64 this doesn't miscompile, it doesn't compile at all. Slicing 16 bytes to coerce into an 8-lane vector is:

src/lexer.zig:332:71: error: expected type '@Vector(8, u8)', found '[16]u8'

So every x86_64 consumer is currently broken. It went unnoticed because development happens on arm64, where the two numbers coincide. It surfaced downstream in craft, whose zig-core (ubuntu-latest) and Benchmark jobs both fail here — zig-js is a path dependency cloned at HEAD, so this is the only thing standing between those two workflows and green.

The change

Takes the count from the type, so the loop bound, the slice width and the index advance can't drift from the vector again:

const lanes = @typeInfo(Block).vector.len;

Behaviour on aarch64 is unchanged by construction — 8 was already the value in use there.

Verified

Analyze-only test compiles against both targets on 0.17.0-dev.1509+bb296ab9b:

targetbeforeafter
x86_64-linux-gnuexpected type '@Vector(8, u8)', found '[16]u8'clean
aarch64-macoscleanclean

All 21 lexer tests pass on aarch64, and zig fmt --check is clean.

Worth a follow-up

There's no x86_64 leg in CI here, which is why a total build failure on that architecture could sit unnoticed. A cross-compile-only job (zig build -Dtarget=x86_64-linux-gnu, no execution needed) would have caught this at the cost of one compile.

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.