Handle pcs outside of program segment in VmException (#1501)

* Handle pc's outside of program segment in VmException' ' ' * Update changelog * clippy * Fix test value * Disable attributes in traceback if si != 0 * fmt * Fix test
lambdaclass · Nov 30, 2023 · 8887e60 · 8887e60
1 parent b657576
commit 8887e60
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 #### Upcoming Changes
 
+* feat: Handle `pc`s outside of program segment in `VmException` [#1501] (https://github.com/lambdaclass/cairo-vm/pull/1501)
+
+  * `VmException` now shows the full pc value instead of just the offset (`VmException.pc` field type changed to `Relocatable`)
+  * `VmException.traceback` now shows the full pc value for each entry instead of hardcoding its index to 0.
+  * Disable debug information for errors produced when `pc` is outside of the program segment (segment_index != 0). `VmException` fields `inst_location` & `error_attr_value` will be `None` in such case.
+
 * feat: Allow running instructions from pcs outside the program segement [#1493](https://github.com/lambdaclass/cairo-vm/pull/14923)
 
 * BREAKING: Partially Revert `Optimize trace relocation #906` [#1492](https://github.com/lambdaclass/cairo-vm/pull/1492)

diff --git a/vm/src/vm/errors/vm_exception.rs b/vm/src/vm/errors/vm_exception.rs
@@ -1,7 +1,10 @@
-use crate::stdlib::{
-    fmt::{self, Display},
-    prelude::*,
-    str,
+use crate::{
+    stdlib::{
+        fmt::{self, Display},
+        prelude::*,
+        str,
+    },
+    types::relocatable::Relocatable,
 };
 
 use thiserror_no_std::Error;
@@ -16,7 +19,7 @@ use crate::{
 use super::vm_errors::VirtualMachineError;
 #[derive(Debug, Error)]
 pub struct VmException {
-    pub pc: usize,
+    pub pc: Relocatable,
     pub inst_location: Option<Location>,
     pub inner_exc: VirtualMachineError,
     pub error_attr_value: Option<String>,
@@ -29,16 +32,24 @@ impl VmException {
         vm: &VirtualMachine,
         error: VirtualMachineError,
     ) -> Self {
-        let pc = vm.run_context.pc.offset;
-        let error_attr_value = get_error_attr_value(pc, runner, vm);
+        let pc = vm.run_context.pc;
+        let error_attr_value = if pc.segment_index == 0 {
+            get_error_attr_value(pc.offset, runner, vm)
+        } else {
+            None
+        };
         let hint_index = if let VirtualMachineError::Hint(ref bx) = error {
             Some(bx.0)
         } else {
             None
         };
         VmException {
             pc,
-            inst_location: get_location(pc, runner, hint_index),
+            inst_location: if pc.segment_index == 0 {
+                get_location(pc.offset, runner, hint_index)
+            } else {
+                None
+            },
             inner_exc: error,
             error_attr_value,
             traceback: get_traceback(vm, runner),
@@ -92,18 +103,21 @@ pub fn get_location(
 pub fn get_traceback(vm: &VirtualMachine, runner: &CairoRunner) -> Option<String> {
     let mut traceback = String::new();
     for (_fp, traceback_pc) in vm.get_traceback_entries() {
-        if let Some(ref attr) = get_error_attr_value(traceback_pc.offset, runner, vm) {
+        if let (0, Some(ref attr)) = (
+            traceback_pc.segment_index,
+            get_error_attr_value(traceback_pc.offset, runner, vm),
+        ) {
             traceback.push_str(attr)
         }
-        match get_location(traceback_pc.offset, runner, None) {
-            Some(location) => traceback.push_str(&format!(
+        match (
+            traceback_pc.segment_index,
+            get_location(traceback_pc.offset, runner, None),
+        ) {
+            (0, Some(location)) => traceback.push_str(&format!(
                 "{}\n",
-                location.to_string_with_content(&format!("(pc=0:{})", traceback_pc.offset))
-            )),
-            None => traceback.push_str(&format!(
-                "Unknown location (pc=0:{})\n",
-                traceback_pc.offset
+                location.to_string_with_content(&format!("(pc={})", traceback_pc))
             )),
+            _ => traceback.push_str(&format!("Unknown location (pc={})\n", traceback_pc)),
         }
     }
     (!traceback.is_empty())
@@ -194,7 +208,7 @@ fn get_value_from_simple_reference(
 impl Display for VmException {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // Build initial message
-        let message = format!("Error at pc=0:{}:\n{}", self.pc, self.inner_exc);
+        let message = format!("Error at pc={}:\n{}", self.pc, self.inner_exc);
         let mut error_msg = String::new();
         // Add error attribute value
         if let Some(ref string) = self.error_attr_value {
@@ -314,7 +328,7 @@ mod test {
     #[test]
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn get_vm_exception_from_vm_error() {
-        let pc = 0;
+        let pc: Relocatable = (0, 0).into();
         let location = Location {
             end_line: 2,
             end_col: 2,
@@ -329,8 +343,9 @@ mod test {
             inst: location.clone(),
             hints: vec![],
         };
-        let program =
-            program!(instruction_locations = Some(HashMap::from([(pc, instruction_location)])),);
+        let program = program!(
+            instruction_locations = Some(HashMap::from([(pc.offset, instruction_location)])),
+        );
         let runner = cairo_runner!(program);
         assert_matches!(
             VmException::from_vm_error(&runner, &vm!(), VirtualMachineError::NoImm,),
@@ -388,7 +403,7 @@ mod test {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn vm_exception_display_instruction_no_location_no_attributes() {
         let vm_excep = VmException {
-            pc: 2,
+            pc: (0, 2).into(),
             inst_location: None,
             inner_exc: VirtualMachineError::FailedToComputeOperands(Box::new((
                 "op0".to_string(),
@@ -413,7 +428,7 @@ mod test {
     #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
     fn vm_exception_display_instruction_no_location_with_attributes() {
         let vm_excep = VmException {
-            pc: 2,
+            pc: (0, 2).into(),
             inst_location: None,
             inner_exc: VirtualMachineError::FailedToComputeOperands(Box::new((
                 "op0".to_string(),
@@ -448,7 +463,7 @@ mod test {
             start_col: 1,
         };
         let vm_excep = VmException {
-            pc: 2,
+            pc: (0, 2).into(),
             inst_location: Some(location),
             inner_exc: VirtualMachineError::FailedToComputeOperands(Box::new((
                 "op0".to_string(),
@@ -495,7 +510,7 @@ mod test {
             start_col: 1,
         };
         let vm_excep = VmException {
-            pc: 2,
+            pc: (0, 2).into(),
             inst_location: Some(location),
             inner_exc: VirtualMachineError::FailedToComputeOperands(Box::new((
                 "op0".to_string(),
@@ -1143,4 +1158,39 @@ cairo_programs/bad_programs/uint256_sub_b_gt_256.cairo:10:2: (pc=0:12)
             )
         );
     }
+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn get_vm_exception_from_vm_error_pc_not_program_segment() {
+        let pc = (9, 5).into();
+        let location = Location {
+            end_line: 2,
+            end_col: 2,
+            input_file: InputFile {
+                filename: String::from("Folder/file.cairo"),
+            },
+            parent_location: None,
+            start_line: 1,
+            start_col: 1,
+        };
+        let instruction_location = InstructionLocation {
+            inst: location,
+            hints: vec![],
+        };
+        let program =
+            program!(instruction_locations = Some(HashMap::from([(5, instruction_location)])),);
+        let runner = cairo_runner!(program);
+        let mut vm = vm!();
+        vm.set_pc(pc);
+        assert_matches!(
+            VmException::from_vm_error(&runner, &vm, VirtualMachineError::NoImm,),
+            VmException {
+                pc: x,
+                inst_location: None,
+                inner_exc: VirtualMachineError::NoImm,
+                error_attr_value: None,
+                traceback: None,
+            } if x == pc
+        )
+    }
 }