fix: implement deallocation for fixed-length lists with heap elements#1538
fix: implement deallocation for fixed-length lists with heap elements#1538sumleo wants to merge 1 commit intobytecodealliance:mainfrom
Conversation
sumleo
force-pushed
the
fix/fixed-length-list-dealloc
branch
2 times, most recently
from
6f79a2d to
f7b5b06
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! For testing this, could this be added to a tests/runtime/* test?
sumleo
force-pushed
the
fix/fixed-length-list-dealloc
branch
from
f7b5b06 to
17933c5
Compare
|
Done. Extended the existing
This exercises both the |
sumleo
force-pushed
the
fix/fixed-length-list-dealloc
branch
from
17933c5 to
9b20147
Compare
crates/rust/tests/codegen.rs
Outdated
| "expected cabi_post_ deallocation function for fixed-length list \ | ||
| with string elements, but none was generated" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Mind dropping this test as well? This should be exercised by tests/runtime/fixed-length-lists and avoids trying to peek into the generated code like this
There was a problem hiding this comment.
Done. Removed the codegen test — the runtime tests now cover this with actual allocation tracking.
|
I think this Update: While the test for the new functionality is (incorrectly) tied to Rust, this could also fix the C++ code because of the change in |
| assert_eq!(result, ([2.0, -42.0], [0.25, -0.125])); | ||
| } | ||
| { | ||
| let _guard = Guard::new(); |
There was a problem hiding this comment.
All these allocation guards test for a no-op (as there must be no allocations for any of the tested types), but they don't hurt either.
The list<string,3> test would become interesting with a guard though.
There was a problem hiding this comment.
Good point. Added list<string, 3> test functions (string-list-param, string-list-result, string-list-roundtrip) with Guards that actually exercise deallocation of heap-allocated elements.
sumleo
force-pushed
the
fix/fixed-length-list-dealloc
branch
2 times, most recently
from
a14be8c to
922c9a7
Compare
Fixed three bugs in the handling of FixedLengthList types:
1. `deallocate` in abi.rs was `todo!()`, causing a panic when flat
deallocation was needed (e.g. `list<own<resource>, 3>`). Now uses
`flat_for_each_record_type` to iterate element types.
2. `deallocate_indirect` in abi.rs was a silent no-op `=> {}`, leaking
heap-allocated elements (e.g. strings in `list<string, 3>`). Now
iterates each element's memory offset and deallocates using
`deallocate_indirect_fields`.
3. `FixedLengthListLower` in the Rust code generator used array
indexing (`a[0]`, `a[1]`, ...) to extract elements, which fails for
non-Copy types like String. Changed to use array destructuring
(`let [e0, e1, ...] = a;`) to properly move elements out.
Added runtime tests with `list<string, 3>` functions (param, result,
roundtrip) and allocation tracking via Guard pattern to verify no
memory leaks when passing/returning fixed-length lists of strings.
sumleo
force-pushed
the
fix/fixed-length-list-dealloc
branch
from
922c9a7 to
d9baa75
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Summary
deallocatefunction in core ABI had atodo!()forFixedLengthList, panicking when flat deallocation was needed (e.g.,list<own<resource>, 3>)deallocate_indirectfunction was a silent no-op (=> {}), leaking memory for fixed-length lists containing heap-allocated elements like strings or nested listsflat_for_each_record_typewith repeated element types to iterate and deallocate each elementdeallocate_indirect_fieldswith a vec of repeated element types to correctly compute memory offsetsTest plan
cabi_post_deallocation functions are generated forlist<string, 3>exportsneeds_deallocatealready correctly returnstruefor fixed-length lists with complex elements — the fix ensures the deallocation instructions are actually emitted