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"); +}