Copy composite elements into aggregate literals - #46
Conversation
Composite values (closures, structs, arrays, slices) are represented by
the address of their storage. Every backend's aggregate-literal codegen
stored that address as if it were the element value, so an array of
lambdas wrote pointers to temporaries — or, on the register VM, the low
32 bits of one — into 16-byte element slots instead of copying the fat
closure pointers in. Reading an element back then interpreted garbage as
a function index: segfaults on the Cranelift and arm64 backends, an
out-of-bounds panic on the VM, a failed assert on the stack backend.
Fix the element-store path in each backend:
- jit: new store_element/is_indirect; memcpy indirect elements instead
of stack_store, in array literals, array fills, and tuples. f32x4
stays a SIMD register value.
- vm_codegen: emit_store_offset emits IAddImm + MemCopy for pointer
types, matching what emit_store already did without an offset.
- stack_codegen: replace the TODO/HACK block that fell through to an
offset-less MemCopy. MemCopy has no offset operand, so the new
emit_dest_addr folds the offset into the destination address before
the value is pushed.
- llvm_jit: new store_element, used by array literals, array fills,
tuples, and struct literals.
Two more cases fall out of the same defect: nested array literals
([[1,2],[3,4]]) were broken on every backend, and a struct literal with
a function-typed field (Op(f: add_one)) panicked the register VM.
Fixes #45.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa
Three follow-ups from review of the composite-element fix. The register VM's new composite-store path computed `base + offset` with IAddImm, whose immediate is packed as an i8 with no WIDE fallback, so any element or field past byte offset 127 was written to the wrong address — `[P(a: 7, b: 8); 20]` read back garbage at index 16, and a struct literal with a field past offset 160 smashed the frame. Route every byte-offset address computation in vm_codegen through emit_offset_addr, which materializes the constant when it doesn't fit, and add a debug assertion to the encoder so the truncation can't come back silently. This also fixes the same latent bug in emit_load_offset's Bool/Int8/UInt8 path, which was reachable before this branch. f32x4 satisfies is_ptr() but rides in a register rather than as an address, so aggregate member access has to ask "is this indirect?". jit.rs grew is_indirect for that in the previous commit but left the struct-literal store and the field/element read sites on raw is_ptr(), which handed a SIMD value to gen_copy as a source address and tripped the Cranelift verifier. Promote is_indirect to a free function, use it at all four sites, and fold the two hand-rolled copies of the same rule (returns_via_pointer, the globals path) into it. Mirror the whole thing in llvm_jit, which had the identical divergence. f32x4 inside a struct or array now works on all five backends; it previously panicked the Cranelift backend and mis-read on LLVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa
|
Follow-up commit b09f490 addresses all three review findings. 1 & 2 — Fixed in codegen rather than the encoder: an encoder-level rewrite to Adding an 3 — The reviewer flagged the read side as out of scope, but fixing only the store would have turned a loud verifier panic into a quieter wrong answer, so both sides are done: Tests. Not changed: |
1 participant
Fixes #45.
Root cause
Composite values (closures, structs, arrays, slices) are represented by the address of their storage. Every backend's aggregate-literal codegen stored that address as if it were the element value, so
[(|i: i32| x + 1.0), (|i: i32| x * 3.0)]wrote pointers-to-temporaries — or, on the register VM, the low 32 bits of one — into 16-byte element slots instead of copying the fat closure pointers in. Reading an element back then interpreted garbage as a function index.Calling the same lambda through a plain local variable works because
varinitialization already goes through a proper copy; only aggregate construction was storing the pointer.Changes
One per backend, all in the element-store path:
src/jit.rs— newstore_element/is_indirect;memcpyfor indirect elements instead ofstack_store, used byArrayLiteral,Arrayfill, andTuple.Float32x4stays a SIMD register value rather than becoming pointer-represented.src/vm_codegen.rs—emit_store_offsetemitsIAddImm+MemCopyfor pointer types, matching whatemit_storealready did in the no-offset case.src/stack_codegen.rs— replaces theTODO/HACKblock that fell through to an offset-lessMemCopy.MemCopyhas no offset operand, so the newemit_dest_addrfolds the offset into the destination address before the value is pushed; the four aggregate-literal callers use it.src/llvm_jit.rs— newstore_element, used by array literals, array fills, tuples, and struct literals.Two more cases fall out of the same defect and are fixed here too:
[[1, 2], [3, 4]]) were wrong on every backendOp(f: add_one)) panicked the register VM with the same out-of-bounds indexThe stack backend's float argument/return bridging that #45 also mentions was already fixed by #43; this branch is based on that.
Tests
tests/cases/lambdas/lambda_array.lyte— the issue's repro, the f32-parameter variant, and a fill-array of closures. This reaches the indirect-expressionCallClosuresite that Bridge f32/f64 across closure calls on the stack backend #43's test could not.tests/cases/arrays/array_literal_composite.lyte— struct elements and nested-array elements, literal and fill.tests/cases/lambdas/func_ref_struct.lyte— adds the struct-literal form alongside the existing field-assignment form../test.shpasses: 382 lib + 8 CLI (golden suite across jit / vm / asm / stack / llvm) + 21 lsp, 0 failures.🤖 Generated with Claude Code
https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa