Skip to content

Commit

Permalink
Remove unsafe impl of Add<usize> for &'a Relocatable (#1718)
Browse files Browse the repository at this point in the history 
* Remove unsafe impl of `Add<usize> for &Relocatable`

* Add changelog entry

* Add error handling to RelocateValue trait
  • Loading branch information
@fmoletta
fmoletta authored Apr 17, 2024
1 parent b05b541 commit 4b17118
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 69 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#### Upcoming Changes

* fix(BREAKING): Remove unsafe impl of `Add<usize> for &'a Relocatable`[#1718](https://github.com/lambdaclass/cairo-vm/pull/1718)

* fix(BREAKING): Handle triple dereference references[#1708](https://github.com/lambdaclass/cairo-vm/pull/1708)
* Replace `ValueAddress` boolean field `dereference` with boolean fields `outer_dereference` & `inner_dereference`
* Replace `HintReference` boolean field `dereference` with boolean fields `outer_dereference` & `inner_dereference`
Expand Down
11 changes: 0 additions & 11 deletions vm/src/types/relocatable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,17 +356,6 @@ impl MaybeRelocatable {
}
}

impl<'a> Add<usize> for &'a Relocatable {
type Output = Relocatable;

fn add(self, other: usize) -> Self::Output {
Relocatable {
segment_index: self.segment_index,
offset: self.offset + other,
}
}
}

/// Turns a MaybeRelocatable into a Felt252 value.
/// If the value is an Int, it will extract the Felt252 value from it.
/// If the value is RelocatableValue, it will relocate it according to the relocation_table
Expand Down
8 changes: 6 additions & 2 deletions vm/src/vm/runners/builtin_runner/signature.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,12 @@ impl SignatureBuiltinRunner {
pub fn air_private_input(&self, memory: &Memory) -> Vec<PrivateInput> {
let mut private_inputs = vec![];
for (addr, signature) in self.signatures.borrow().iter() {
if let (Ok(pubkey), Ok(msg)) = (memory.get_integer(*addr), memory.get_integer(addr + 1))
{
if let (Ok(pubkey), Some(msg)) = (
memory.get_integer(*addr),
(*addr + 1_usize)
.ok()
.and_then(|addr| memory.get_integer(addr).ok()),
) {
private_inputs.push(PrivateInput::Signature(PrivateInputSignature {
index: addr
.offset
Expand Down
17 changes: 5 additions & 12 deletions vm/src/vm/runners/cairo_runner.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -568,10 +568,7 @@ impl CairoRunner {
} else {
let mut stack_prefix = vec![
Into::<MaybeRelocatable>::into(
self.execution_base
.as_ref()
.ok_or(RunnerError::NoExecBase)?
+ target_offset,
(self.execution_base.ok_or(RunnerError::NoExecBase)? + target_offset)?,
),
MaybeRelocatable::from(Felt252::zero()),
];
Expand All @@ -589,20 +586,16 @@ impl CairoRunner {
)?;
}

self.initial_fp = Some(
self.execution_base
.as_ref()
.ok_or(RunnerError::NoExecBase)?
+ target_offset,
);
self.initial_fp =
Some((self.execution_base.ok_or(RunnerError::NoExecBase)? + target_offset)?);

self.initial_ap = self.initial_fp;
return Ok(self.program_base.as_ref().ok_or(RunnerError::NoProgBase)?
return Ok((self.program_base.ok_or(RunnerError::NoProgBase)?
+ self
.program
.shared_program_data
.end
.ok_or(RunnerError::NoProgramEnd)?);
.ok_or(RunnerError::NoProgramEnd)?)?);
}

let return_fp = vm.segments.add();
Expand Down
106 changes: 62 additions & 44 deletions vm/src/vm/vm_memory/memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,24 +178,23 @@ impl Memory {
&self.data
};
let (i, j) = from_relocatable_to_indexes(relocatable);
Some(self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value()))
self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value())
.ok()
}

// Version of Memory.relocate_value() that doesn't require a self reference
fn relocate_address(
addr: Relocatable,
relocation_rules: &HashMap<usize, Relocatable>,
) -> MaybeRelocatable {
let segment_idx = addr.segment_index;
if segment_idx >= 0 {
return addr.into();
}

// Adjust the segment index to begin at zero, as per the struct field's
match relocation_rules.get(&(-(segment_idx + 1) as usize)) {
Some(x) => (x + addr.offset).into(),
None => addr.into(),
) -> Result<MaybeRelocatable, MemoryError> {
if addr.segment_index < 0 {
// Adjust the segment index to begin at zero, as per the struct field's
// comment.
if let Some(x) = relocation_rules.get(&(-(addr.segment_index + 1) as usize)) {
return Ok((*x + addr.offset)?.into());
}
}
Ok(addr.into())
}

/// Relocates the memory according to the relocation rules and clears `self.relocaction_rules`.
Expand All @@ -209,7 +208,7 @@ impl Memory {
let value = cell.get_value_mut();
match value {
MaybeRelocatable::RelocatableValue(addr) if addr.segment_index < 0 => {
*value = Memory::relocate_address(*addr, &self.relocation_rules);
*value = Memory::relocate_address(*addr, &self.relocation_rules)?;
}
_ => {}
}
Expand Down Expand Up @@ -581,39 +580,42 @@ impl fmt::Display for Memory {

/// Applies `relocation_rules` to a value
pub(crate) trait RelocateValue<'a, Input: 'a, Output: 'a> {
fn relocate_value(&self, value: Input) -> Output;
fn relocate_value(&self, value: Input) -> Result<Output, MemoryError>;
}

impl RelocateValue<'_, Relocatable, Relocatable> for Memory {
fn relocate_value(&self, addr: Relocatable) -> Relocatable {
let segment_idx = addr.segment_index;
if segment_idx >= 0 {
return addr;
}

// Adjust the segment index to begin at zero, as per the struct field's
// comment.
match self.relocation_rules.get(&(-(segment_idx + 1) as usize)) {
Some(x) => x + addr.offset,
None => addr,
fn relocate_value(&self, addr: Relocatable) -> Result<Relocatable, MemoryError> {
if addr.segment_index < 0 {
// Adjust the segment index to begin at zero, as per the struct field's
// comment.
if let Some(x) = self
.relocation_rules
.get(&(-(addr.segment_index + 1) as usize))
{
return (*x + addr.offset).map_err(MemoryError::Math);
}
}
Ok(addr)
}
}

impl<'a> RelocateValue<'a, &'a Felt252, &'a Felt252> for Memory {
fn relocate_value(&self, value: &'a Felt252) -> &'a Felt252 {
value
fn relocate_value(&self, value: &'a Felt252) -> Result<&'a Felt252, MemoryError> {
Ok(value)
}
}

impl<'a> RelocateValue<'a, &'a MaybeRelocatable, Cow<'a, MaybeRelocatable>> for Memory {
fn relocate_value(&self, value: &'a MaybeRelocatable) -> Cow<'a, MaybeRelocatable> {
match value {
fn relocate_value(
&self,
value: &'a MaybeRelocatable,
) -> Result<Cow<'a, MaybeRelocatable>, MemoryError> {
Ok(match value {
MaybeRelocatable::Int(_) => Cow::Borrowed(value),
MaybeRelocatable::RelocatableValue(addr) => {
Cow::Owned(self.relocate_value(*addr).into())
Cow::Owned(self.relocate_value(*addr)?.into())
}
}
})
}
}

Expand Down Expand Up @@ -1043,7 +1045,9 @@ mod memory_tests {

// Test when value is Some(BigInt):
assert_eq!(
memory.relocate_value(&MaybeRelocatable::Int(Felt252::from(0))),
memory
.relocate_value(&MaybeRelocatable::Int(Felt252::from(0)))
.unwrap(),
Cow::Owned(MaybeRelocatable::Int(Felt252::from(0))),
);
}
Expand All @@ -1061,11 +1065,15 @@ mod memory_tests {

// Test when value is Some(MaybeRelocatable) with segment_index >= 0:
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((0, 0).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((0, 0).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((0, 0).into())),
);
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((5, 0).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((5, 0).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((5, 0).into())),
);
}
Expand All @@ -1084,7 +1092,9 @@ mod memory_tests {
// Test when value is Some(MaybeRelocatable) with segment_index < 0 and
// there are no applicable relocation rules:
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((-5, 0).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((-5, 0).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((-5, 0).into())),
);
}
Expand All @@ -1103,19 +1113,27 @@ mod memory_tests {
// Test when value is Some(MaybeRelocatable) with segment_index < 0 and
// there are applicable relocation rules:
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 0).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 0).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((2, 0).into())),
);
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 0).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 0).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((2, 2).into())),
);
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 5).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 5).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((2, 5).into())),
);
assert_eq!(
memory.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 5).into())),
memory
.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 5).into()))
.unwrap(),
Cow::Owned(MaybeRelocatable::RelocatableValue((2, 7).into())),
);
}
Expand Down Expand Up @@ -1498,11 +1516,11 @@ mod memory_tests {
.unwrap();

assert_eq!(
Memory::relocate_address((-1, 0).into(), &memory.relocation_rules),
Memory::relocate_address((-1, 0).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((2, 0).into()),
);
assert_eq!(
Memory::relocate_address((-2, 1).into(), &memory.relocation_rules),
Memory::relocate_address((-2, 1).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((2, 3).into()),
);
}
Expand All @@ -1512,11 +1530,11 @@ mod memory_tests {
fn relocate_address_no_rules() {
let memory = Memory::new();
assert_eq!(
Memory::relocate_address((-1, 0).into(), &memory.relocation_rules),
Memory::relocate_address((-1, 0).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((-1, 0).into()),
);
assert_eq!(
Memory::relocate_address((-2, 1).into(), &memory.relocation_rules),
Memory::relocate_address((-2, 1).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((-2, 1).into()),
);
}
Expand All @@ -1526,11 +1544,11 @@ mod memory_tests {
fn relocate_address_real_addr() {
let memory = Memory::new();
assert_eq!(
Memory::relocate_address((1, 0).into(), &memory.relocation_rules),
Memory::relocate_address((1, 0).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((1, 0).into()),
);
assert_eq!(
Memory::relocate_address((1, 1).into(), &memory.relocation_rules),
Memory::relocate_address((1, 1).into(), &memory.relocation_rules).unwrap(),
MaybeRelocatable::RelocatableValue((1, 1).into()),
);
}
Expand Down

0 comments on commit 4b17118

Please sign in to comment.