Merge pull request #180 from benfred/localvars_fix

Fixes for printing local variables

src/dump.rs

@@ -1,13 +1,13 @@
 use std::collections::HashMap;
 
 use console::style;
-use failure::{Error, ResultExt};
+use failure::Error;
 
 use crate::config::Config;
 use crate::python_bindings::{v3_6_6, v3_7_0, v3_8_0};
 use crate::python_interpreters::{InterpreterState, Object, TypeObject};
 use crate::python_spy::PythonSpy;
-use crate::python_data_access::{copy_string, copy_long, stringify_pyobject, DictIterator};
+use crate::python_data_access::{copy_string, copy_long, DictIterator};
 
 use crate::version::Version;
 
@@ -20,16 +20,6 @@ pub fn print_traces(process: &mut PythonSpy, config: &Config) -> Result<(), Erro
         return Ok(())
     }
 
-    // if we're printing out local variables, we need to lock the process for correct results.
-    let _lock = if !config.dump_locals || config.non_blocking {
-        None
-    } else {
-        // disable other locking inside python_spy: some OS process locks aren't
-        // re-entrant and we will fail to acquire the lock in get_stack_traces otherwise
-        process.config.non_blocking = true;
-        Some(process.process.lock().context("Failed to suspend process")?)
-    };
-
     // try getting the threadnames, but don't sweat it if we can't. Since this relies on dictionary
     // processing we only handle py3.6+ right now, and this doesn't work at all if the
     // threading module isn't imported in the target program
@@ -85,9 +75,8 @@ pub fn print_traces(process: &mut PythonSpy, config: &Config) -> Result<(), Erro
                         shown_locals = true;
                     }
 
-                    let value = stringify_pyobject(&process.process, &process.version, local.addr, 128)
-                        .unwrap_or("?".to_owned());
-                    println!("            {}: {}", local.name, value);
+                    let repr = local.repr.as_ref().map(String::as_str).unwrap_or("?");
+                    println!("            {}: {}", local.name, repr);
                 }
             }
         }

src/lib.rs

@@ -67,4 +67,3 @@ pub use config::Config;
 pub use stack_trace::StackTrace;
 pub use stack_trace::Frame;
 pub use remoteprocess::Pid;
-pub use python_data_access::stringify_pyobject;

src/python_data_access.rs

@@ -4,7 +4,6 @@ use failure::Error;
 
 use remoteprocess::ProcessMemory;
 use crate::python_interpreters::{StringObject, BytesObject, InterpreterState, Object, TypeObject, TupleObject, ListObject};
-use crate::python_bindings::{v2_7_15, v3_3_7, v3_5_5, v3_6_6, v3_7_0, v3_8_0};
 use crate::version::Version;
 
 /// Copies a string from a target process. Attempts to handle unicode differences, which mostly seems to be working
@@ -140,28 +139,6 @@ impl<'a> Iterator for DictIterator<'a> {
     }
 }
 
-/// Converts a python object in the other process to a string representation
-pub fn stringify_pyobject(process: &remoteprocess::Process,
-                          version: &Version,
-                          addr: usize,
-                          max_length: isize) -> Result<String, Error> {
-    match version {
-        Version{major: 2, minor: 3..=7, ..} => format_variable::<v2_7_15::_is>(process, version, addr, max_length),
-        Version{major: 3, minor: 3, ..} => format_variable::<v3_3_7::_is>(process, version, addr, max_length),
-        Version{major: 3, minor: 4..=5, ..} => format_variable::<v3_5_5::_is>(process, version, addr, max_length),
-        Version{major: 3, minor: 6, ..} => format_variable::<v3_6_6::_is>(process, version, addr, max_length),
-        Version{major: 3, minor: 7, ..} => format_variable::<v3_7_0::_is>(process, version, addr, max_length),
-        Version{major: 3, minor: 8, patch: 0, ..} => {
-            match version.release_flags.as_ref() {
-                "a1" | "a2" | "a3" => format_variable::<v3_7_0::_is>(process, version, addr, max_length),
-                _ => format_variable::<v3_8_0::_is>(process, version, addr, max_length)
-            }
-        },
-        Version{major: 3, minor: 8..=9, ..} => format_variable::<v3_8_0::_is>(process, version, addr, max_length),
-        _ => Err(format_err!("Unsupported version of Python: {}", version))
-    }
-}
-
 const PY_TPFLAGS_INT_SUBCLASS: usize =     1 << 23;
 const PY_TPFLAGS_LONG_SUBCLASS: usize =    1 << 24;
 const PY_TPFLAGS_LIST_SUBCLASS: usize =    1 << 25;

src/python_spy.rs

@@ -253,6 +253,13 @@ impl PythonSpy {
 
             for frame in &mut trace.frames {
                 frame.short_filename = self.shorten_filename(&frame.filename);
+                if let Some(locals) = frame.locals.as_mut() {
+                    use crate::python_data_access::format_variable;
+                    for local in locals {
+                        let repr = format_variable::<I>(&self.process, &self.version, local.addr, 128);
+                        local.repr = Some(repr.unwrap_or("?".to_owned()));
+                    }
+                }
             }
 
             traces.push(trace);

src/stack_trace.rs

@@ -43,7 +43,8 @@ pub struct Frame {
 pub struct LocalVariable {
     pub name: String,
     pub addr: usize,
-    pub arg: bool
+    pub arg: bool,
+    pub repr: Option<String>,
 }
 
 /// Given an InterpreterState, this function returns a vector of stack traces for each thread
@@ -170,7 +171,7 @@ fn get_locals<C: CodeObject, F: FrameObject, P: ProcessMemory>(code: &C, framept
         if addr == 0 {
             continue;
         }
-        ret.push(LocalVariable{name, addr, arg: i < argcount });
+        ret.push(LocalVariable{name, addr, arg: i < argcount, repr: None});
     }
     Ok(ret)
 }

tests/integration_test.rs

@@ -185,52 +185,44 @@ fn test_local_vars() {
     let arg1 = &locals[0];
     assert_eq!(arg1.name, "arg1");
     assert!(arg1.arg);
-    let arg1_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, arg1.addr, 128).unwrap();
-    assert_eq!(arg1_str, "\"foo\"");
+    assert_eq!(arg1.repr, Some("\"foo\"".to_owned()));
 
     let arg2 = &locals[1];
     assert_eq!(arg2.name, "arg2");
     assert!(arg2.arg);
-    let arg2_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, arg2.addr, 128).unwrap();
-    assert_eq!(arg2_str, "None");
+    assert_eq!(arg2.repr, Some("None".to_owned()));
 
     let arg3 = &locals[2];
     assert_eq!(arg3.name, "arg3");
     assert!(arg3.arg);
-    let arg3_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, arg3.addr, 128).unwrap();
-    assert_eq!(arg3_str, "True");
+    assert_eq!(arg3.repr, Some("True".to_owned()));
 
     let local1 = &locals[3];
     assert_eq!(local1.name, "local1");
     assert!(!local1.arg);
-    let local1_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, local1.addr, 128).unwrap();
-    assert_eq!(local1_str, "[-1234, 5678]");
+    assert_eq!(local1.repr, Some("[-1234, 5678]".to_owned()));
 
     let local2 = &locals[4];
     assert_eq!(local2.name, "local2");
     assert!(!local2.arg);
-    let local2_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, local2.addr, 128).unwrap();
-    assert_eq!(local2_str, "(\"a\", \"b\", \"c\")");
+    assert_eq!(local2.repr, Some("(\"a\", \"b\", \"c\")".to_owned()));
 
     let local3 = &locals[5];
     assert_eq!(local3.name, "local3");
     assert!(!local3.arg);
-    let local3_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, local3.addr, 128).unwrap();
-    assert_eq!(local3_str, "123456789123456789");
+    assert_eq!(local3.repr, Some("123456789123456789".to_owned()));
 
     let local4 = &locals[6];
     assert_eq!(local4.name, "local4");
     assert!(!local4.arg);
-    let local4_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, local4.addr, 128).unwrap();
-    assert_eq!(local4_str, "3.1415");
+    assert_eq!(local4.repr, Some("3.1415".to_owned()));
 
     let local5 = &locals[7];
     assert_eq!(local5.name, "local5");
     assert!(!local5.arg);
-    let local4_str = py_spy::stringify_pyobject(&runner.spy.process, &runner.spy.version, local5.addr, 128).unwrap();
 
     // we only support dictionary lookup on python 3.6+ right now
     if runner.spy.version.major == 3 && runner.spy.version.minor >= 6 {
-        assert_eq!(local4_str, "{\"a\": False, \"b\": (1, 2, 3)}");
+        assert_eq!(local5.repr, Some("{\"a\": False, \"b\": (1, 2, 3)}".to_owned()));
     }
 }