Repository navigation
Alignment: one rule for entrypoint arguments, strict eval test allocators, an LLVM alignment corpus, cache-reading and wasm hosted-call gaps closed - #12665
Merged
Conversation
The LLVM backend's host-facing wrapper packs an entrypoint's arguments into a buffer whose slots are each at least 8 bytes. The interpreter shim (marshalAbiArgs) and the dev backend's machine-code entry (computeEntrypointArgOffsets) read that buffer at each argument's plain size, so with --opt=interpreter, and with roc run --opt=dev, an entrypoint typed U32, U32 called with (7, 300) saw (7, 0). The slot rule now lives in src/layout/entrypoint_args.zig. The LLVM writer, both readers, and inspected_run's zeroed-buffer sizing all call it. test/int gains a U32, U32 entrypoint. The dev backend's offsets test gains a U32, U32 case, and the new module has unit tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t on free RuntimeHostEnv and CompileTimeHost aligned every block to 16 and ignored the alignment passed to dealloc. Production's tracking allocator finds a block's start from that alignment, so a mismatch there frees a pointer that is off by some bytes, and code that relies on more alignment than it asked for only breaks on hosts whose allocator differs. Both hosts now place each payload at exactly the requested alignment and never twice it, and panic when dealloc or realloc passes a different alignment than the allocation recorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
showell
force-pushed
the
alignment-hunt
branch
from
October 28, 2026 11:27
2fabf73 to
691a331
Compare
ModuleEnv.Serialized (method_idents, method_defs) and ExposedItems.Serialized embed SortedArrayBuilder.Serialized, but it declared no serialized_relocatable_pointers, so the cache validator walked into it as plain integers and never checked entries_offset. A cached module whose entries offset was misaligned, past the end of the buffer, or paired with an overflowing length was relocated and dereferenced unchecked. It now declares the marker, its SerializedElement, and a validateRelocations that goes through validateRelocatedSpan like SafeList.Serialized. Its serialize used appendSlice, which copies Entry's padding bytes verbatim and encodes the offset in a pointer. It now uses appendSlicePodZeroedOffset, as SafeList does, so the bytes are deterministic and the offset is an integer. The fixup count and layout fingerprint of ModuleEnv.Serialized change, so the module cache layout hash changes with them. A new test serializes a builder inside a composing header and checks that the validator rejects an offset off by one byte, a truncated buffer, and an overflowing length. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cache file pads its header so the data section keeps the 16-byte serialization alignment: 52 bytes of Header, data at 64. create wrote the data at the padded offset, but initFromBytes and fromMappedMemory checked the buffer's length against the unpadded @sizeof(Header) and then sliced at the padded one. A buffer between the two sizes passed the check and was sliced past its end ("index out of bounds: index 64, len 63" in a debug build; an out-of-bounds read in a release one). Header.data_offset is now the one size. create, initFromBytes and fromMappedMemory all use it, and fromMappedMemory keeps the header that initFromBytes returns instead of re-checking the length itself. The only production caller is the snapshot tool's cache round-trip check; the cache manager does not use this header format. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
emitCAbiCall packs a hosted call's arguments into one stack slot,
most-aligned argument first at offset 0, but allocated that slot at a
literal alignment of 4. A record holding a Dec or I128 is passed
.indirect on wasm32, so the host received a pointer typed as 16-byte
aligned at an address that could be only 4-aligned.
allocHostedArgsSlot now allocates the slot at the largest argument
alignment, as computeHostedArgOffsets reports it from the layouts. The
frame base honors 16: every frame size is rounded to 16 and the initial
__stack_pointer is a page boundary.
A new test passes { Dec, U8 } after a 4-byte return slot, checks that
it is lowered .indirect, and checks that its pointer is 16-aligned.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y builds CompileTimeHost runs every compile's compile-time evaluation, and RuntimeHostEnv backs the REPL, glue and the playground, so both ship in release builds. design.md rules out release-build runtime checks for compiler invariants, so the alignment-mismatch panic on dealloc/realloc now runs only when std.debug.runtime_safety is on, as the stamp and live-set checks already did. The payload skew (exactly `alignment`, never twice it) is a test trap, so release builds place the payload right after the header region instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Anton-4-bot
force-pushed
the
alignment-hunt
branch
from
October 28, 2026 17:17
691a331 to
e8e928a
Compare
# Conflicts: # src/compile/cache_config.zig # src/compile/cache_versioning.zig
Anton-4
approved these changes
Oct 30, 2026
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
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.
For a reviewer, human or agent. These are six independent commits from one hunt for alignment bugs, which began after #12641 (now merged). Review them one at a time: each stands alone, and each has its own evidence below.
The hunt, and what it found
#12641's bug had one shape. The alignment was known in one place (the string store's
strings.alignment(id)), but one of four places that rebuild the same blob image used a stand-in (word_size) instead of asking. We went looking for that shape across the compiler.How. Four read-only surveys covered four areas: static data, layout and code generation, the runtime and host boundary, and the compiler's own serialization and allocators. Each listed where alignment is decided, carried and applied, and named the places shaped like #12641. We then encoded the shapes as a reusable hunt in Shale, a small sandboxed scripting language we use with Claude for searching and changing code instead of ad-hoc shell. The hunt is
cookbook/zig/align_hunt.shale. It reads Zig's tokens, so comments and strings don't count, and it judges whole functions where that matters. It has seven named checks, each with a planted example that must fire (self_test=1). Run from a Roc checkout:On
mainbefore #12641 merged, it read all 1,008 files in about 15 s, and it finds #12641's line with two separate checks:stand_in_alignmentflagsstatic_data.zig:1089;backing_without_alignmentflagsstringBacking.Its findings are questions for a reader, not verdicts. Most are fine, including the 35 wasm stack temporaries aligned at 8, where wasm alignment is only a hint. The README says how to install and run Shale.
What the compiler gets right. The strongest pattern carries the alignment with the bytes:
StaticDataExport.alignment, which every backend applies. The weak spots all share one shape: the same arithmetic copied, with each copy left to remember to ask. The blob header is built four times, the refcount header formula is repeated in the builtins, wasm codegen and two glue generators, and the entrypoint argument layout existed in five copies. Those five had already drifted apart, which is commit 1.Why the tests missed #12641.
run-test-eval, and the full LLVM lane runs only nightly. Optimized LLVM is the only backend that turns a misaligned 16-byte value into a crash, through aligned vector loads. Commit 3 addresses that.Commit 1: read entrypoint arguments at the offsets the LLVM wrapper writes them
Bug. When a platform entrypoint takes several arguments,
roc build --opt=interpreter(androc run --opt=dev) can hand it the wrong values. Called with(7, 300), an entrypoint typedU32, U32sees(7, 0);U8, U16and{ x : U32, y : U32, z : U32 }, U32are wrong too.roc build --opt=devand--opt=speedget them right.Cause. The host calls an LLVM-generated wrapper. The wrapper packs the arguments into a zeroed buffer, giving each one a slot of at least 8 bytes (
entrypointParamSlotSize), and passes the buffer toroc_entrypoint. Two readers then lay the buffer out by plain size instead:marshalAbiArgs;computeEntrypointArgOffsets, behindroc run --opt=dev.So the second
U32is written at offset 8 and read at offset 4, which is zeroed padding. Arguments whose sizes are multiples of 8 line up by accident, which is whytest_mixed_args : Bool, I64passes.Fix. The rule now lives in one place,
src/layout/entrypoint_args.zig:slotSize, moved unchanged from LLVM;computeOffsets; andbufferSize. Four places use it:inspected_run's buffer sizing, a fourth copy that hard-coded 24 forStrand lacked the 16-byte vector slot.Three look-alikes are left alone, because they lay out different things: wasm's hosted-call arguments, dev's own stack frame, and LLVM's return buffer.
Tests.
test/intgainstest_two_u32_args : U32, U32, called with(7, 300). Red without the fix,first=7, second=0(run); green with it (run).U32, U32case, which coversroc run --opt=dev. Red without the fix, expecting{0, 8}and finding{0, 4}(run); green with it (run).Commit 2: eval test hosts hand out exactly the requested alignment, and check it on free
What was kind.
RuntimeHostEnvandCompileTimeHostaligned every block to 16, whatever Roc asked for, and theirroc_deallocignored its alignment argument. Production'ssrc/host_alloc/tracking.zigfinds a block's start from that argument. So an allocation at 16 freed at 8 frees a pointer 8 bytes off in production, and no eval test could notice.Change.
alignment(mod2*alignment) for 1 to 16.roc_dealloc/roc_reallocpanic, giving both values, when the alignment passed in differs.No new test and no new CI step: every existing eval test now also checks this.
One existing test changed.
compile_time_host.zig's "compile-time allocations honor alignment, keep their bytes across reallocation…" allocated at alignment 1, reallocated at 8, then shrank at 1. The new check refuses that, and so would production in general.host_alloc/tracking.zig'sreallocfinds a block's start from the alignment it is handed. 1 and 8 happen to reserve the same 8-byte prefix on 64-bit; at 16 it would free the wrong address. The test now grows and shrinks at one alignment (8), still checking that the bytes survive both ways. The cost is one comparison per free, and at most 16 bytes per test allocation.Results. No allocation in the eval suite frees or reallocates at an alignment other than the one it was given. The whole suite (
run-test-eval -Dtest-filter=e) gives the same counts with and without this commit: 2,594 passed, 0 failed, 0 crashed, 47 skipped.--llvmturned on for the whole suite, a measurement-only change that is not part of this PR: run. The counts are the same, and LLVM ran 2,429 of the tests.The two plain runs ran at the same time on the same base. The timings match within 0.2%, so the check costs nothing measurable.
Commit 3: a small LLVM alignment corpus on every pull request
Change. Twelve eval cases named
alignment corpus: ...(src/eval/test/eval_alignment_corpus_tests.zig) each put a 16-byte value (Dec,I128orU128) where alignment matters, and check the result:U8beside one;Box;A new step,
run-test-eval-llvm-alignment(--llvm --threads 1 --filter "alignment corpus:"), is a twin ofrun-test-eval-llvm-float-bits. MiniCI lists it right after that step, so it runs on every PR with no new CI job.Cost. The twelve cases take about 18-24 s under LLVM on GitHub's runner, against about 75 minutes for the nightly LLVM lane.
Results. All 12 pass on
mainbefore #12641 merged (run), and with #12641 applied (run).Note what it is and isn't. The corpus did not reproduce #12641 on unfixed
main. In the eval runner, the unannotatedDeclist happens to land aligned, so #12641's own test inhoisted_constants_test.zigstays the regression test for that bug. The corpus is cheap insurance: optimized LLVM sees each 16-byte case on every PR, not only nightly.Commit 4: the cache validator checks
SortedArrayBuilder.Serialized's offsetGap.
ModuleEnv.Serialized(inmethod_identsandmethod_defs) andExposedItems.SerializedembedSortedArrayBuilder.Serialized. But it declared noserialized_relocatable_pointers, so the cache validator walked into it as plain integers. As a result,entries_offsetwas never checked for alignment or bounds beforedeserializeIntodereferenced it. A corrupt cached module could therefore be relocated through a misaligned or out-of-range offset.Fix. It now follows
SafeList.Serializedexactly:SerializedElement = Entry;validateRelocationsthat goes throughvalidateRelocatedSpan, with the length multiplication checked for overflow;appendSlicePodZeroedOffseton the write side, soEntry's padding is zeroed (deterministic bytes) and the offset is stored as an integer.The write side was already aligned; the gap was only in validation. The layout fingerprint of
ModuleEnv.Serializedchanges, so existing module caches are invalidated, which is intended. As themodule_env_layouttripwire asks for an intentional change,CACHE_VERSIONgoes from 161 to 162 and the golden bytes are replaced.Test. A new test serializes a builder inside a header that embeds it, as
ModuleEnvdoes. The untouched buffer passes, and three corruptions are rejected: an offset 1 byte off, a buffer truncated by 1 byte, and an overflowing length. Without the fix:expected error.CorruptArtifact, found void. With it,run-test-zig-module-collectionspasses 99/99 andrun-test-zig-module-can, which buildsModuleEnvon the new write path, passes 279/279.Commit 5:
cache_module.zigchecks and slices at one header sizeBug.
@sizeOf(Header)is 52, butcreatewrites the data at the padded offset, 64, to keep the 16-byte serialization alignment.initFromBytesandfromMappedMemoryboth checked the buffer's length against 52 plus the data size, then sliced from 64. So a buffer between the two sizes passed the check and was sliced past its end. Its only production caller is the snapshot tool's round-trip check; the cache manager doesn't use this header format.Fix.
Header.data_offsetis now the one size, andcreate,initFromBytesandfromMappedMemoryall use it.fromMappedMemorykeeps the headerinitFromBytesreturns instead of repeating its own length checks.Test. A valid header claiming no data, in a buffer one byte short of
data_offset, must giveerror.BufferTooSmall. Without the fix:panic: index out of bounds: index 64, len 63. With it, the test passes underrun-test-zig-module-compile.Commit 6: wasm aligns a hosted call's argument slot to its most-aligned argument
Gap.
emitCAbiCallpacks a hosted call's arguments into one stack slot, most-aligned first at offset 0. But it allocated that slot at a literal alignment of 4. On wasm32, a record holding aDecorI128is passed.indirect, so the host received a pointer typed as 16-byte-aligned at an address that might be only 4-aligned. Wasm doesn't fault on that, but it breaks the guarantee of the host's own type, and a host that checks alignment would object.Fix.
computeHostedArgOffsetsnow also reports the arguments' largest alignment, taken from their layouts. A newallocHostedArgsSlotallocates the slot at that alignment, andemitCAbiCalluses it. The frame base does honor 16: every frame size is rounded up to 16, and the initial__stack_pointeris a page boundary.These nearby sites are left alone, because they already ask the layout or hold only 4-byte values:
Test.
{ Dec, U8 }is passed after a 4-byte return slot, asemitCAbiCallallocates one. The test checks that the record is lowered.indirect, then that its pointer is 16-aligned. Without the fix:expected 0, found 4. With it, the test passes, and all 173 wasm tests in the backend module pass.Note for running it:
WasmCodeGen.zig's tests compile only when the filter also names the module's aggregator test. Use-Dtest-filter="backend tests" -Dtest-filter="args slot is aligned"; the second filter alone matches nothing and reports success.How it was found
Claude (Anthropic's model) ran the hunt, reproduced and fixed the entrypoint bug, and wrote all six commits, working for me (Steve Howell).
Checked and left alone. The 22 wasm stack temporaries allocated at
(16, 8)or(17, 8)inWasmCodeGen.zig. Every consumer needs at most 8-byte alignment:dev_wrappers.zigtake 128-bit values as twou64halves, and write results through*u64or byte by byte;The check of the wasm temporaries is what turned up commit 6.
🤖 Generated with Claude Code