From 5d7a5d31534a5d2f850abd186645716f83e3f268 Mon Sep 17 00:00:00 2001 From: TomerStarkware Date: Tue, 18 Aug 2026 10:11:08 +0300 Subject: [PATCH] fix(values): make Value::to_ptr always return the inline representation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For memory-allocated types (>=2-variant enums and aggregates containing one), `Value::to_ptr` returned a *wrapper* pointer (a slot holding the data pointer) while `Value::from_ptr` always reads the inline representation. Every recursive consumer had to un-box the wrapper, and four of six forgot: - the Array arm copied `elem_layout.size()` bytes out of the 8-byte wrapper, so even `Array` was corrupted (the 16-byte-aligned enum body keeps the tag bit clear, decoding every element as variant 0); - the Felt252Dict arm had the same omission (`Felt252Dict`); - the `AbiArgument` Box and Nullable arms memcpy'd the wrapper bytes into the heap block (`Box`). Enumerating all callers shows nothing consumes the wrapper — every call site either stripped it immediately or was one of these bugs — so invert the contract instead of patching each site: `to_ptr` now always returns a pointer to the inline representation per `TypeBuilder::layout()`, exactly what `from_ptr` reads, and the by-pointer ABI decision lives only in `crate::arch`'s `AbiArgument` impl. Also make the Felt252Dict arm follow the same convention: it returned the `FeltDict*` itself instead of a slot holding it, so a dict nested in an aggregate copied 8 bytes of HashMap internals instead of the dict pointer. The arch.rs dict arm now dereferences the slot once. The Nullable arm also now passes the payload type id like the Box arm does. Only reachable via the `invoke_dynamic(&[Value])` API; the Starknet contract path marshals felts directly and is unaffected. Adds a VM-vs-native regression test for `Array` and `to_ptr`->`from_ptr` round-trip unit tests (bool array, struct with bool, nested enum, bool dict) that the new symmetric contract enables. Co-Authored-By: Claude Fable 5 --- src/arch.rs | 21 ++-- src/values.rs | 150 ++++++++++++++++++------ test_data/programs/array_bool_arg.cairo | 9 ++ tests/tests/enums.rs | 52 ++++++++ 4 files changed, 185 insertions(+), 47 deletions(-) create mode 100644 test_data/programs/array_bool_arg.cairo diff --git a/src/arch.rs b/src/arch.rs index 45a69b535..7bc13fe9b 100644 --- a/src/arch.rs +++ b/src/arch.rs @@ -15,7 +15,7 @@ use cairo_lang_sierra::{ ids::ConcreteTypeId, program_registry::ProgramRegistry, }; -use std::ptr::{null, NonNull}; +use std::ptr::null; mod aarch64; mod x86_64; @@ -76,7 +76,7 @@ impl AbiArgument for ValueWithInfoWrapper<'_> { if matches!(value, Value::Null) { null::<()>().to_bytes(buffer)?; } else { - let ptr = value.to_ptr(self.arena, self.registry, self.type_id)?; + let ptr = value.to_ptr(self.arena, self.registry, &info.ty)?; let layout = self.registry.get_type(&info.ty)?.layout(self.registry)?; let heap_ptr = unsafe { @@ -126,9 +126,9 @@ impl AbiArgument for ValueWithInfoWrapper<'_> { } (Value::Enum { tag, value, .. }, CoreTypeConcrete::Enum(info)) => { if self.info.is_memory_allocated(self.registry)? { + // Memory-allocated types are passed by pointer to their inline + // representation. let abi_ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?; - - let abi_ptr = unsafe { *abi_ptr.cast::>().as_ref() }; abi_ptr.as_ptr().to_bytes(buffer)?; } else { match info @@ -158,10 +158,11 @@ impl AbiArgument for ValueWithInfoWrapper<'_> { (Value::Felt252Dict { .. }, CoreTypeConcrete::Felt252Dict(_)) => { // TODO: Assert that `info.ty` matches all the values' types. - self.value - .to_ptr(self.arena, self.registry, self.type_id)? - .as_ptr() - .to_bytes(buffer)? + let ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?; + + // The dict's inline representation is a slot holding the `FeltDict` + // pointer; the ABI passes that pointer by value. + unsafe { *ptr.cast::<*mut ()>().as_ref() }.to_bytes(buffer)? } ( Value::Secp256K1Point(Secp256k1Point { x, y, is_infinity }), @@ -186,9 +187,9 @@ impl AbiArgument for ValueWithInfoWrapper<'_> { (Value::Sint8(value), CoreTypeConcrete::Sint8(_)) => value.to_bytes(buffer)?, (Value::Struct { fields, .. }, CoreTypeConcrete::Struct(info)) => { if self.info.is_memory_allocated(self.registry)? { + // Memory-allocated types are passed by pointer to their inline + // representation. let abi_ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?; - - let abi_ptr = unsafe { *abi_ptr.cast::>().as_ref() }; abi_ptr.as_ptr().to_bytes(buffer)?; } else { fields diff --git a/src/values.rs b/src/values.rs index 02fb27268..916779fe4 100644 --- a/src/values.rs +++ b/src/values.rs @@ -157,6 +157,11 @@ impl Value { } /// Allocates the value in the given arena so it can be passed to the JIT engine or a compiled program. + /// + /// Invariant: the returned pointer always points to the value's inline representation as + /// given by [`TypeBuilder::layout`], which is exactly what [`Value::from_ptr`] reads. The + /// ABI decision of passing memory-allocated types by pointer is made by the `AbiArgument` + /// impl in `crate::arch`, not here. pub(crate) fn to_ptr( &self, arena: &Bump, @@ -269,7 +274,6 @@ impl Value { let mut layout: Option = None; let mut data = Vec::with_capacity(info.members.len()); - let mut is_memory_allocated = false; for (member_type_id, member) in info.members.iter().zip(members) { let member_ty = registry.get_type(member_type_id)?; let member_layout = member_ty.layout(registry)?; @@ -281,19 +285,7 @@ impl Value { layout = Some(new_layout); let member_ptr = member.to_ptr(arena, registry, member_type_id)?; - data.push(( - member_layout, - offset, - if member_ty.is_memory_allocated(registry)? { - is_memory_allocated = true; - - // Undo the wrapper pointer added because the member's memory - // allocated flag. - *member_ptr.cast::>().as_ref() - } else { - member_ptr - }, - )); + data.push((member_layout, offset, member_ptr)); } let ptr = arena @@ -308,12 +300,7 @@ impl Value { ); } - if is_memory_allocated { - // alloc returns a ref, so its never null - NonNull::new_unchecked(arena.alloc(ptr) as *mut _).cast() - } else { - NonNull::new_unchecked(ptr).cast() - } + NonNull::new_unchecked(ptr).cast() } else { Err(Error::UnexpectedValue(format!( "expected value of type {:?} but got a struct", @@ -327,18 +314,8 @@ impl Value { native_assert!(*tag < info.variants.len(), "Variant index out of range."); let payload_type_id = &info.variants[*tag]; - let payload_ty = registry.get_type(payload_type_id)?; let payload = value.to_ptr(arena, registry, payload_type_id)?; - // Undo the wrapper pointer added when the payload is memory - // allocated (e.g. a nested >=2-variant enum), so that the copy - // below reads the payload data rather than the wrapper pointer. - let payload = if payload_ty.is_memory_allocated(registry)? { - *payload.cast::>().as_ref() - } else { - payload - }; - let (layout, tag_layout, variant_layouts) = crate::types::r#enum::get_layout_for_variants( registry, @@ -362,12 +339,7 @@ impl Value { variant_layouts[*tag].size(), ); - if resolved_ty.is_memory_allocated(registry)? { - // alloc returns a reference so its never null - NonNull::new_unchecked(arena.alloc(ptr) as *mut _).cast() - } else { - NonNull::new_unchecked(ptr).cast() - } + NonNull::new_unchecked(ptr).cast() } else { Err(Error::UnexpectedValue(format!( "expected value of type {:?} but got an enum value", @@ -417,7 +389,10 @@ impl Value { ); } - NonNull::new_unchecked(dict_ptr as *mut ()).cast() + // The dict's inline representation is a single pointer to the + // `FeltDict`, so return a slot holding that pointer. + // alloc returns a reference so its never null + NonNull::new_unchecked(arena.alloc(dict_ptr) as *mut _).cast() } else { Err(Error::UnexpectedValue(format!( "expected value of type {:?} but got a felt dict", @@ -1766,6 +1741,107 @@ mod test { _ => panic!("Unexpected error type: {:?}", result), } } + + /// Helper for the round-trip tests below: a `bool` value (a 2-variant enum + /// of unit structs, hence memory-allocated). + fn bool_value(value: bool) -> Value { + Value::Enum { + tag: value as usize, + value: Box::new(Value::Struct { + fields: Vec::new(), + debug_name: None, + }), + debug_name: None, + } + } + + /// `to_ptr` returns a pointer to the inline representation, which is + /// exactly what `from_ptr` reads — so any value must round-trip. + fn assert_roundtrip(program_src: &str, type_idx: usize, value: Value) { + let program = ProgramParser::new().parse(program_src).unwrap(); + let registry = ProgramRegistry::::new(&program).unwrap(); + let type_id = &program.type_declarations[type_idx].id; + + let arena = Bump::new(); + let ptr = value.to_ptr(&arena, ®istry, type_id).unwrap(); + let result = Value::from_ptr(ptr, type_id, ®istry).unwrap(); + + assert_eq!(result, value); + } + + #[test] + fn test_roundtrip_bool_array() { + assert_roundtrip( + "type Unit = Struct; + type bool = Enum; + type BoolArray = Array;", + 2, + Value::Array(vec![ + bool_value(true), + bool_value(false), + bool_value(true), + bool_value(true), + ]), + ); + } + + #[test] + fn test_roundtrip_struct_with_bool() { + assert_roundtrip( + "type u8 = u8; + type Unit = Struct; + type bool = Enum; + type MyStruct = Struct;", + 3, + Value::Struct { + fields: vec![Value::Uint8(123), bool_value(true)], + debug_name: None, + }, + ); + } + + #[test] + fn test_roundtrip_nested_enum() { + let program_src = "type felt252 = felt252; + type Inner = Enum; + type Outer = Enum;"; + + for (outer_tag, inner_tag) in [(0, 0), (0, 1), (1, 0), (1, 1)] { + assert_roundtrip( + program_src, + 2, + Value::Enum { + tag: outer_tag, + value: Box::new(Value::Enum { + tag: inner_tag, + value: Box::new(Value::Felt252(Felt::from(0x1234))), + debug_name: None, + }), + debug_name: None, + }, + ); + } + } + + #[test] + fn test_roundtrip_bool_dict() { + assert_roundtrip( + "type Unit = Struct; + type bool = Enum; + type BoolDict = Felt252Dict;", + 2, + Value::Felt252Dict { + value: [ + (Felt::from(0), bool_value(true)), + (Felt::from(1), bool_value(false)), + (Felt::from(2), bool_value(true)), + ] + .into_iter() + .collect(), + debug_name: None, + }, + ); + } } mod range_serde { diff --git a/test_data/programs/array_bool_arg.cairo b/test_data/programs/array_bool_arg.cairo new file mode 100644 index 000000000..db7ab661a --- /dev/null +++ b/test_data/programs/array_bool_arg.cairo @@ -0,0 +1,9 @@ +fn run_test(data: Array) -> u32 { + let mut count: u32 = 0; + for value in data { + if value { + count += 1; + } + } + count +} diff --git a/tests/tests/enums.rs b/tests/tests/enums.rs index 8720ea936..dbdb32d0c 100644 --- a/tests/tests/enums.rs +++ b/tests/tests/enums.rs @@ -99,3 +99,55 @@ fn nested_enum_argument_matches_vm() { }); } } + +#[test] +fn bool_array_argument_matches_vm() { + // `bool` is a 2-variant enum, so it is memory-allocated and `to_ptr` must + // un-wrap each element before copying it into the array's data buffer. The + // program counts the `true` elements, so any corruption changes the count. + let program = &load_program_and_runner("programs/array_bool_arg"); + + // The VM tag of a 2-variant enum equals the variant index. + let values = [true, false, true, true, false]; + + let result_vm = run_vm_program( + program, + "run_test", + vec![Arg::Array( + values + .iter() + .map(|&v| Arg::Value(Felt::from(v as u64))) + .collect(), + )], + Some(DEFAULT_GAS as usize), + ) + .unwrap(); + + let result_native = run_native_program( + program, + "run_test", + &[Value::Array( + values + .iter() + .map(|&v| Value::Enum { + tag: v as usize, + value: Box::new(Value::Struct { + fields: Vec::new(), + debug_name: None, + }), + debug_name: None, + }) + .collect(), + )], + Some(DEFAULT_GAS), + Option::::None, + ); + + compare_outputs( + &program.1, + &program.2.find_function("run_test").unwrap().id, + &result_vm, + &result_native, + ) + .expect("bool array argument must agree between VM and native"); +}