fix(values): support Box and Nullable nested inside aggregates in to_ptr - #1656
Merged
Conversation
|
✅ Code is now correctly formatted. |
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 18, 2026 09:43
9bed290 to
c09a4f1
Compare
TomerStarkware
force-pushed
the
tomer/values_inline_to_ptr
branch
from
August 18, 2026 09:43
d046557 to
ab01fbe
Compare
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 18, 2026 15:39
c09a4f1 to
b7dd582
Compare
TomerStarkware
force-pushed
the
tomer/values_inline_to_ptr
branch
from
August 19, 2026 16:07
ab01fbe to
c534003
Compare
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 19, 2026 16:07
b7dd582 to
3d5c68c
Compare
orizi
approved these changes
Aug 20, 2026
orizi
left a comment
Collaborator
There was a problem hiding this comment.
@orizi reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
TomerStarkware
force-pushed
the
tomer/values_inline_to_ptr
branch
from
August 23, 2026 08:31
c534003 to
5d7a5d3
Compare
`Value::to_ptr` dispatched only on the `Value` variant, and `Box<T>` / `Nullable<T>` have no dedicated variant — so a box nested inside a struct, array, enum, or dict fell through to the payload's own arm and wrote the payload inline where the layout expects a single pointer, corrupting every following member. Only top-level boxes worked, via a dedicated `AbiArgument` arm. Handle both types by type before dispatching on the value: marshal the payload recursively, move it into the execution arena, and return a slot holding the payload pointer — the type's inline representation, matching what `from_ptr` reads. The `AbiArgument` Box/Nullable arms now delegate to this path and pass the pointer by value. This also replaces their `libc_malloc`, which leaked: boxes are arena-owned and never individually freed (see `crate::libfuncs::box`), so nothing ever released the malloc'd payload. Adds native execution tests for `Nullable<bool>` (null and both variants, exercising a memory-allocated payload) and a `Box<felt252>` nested in a struct argument, plus `to_ptr`->`from_ptr` round-trip unit tests for boxed bool, nullable bool, and a struct with a boxed member. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 23, 2026 10:33
3d5c68c to
e42ae5e
Compare
Benchmarking resultsBenchmark for program
|
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
8.600 ± 0.065 | 8.483 | 8.713 | 5.41 ± 0.07 |
cairo-native (embedded AOT) |
1.589 ± 0.016 | 1.565 | 1.613 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1.593 ± 0.015 | 1.568 | 1.615 | 1.00 ± 0.01 |
Benchmark for program dict_snapshot
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
449.2 ± 4.8 | 443.0 | 455.9 | 1.00 |
cairo-native (embedded AOT) |
1385.1 ± 15.0 | 1365.5 | 1408.0 | 3.08 ± 0.05 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1420.0 ± 21.9 | 1393.4 | 1450.5 | 3.16 ± 0.06 |
Benchmark for program factorial_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
3.682 ± 0.079 | 3.626 | 3.880 | 2.55 ± 0.06 |
cairo-native (embedded AOT) |
1.444 ± 0.008 | 1.430 | 1.456 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1.460 ± 0.013 | 1.434 | 1.478 | 1.01 ± 0.01 |
Benchmark for program fib_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
3.581 ± 0.011 | 3.555 | 3.595 | 2.55 ± 0.02 |
cairo-native (embedded AOT) |
1.412 ± 0.011 | 1.387 | 1.425 | 1.00 ± 0.01 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1.406 ± 0.008 | 1.397 | 1.420 | 1.00 |
Benchmark for program linear_search
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
481.1 ± 3.9 | 477.3 | 488.6 | 1.00 |
cairo-native (embedded AOT) |
1412.1 ± 14.4 | 1390.1 | 1429.6 | 2.94 ± 0.04 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1438.2 ± 16.6 | 1418.1 | 1467.4 | 2.99 ± 0.04 |
Benchmark for program logistic_map
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
412.7 ± 3.3 | 407.8 | 417.2 | 1.00 |
cairo-native (embedded AOT) |
1401.9 ± 15.2 | 1376.8 | 1427.6 | 3.40 ± 0.05 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1428.8 ± 15.3 | 1408.8 | 1456.2 | 3.46 ± 0.05 |
Benchmark results Main vs HEAD.Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
|
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.
Stacked on #1655.
Problem
Value::to_ptrdispatched only on theValuevariant, andBox<T>/Nullable<T>have no dedicated variant — so a box nested inside a struct, array, enum, or dict fell through to the payload's own arm and wrote the payload inline where the layout expects a single pointer, corrupting every following member. Only top-level boxes worked, via a dedicatedAbiArgumentarm.Separately, that arm allocated the payload with
libc_malloc, which leaked: boxes are arena-owned and never individually freed (seecrate::libfuncs::box), so nothing ever released the block.Fix
Handle both types by type before dispatching on the value: marshal the payload recursively, move it into the execution arena, and return a slot holding the payload pointer — the type's inline representation, matching
from_ptr's Box/Nullable arms.Value::Nullproduces a slot holding a null pointer.The
AbiArgumentBox/Nullable arms now delegate to this path and pass the pointer by value, removing the malloc leak and the Box/Nullabletype_idinconsistency.Tests
Nullable<bool>argument (null and both variants — a memory-allocated payload), and aBox<felt252>nested inside a struct argument.🤖 Generated with Claude Code
This change is