Skip to content

Commit

Permalink
Fix clone and drop implementations for enums, structs and snapshots (#…
Browse files Browse the repository at this point in the history 
…815)

* Fix snapshot clones. Extract if statement from function for better readability.

* Fix broken MLIR bug.

* Fix snapshot dropping.

* Implement clone and drop for structs.

* Fix stuff.

* Implement enum custom snapshot_take.

* Fix stuff.

* free array

* Fix stuff.

* Fix stuff.

* Fix stuff.

* Fix compile error.

* Fix more leaks.

* Fix bug.

* Fix stuff.

* Fix stuff.

* Fix infinite recursion when registering composite types' clone implementations.

* Add some debug prints.

* Debug.

* Undo debug.

* Rename `SnapshotClonesMeta` to `DupOverrideMeta`.

* Completely refactor value duplication logic.

* Add some documentation on `DupOverridesMeta`.

* Completely refactor value dropping logic.

* Remove commented code.

* Update docs.

* Update unwrap comments.

* Add note to `Option<_>` in FFI interface.

* Fix `array_get` memory leak. Fix bug in nullables.

* Fix stuff.

* Fix clippy issues.

---------

Co-authored-by: Edgar Luque <[email protected]>
  • Loading branch information
@azteca1998 @edg-l
azteca1998 and edg-l authored Oct 3, 2024
1 parent 1b8656c commit 79a8dcf
Show file tree
Hide file tree
Showing 23 changed files with 1,693 additions and 815 deletions.
32 changes: 15 additions & 17 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ required-features = ["scarb"]
[features]
default = ["build-cli", "with-runtime"]
build-cli = [
"dep:clap",
"dep:tracing-subscriber",
"dep:anyhow",
"dep:cairo-lang-test-plugin",
"dep:cairo-lang-runner",
"dep:colored",
"dep:clap",
"dep:tracing-subscriber",
"dep:anyhow",
"dep:cairo-lang-test-plugin",
"dep:cairo-lang-runner",
"dep:colored",
]
scarb = ["build-cli", "dep:scarb-ui", "dep:scarb-metadata"]
with-debug-utils = []
Expand All @@ -70,16 +70,14 @@ itertools = "0.13.0"
lazy_static = "1.4"
libc = "0.2"
llvm-sys = "191.0.0"
melior = { version = "0.19.0", features = [
"ods-dialects",
] }
mlir-sys = { version = "0.3.0"}
melior = { version = "0.19.0", features = ["ods-dialects"] }
mlir-sys = { version = "0.3.0" }
num-bigint = "0.4.4"
num-traits = "0.2"
starknet-types-core = { version = "0.1.5", default-features = false, features = [
"std",
"serde",
"num-traits",
"std",
"serde",
"num-traits",
] }
tempfile = "3.6"
thiserror = "1.0.64"
Expand All @@ -97,9 +95,9 @@ cairo-native-runtime = { version = "0.2.0-alpha.2", path = "runtime", optional =
clap = { version = "4.5.18", features = ["derive"], optional = true }
libloading = "0.8.3"
tracing-subscriber = { version = "0.3.18", features = [
"env-filter",
"json",
"registry",
"env-filter",
"json",
"registry",
], optional = true }
serde = { version = "1.0", features = ["derive"] }
anyhow = { version = "1.0", optional = true }
Expand All @@ -110,7 +108,7 @@ colored = { version = "2.1.0", optional = true }
keccak = "0.1.5"
k256 = "0.13.4"
p256 = "0.13.2"
sha2 = "0.10.8" # needed for the syscall handler stub
sha2 = "0.10.8" # needed for the syscall handler stub
scarb-metadata = { version = "1.12.0", optional = true }
scarb-ui = { version = "0.1.5", optional = true }
sec1 = "0.7.3"
Expand Down
90 changes: 45 additions & 45 deletions runtime/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,73 +134,75 @@ pub unsafe extern "C" fn cairo_native__libfunc__hades_permutation(
}

/// Felt252 type used in cairo native runtime
pub type FeltDict = (HashMap<[u8; 32], NonNull<std::ffi::c_void>>, u64);
#[derive(Debug, Default)]
pub struct FeltDict {
pub inner: HashMap<[u8; 32], NonNull<std::ffi::c_void>>,
pub count: u64,
}

/// Allocates a new dictionary. Internally a rust hashmap: `HashMap<[u8; 32], NonNull<()>`
/// Allocate a new dictionary.
///
/// # Safety
///
/// This function is intended to be called from MLIR, deals with pointers, and is therefore
/// definitely unsafe to use manually.
#[no_mangle]
pub unsafe extern "C" fn cairo_native__alloc_dict() -> *mut std::ffi::c_void {
Box::into_raw(Box::<FeltDict>::default()) as _
pub unsafe extern "C" fn cairo_native__dict_new() -> *mut FeltDict {
Box::into_raw(Box::<FeltDict>::default())
}

/// Frees the dictionary.
/// Free a dictionary using an optional callback to drop each element.
///
/// # Safety
///
/// This function is intended to be called from MLIR, deals with pointers, and is therefore
/// definitely unsafe to use manually.
// Note: Using `Option<extern "C" fn(*mut std::ffi::c_void)>` is ffi-safe thanks to Option's null
// pointer optimization. Check out
// https://doc.rust-lang.org/nomicon/ffi.html#the-nullable-pointer-optimization for more info.
#[no_mangle]
pub unsafe extern "C" fn cairo_native__dict_free(ptr: *mut FeltDict) {
let mut map = Box::from_raw(ptr);
pub unsafe extern "C" fn cairo_native__dict_drop(
ptr: *mut FeltDict,
drop_fn: Option<extern "C" fn(*mut std::ffi::c_void)>,
) {
let map = Box::from_raw(ptr);

// Free the entries manually.
for (_, entry) in map.as_mut().0.drain() {
libc::free(entry.as_ptr().cast());
}
}
for entry in map.inner.into_values() {
if let Some(drop_fn) = drop_fn {
drop_fn(entry.as_ptr());
}

/// Needed for the correct alignment,
/// since the key [u8; 32] in rust has 8 byte alignment but its a felt,
/// so in reality it has 16.
#[repr(C, align(16))]
pub struct DictValuesArrayAbi {
pub key: [u8; 32],
pub value: std::ptr::NonNull<libc::c_void>,
libc::free(entry.as_ptr());
}
}

/// Returns a array over the values of the dict, used for deep cloning.
/// Duplicate a dictionary using a provided callback to clone each element.
///
/// # Safety
///
/// This function is intended to be called from MLIR, deals with pointers, and is therefore
/// definitely unsafe to use manually.
#[no_mangle]
pub unsafe extern "C" fn cairo_native__dict_values(
pub unsafe extern "C" fn cairo_native__dict_dup(
ptr: *mut FeltDict,
len: *mut u64,
) -> *mut DictValuesArrayAbi {
let dict: &mut FeltDict = &mut *ptr;
dup_fn: extern "C" fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void,
) -> *mut FeltDict {
let old_dict = &*ptr;
let mut new_dict = Box::<FeltDict>::default();

new_dict.inner.extend(
old_dict
.inner
.iter()
.map(|(&k, &v)| (k, NonNull::new(dup_fn(v.as_ptr())).unwrap())),
);

let values: Vec<_> = dict
.0
.clone()
.into_iter()
// make it ffi safe for use within MLIR.
.map(|x| DictValuesArrayAbi {
key: x.0,
value: x.1,
})
.collect();
*len = values.len() as u64;
values.leak::<'static>().as_mut_ptr()
Box::into_raw(new_dict)
}

/// Gets the value for a given key, the returned pointer is null if not found.
/// Increments the access count.
/// Return the value (reference) for a given key, or null if not present. Increment the access
/// count.
///
/// # Safety
///
Expand All @@ -212,13 +214,11 @@ pub unsafe extern "C" fn cairo_native__dict_get(
key: &[u8; 32],
) -> *mut std::ffi::c_void {
let dict: &mut FeltDict = &mut *ptr;
let map = &dict.0;
dict.1 += 1;
dict.count += 1;

if let Some(v) = map.get(key) {
v.as_ptr()
} else {
std::ptr::null_mut()
match dict.inner.get(key) {
Some(v) => v.as_ptr(),
None => std::ptr::null_mut(),
}
}

Expand All @@ -235,7 +235,7 @@ pub unsafe extern "C" fn cairo_native__dict_insert(
value: NonNull<std::ffi::c_void>,
) -> *mut std::ffi::c_void {
let dict = &mut *ptr;
let old_ptr = dict.0.insert(*key, value);
let old_ptr = dict.inner.insert(*key, value);

if let Some(v) = old_ptr {
v.as_ptr()
Expand All @@ -253,7 +253,7 @@ pub unsafe extern "C" fn cairo_native__dict_insert(
#[no_mangle]
pub unsafe extern "C" fn cairo_native__dict_gas_refund(ptr: *const FeltDict) -> u64 {
let dict = &*ptr;
(dict.1 - dict.0.len() as u64) * *DICT_GAS_REFUND_PER_ACCESS
(dict.count - dict.inner.len() as u64) * *DICT_GAS_REFUND_PER_ACCESS
}

/// Compute `ec_point_from_x_nz(x)` and store it.
Expand Down
108 changes: 99 additions & 9 deletions src/libfuncs/array.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
// TODO: A future possible improvement would be to put the array behind a double pointer and a
// reference counter, to avoid unnecessary clones.

use std::ops::Deref;

use super::LibfuncHelper;
use crate::{
error::Result,
metadata::{realloc_bindings::ReallocBindingsMeta, MetadataStorage},
metadata::{
drop_overrides::DropOverridesMeta, realloc_bindings::ReallocBindingsMeta, MetadataStorage,
},
types::TypeBuilder,
utils::{BlockExt, ProgramRegistryExt},
};
Expand All @@ -35,6 +35,7 @@ use melior::{
},
Context,
};
use std::ops::Deref;

/// Select and call the correct libfunc builder function from the selector.
pub fn build<'ctx, 'this>(
Expand Down Expand Up @@ -411,6 +412,7 @@ pub fn build_get<'ctx, 'this>(
let elem_ty = registry.get_type(&info.ty)?;
let elem_layout = elem_ty.layout(registry)?;
let elem_stride = elem_layout.pad_to_align().size();
let elem_ty = elem_ty.build(context, helper, registry, metadata, &info.ty)?;

let ptr_ty = crate::ffi::get_struct_field_type_at(&array_ty, 0);
let len_ty = crate::ffi::get_struct_field_type_at(&array_ty, 1);
Expand Down Expand Up @@ -445,12 +447,12 @@ pub fn build_get<'ctx, 'this>(
{
let ptr = valid_block.extract_value(context, location, value, ptr_ty, 0)?;

let array_start = valid_block.append_op_result(arith::extui(
array_start,
IntegerType::new(context, 64).into(),
location,
))?;
let index = {
let array_start = valid_block.append_op_result(arith::extui(
array_start,
IntegerType::new(context, 64).into(),
location,
))?;
let index = valid_block.append_op_result(arith::extui(
index,
IntegerType::new(context, 64).into(),
Expand Down Expand Up @@ -488,9 +490,91 @@ pub fn build_get<'ctx, 'this>(
"realloc returned nullptr",
)?;

// TODO: Support clone-only types (those that are not copy).
// There's no need to clone it since we're moving it out of the array.
valid_block.memcpy(context, location, elem_ptr, target_ptr, elem_size);

match metadata.get::<DropOverridesMeta>() {
Some(drop_overrides_meta) if drop_overrides_meta.is_overriden(&info.ty) => {
let array_end = valid_block.append_op_result(arith::extui(
array_end,
IntegerType::new(context, 64).into(),
location,
))?;

let array_start = valid_block.append_op_result(arith::muli(
array_start,
elem_stride,
location,
))?;
let array_end =
valid_block.append_op_result(arith::muli(array_end, elem_stride, location))?;

valid_block.append_operation(scf::r#for(
array_start,
array_end,
elem_stride,
{
let region = Region::new();
let block = region.append_block(Block::new(&[(
IntegerType::new(context, 64).into(),
location,
)]));

let value_ptr = block.append_op_result(llvm::get_element_ptr_dynamic(
context,
ptr,
&[block.argument(0)?.into()],
IntegerType::new(context, 8).into(),
llvm::r#type::pointer(context, 0),
location,
))?;

let is_target_element = block.append_op_result(
ods::llvm::icmp(
context,
IntegerType::new(context, 1).into(),
value_ptr,
elem_ptr,
IntegerAttribute::new(IntegerType::new(context, 64).into(), 0)
.into(),
location,
)
.into(),
)?;
block.append_operation(scf::r#if(
is_target_element,
&[],
{
let region = Region::new();
let block = region.append_block(Block::new(&[]));

let value = block.load(context, location, value_ptr, elem_ty)?;
drop_overrides_meta
.invoke_override(context, &block, location, &info.ty, value)?;

block.append_operation(scf::r#yield(&[], location));
region
},
{
let region = Region::new();
let block = region.append_block(Block::new(&[]));

block.append_operation(scf::r#yield(&[], location));
region
},
location,
));

block.append_operation(scf::r#yield(&[], location));
region
},
location,
));
}
_ => {}
}
valid_block.append_operation(ReallocBindingsMeta::free(context, ptr, location));

valid_block.append_operation(helper.br(0, &[range_check, target_ptr], location));
}

Expand Down Expand Up @@ -1135,6 +1219,12 @@ pub fn build_span_from_tuple<'ctx, 'this>(
// load box
entry.load(context, location, entry.argument(0)?.into(), struct_ty)?
};
// TODO: Maybe reuse the pointer?
entry.append_operation(ReallocBindingsMeta::free(
context,
entry.argument(0)?.into(),
location,
));

let fields = struct_type_info.fields().expect("should have fields");
let (field_ty, field_layout) =
Expand Down
Loading

0 comments on commit 79a8dcf

Please sign in to comment.