Fixes for small edge cases, docs and cache stats

python · pablogsal · Dec 6, 2025 · Dec 1, 2025 · Dec 1, 2025 · Dec 1, 2025
commit 1944a02df5d239805d4c5b36d65a705d99e5a7fa
@@ -126,7 +126,9 @@ and can be retrieved from a cache. This significantly reduces the amount of remo
 memory reads needed when call stacks are deep and stable at their base.
 
 The update in `_PyEval_FrameClearAndPop` is guarded: it only writes when
-`last_profiled_frame` is non-NULL, avoiding any overhead when profiling is inactive.
+`last_profiled_frame` is non-NULL AND matches the frame being popped. This
+prevents transient frames (called and returned between profiler samples) from
+corrupting the cache pointer, while avoiding any overhead when profiling is inactive.
 
 
 ### The Instruction Pointer

@@ -255,6 +255,18 @@ def _print_unwinder_stats(self):
         print(f"    Hits:             {code_hits:,} ({ANSIColors.GREEN}{code_hits_pct:.1f}%{ANSIColors.RESET})")
         print(f"    Misses:           {code_misses:,} ({ANSIColors.RED}{code_misses_pct:.1f}%{ANSIColors.RESET})")
 
+        # Memory operations
+        memory_reads = stats.get('memory_reads', 0)
+        memory_bytes = stats.get('memory_bytes_read', 0)
+        if memory_bytes >= 1024 * 1024:
+            memory_str = f"{memory_bytes / (1024 * 1024):.1f} MB"
+        elif memory_bytes >= 1024:
+            memory_str = f"{memory_bytes / 1024:.1f} KB"
+        else:
+            memory_str = f"{memory_bytes} B"
+        print(f"  {ANSIColors.CYAN}Memory:{ANSIColors.RESET}")
+        print(f"    Read operations:  {memory_reads:,} ({memory_str})")
+
         # Stale invalidations
         stale_invalidations = stats.get('stale_cache_invalidations', 0)
         if stale_invalidations > 0:

diff --git a/Lib/test/test_external_inspection.py b/Lib/test/test_external_inspection.py
@@ -2745,6 +2745,60 @@ def recurse(n):
         self.assertEqual(len(frames_cached), len(frames_no_cache),
                         "Cache exhaustion should not affect stack completeness")
 
+    @skip_if_not_supported
+    @unittest.skipIf(
+        sys.platform == "linux" and not PROCESS_VM_READV_SUPPORTED,
+        "Test only runs on Linux with process_vm_readv support",
+    )
+    def test_get_stats(self):
+        """Test that get_stats() returns statistics when stats=True."""
+        script_body = """\
+            sock.sendall(b"ready")
+            sock.recv(16)
+            """
+
+        with self._target_process(script_body) as (p, client_socket, _):
+            unwinder = RemoteUnwinder(p.pid, all_threads=True, stats=True)
+            self._wait_for_signal(client_socket, b"ready")
+
+            # Take a sample
+            unwinder.get_stack_trace()
+
+            stats = unwinder.get_stats()
+            client_socket.sendall(b"done")
+
+        # Verify expected keys exist
+        expected_keys = [
+            'total_samples', 'frame_cache_hits', 'frame_cache_misses',
+            'frame_cache_partial_hits', 'frames_read_from_cache',
+            'frames_read_from_memory', 'frame_cache_hit_rate'
+        ]
+        for key in expected_keys:
+            self.assertIn(key, stats)
+
+        self.assertEqual(stats['total_samples'], 1)
+
+    @skip_if_not_supported
+    @unittest.skipIf(
+        sys.platform == "linux" and not PROCESS_VM_READV_SUPPORTED,
+        "Test only runs on Linux with process_vm_readv support",
+    )
+    def test_get_stats_disabled_raises(self):
+        """Test that get_stats() raises RuntimeError when stats=False."""
+        script_body = """\
+            sock.sendall(b"ready")
+            sock.recv(16)
+            """
+
+        with self._target_process(script_body) as (p, client_socket, _):
+            unwinder = RemoteUnwinder(p.pid, all_threads=True)  # stats=False by default
+            self._wait_for_signal(client_socket, b"ready")
+
+            with self.assertRaises(RuntimeError):
+                unwinder.get_stats()
+
+            client_socket.sendall(b"done")
+
 
 if __name__ == "__main__":
     unittest.main()
@@ -174,6 +174,7 @@ typedef struct {
     uint64_t frames_read_from_cache;         // Total frames retrieved from cache
     uint64_t frames_read_from_memory;        // Total frames read from remote memory
     uint64_t memory_reads;                   // Total remote memory read operations
+    uint64_t memory_bytes_read;              // Total bytes read from remote memory
     uint64_t code_object_cache_hits;         // Code object cache hits
     uint64_t code_object_cache_misses;       // Code object cache misses
     uint64_t stale_cache_invalidations;      // Times stale entries were cleared
@@ -422,6 +423,7 @@ extern int frame_cache_lookup_and_extend(
     uintptr_t *frame_addrs,
     Py_ssize_t *num_addrs,
     Py_ssize_t max_addrs);
+// Returns: 1 = stored, 0 = not stored (graceful), -1 = error
 extern int frame_cache_store(
     RemoteUnwinderObject *unwinder,
     uint64_t thread_id,

diff --git a/Modules/_remote_debugging/clinic/module.c.h b/Modules/_remote_debugging/clinic/module.c.h
@@ -194,6 +194,7 @@ frame_cache_lookup_and_extend(
 }
 
 // Store frame list with addresses in cache
+// Returns: 1 = stored successfully, 0 = not stored (graceful degradation), -1 = error
 int
 frame_cache_store(
     RemoteUnwinderObject *unwinder,
@@ -220,11 +221,14 @@ frame_cache_store(
     // Clear old frame_list if replacing
     Py_CLEAR(entry->frame_list);
 
-    // Store data
+    // Store data - truncate frame_list to match num_addrs
+    entry->frame_list = PyList_GetSlice(frame_list, 0, num_addrs);
+    if (!entry->frame_list) {
+        return -1;
+    }
     entry->thread_id = thread_id;
     memcpy(entry->addrs, addrs, num_addrs * sizeof(uintptr_t));
     entry->num_addrs = num_addrs;
-    entry->frame_list = Py_NewRef(frame_list);
 
-    return 0;
+    return 1;
 }
@@ -189,6 +189,8 @@ parse_frame_object(
         set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to read interpreter frame");
         return -1;
     }
+    STATS_INC(unwinder, memory_reads);
+    STATS_ADD(unwinder, memory_bytes_read, SIZEOF_INTERP_FRAME);
 
     *previous_frame = GET_MEMBER(uintptr_t, frame, unwinder->debug_offsets.interpreter_frame.previous);
     uintptr_t code_object = GET_MEMBER_NO_TAG(uintptr_t, frame, unwinder->debug_offsets.interpreter_frame.executable);
@@ -477,23 +479,30 @@ try_full_cache_hit(
         return -1;
     }
 
-    // Add current frame if valid
+    // Get cached parent frames first (before modifying frame_info)
+    Py_ssize_t cached_size = PyList_GET_SIZE(entry->frame_list);
+    PyObject *parent_slice = NULL;
+    if (cached_size > 1) {
+        parent_slice = PyList_GetSlice(entry->frame_list, 1, cached_size);
+        if (!parent_slice) {
+            Py_XDECREF(current_frame);
+            return -1;
+        }
+    }
+
+    // Now safe to modify frame_info - add current frame if valid
     if (current_frame != NULL) {
         if (PyList_Append(frame_info, current_frame) < 0) {
             Py_DECREF(current_frame);
+            Py_XDECREF(parent_slice);
             return -1;
         }
         Py_DECREF(current_frame);
         STATS_ADD(unwinder, frames_read_from_memory, 1);
     }
 
-    // Extend with cached parent frames (from index 1 onwards, skipping the current frame)
-    Py_ssize_t cached_size = PyList_GET_SIZE(entry->frame_list);
-    if (cached_size > 1) {
-        PyObject *parent_slice = PyList_GetSlice(entry->frame_list, 1, cached_size);
-        if (!parent_slice) {
-            return -1;
-        }
+    // Extend with cached parent frames
+    if (parent_slice) {
         Py_ssize_t cur_size = PyList_GET_SIZE(frame_info);
         int result = PyList_SetSlice(frame_info, cur_size, cur_size, parent_slice);
         Py_DECREF(parent_slice);
@@ -569,7 +578,9 @@ collect_frames_with_cache(
     }
 
     // Store in cache (frame_cache_store handles truncation if num_addrs > FRAME_CACHE_MAX_FRAMES)
-    frame_cache_store(unwinder, thread_id, frame_info, addrs, num_addrs);
+    if (frame_cache_store(unwinder, thread_id, frame_info, addrs, num_addrs) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -814,6 +814,7 @@ RemoteUnwinder was created with stats=True.
         - frames_read_from_cache: Total frames retrieved from cache
         - frames_read_from_memory: Total frames read from remote memory
         - memory_reads: Total remote memory read operations
+        - memory_bytes_read: Total bytes read from remote memory
         - code_object_cache_hits: Code object cache hits
         - code_object_cache_misses: Code object cache misses
         - stale_cache_invalidations: Times stale cache entries were cleared
@@ -826,7 +827,7 @@ RemoteUnwinder was created with stats=True.
 
 static PyObject *
 _remote_debugging_RemoteUnwinder_get_stats_impl(RemoteUnwinderObject *self)
-/*[clinic end generated code: output=21e36477122be2a0 input=0a037cbf1c572d2b]*/
+/*[clinic end generated code: output=21e36477122be2a0 input=75fef4134c12a8c9]*/
 {
     if (!self->collect_stats) {
         PyErr_SetString(PyExc_RuntimeError,
@@ -857,6 +858,7 @@ _remote_debugging_RemoteUnwinder_get_stats_impl(RemoteUnwinderObject *self)
     ADD_STAT(frames_read_from_cache);
     ADD_STAT(frames_read_from_memory);
     ADD_STAT(memory_reads);
+    ADD_STAT(memory_bytes_read);
     ADD_STAT(code_object_cache_hits);
     ADD_STAT(code_object_cache_misses);
     ADD_STAT(stale_cache_invalidations);

@@ -296,6 +296,8 @@ unwind_stack_for_thread(
         set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to read thread state");
         goto error;
     }
+    STATS_INC(unwinder, memory_reads);
+    STATS_ADD(unwinder, memory_bytes_read, unwinder->debug_offsets.thread_state.size);
 
     long tid = GET_MEMBER(long, ts, unwinder->debug_offsets.thread_state.native_thread_id);
 
@@ -309,6 +311,8 @@ unwind_stack_for_thread(
         set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to read GC state");
         goto error;
     }
+    STATS_INC(unwinder, memory_reads);
+    STATS_ADD(unwinder, memory_bytes_read, unwinder->debug_offsets.gc.size);
 
     // Calculate thread status using flags (always)
     int status_flags = 0;