-
Notifications
You must be signed in to change notification settings - Fork 57
Add subinterpreter support #279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c46223d
77a8d89
a2ae327
f6f943b
cd5b967
e108116
febc51d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Fix an issue where the PID offset could not be determined when multiple | ||
| subinterpreters were present. Previously, pystack only checked the first | ||
| interpreter in the linked list, which was not guaranteed to be the main | ||
| interpreter. The fix now iterates over all interpreters and correctly locates | ||
| the TID. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Add support for subinterpreters when reporting pure Python stacks. Threads | ||
| running in subinterpreters are now identified and grouped by interpreter ID. | ||
| Native stack reporting for subinterpreters is not yet supported. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,5 @@ | ||
| from ._version import __version__ | ||
| from .traceback_formatter import print_thread | ||
|
|
||
| __all__ = [ | ||
| "__version__", | ||
| "print_thread", | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ from _pystack.elf_common cimport CoreFileAnalyzer as NativeCoreFileAnalyzer | |
| from _pystack.elf_common cimport ProcessAnalyzer as NativeProcessAnalyzer | ||
| from _pystack.elf_common cimport SectionInfo | ||
| from _pystack.elf_common cimport getSectionInfo | ||
| from _pystack.interpreter cimport InterpreterUtils | ||
| from _pystack.logging cimport initializePythonLoggerInterface | ||
| from _pystack.mem cimport AbstractRemoteMemoryManager | ||
| from _pystack.mem cimport MemoryMapInformation as CppMemoryMapInformation | ||
|
|
@@ -65,6 +66,7 @@ from .types import NativeFrame | |
| from .types import PyCodeObject | ||
| from .types import PyFrame | ||
| from .types import PyThread | ||
| from .types import frame_type | ||
|
|
||
| LOGGER = logging.getLogger(__file__) | ||
|
|
||
|
|
@@ -462,6 +464,7 @@ cdef object _construct_threads_from_interpreter_state( | |
| bint add_native_traces, | ||
| bint resolve_locals, | ||
| ): | ||
| interpreter_id = InterpreterUtils.getInterpreterId(manager, head) | ||
| LOGGER.info("Fetching Python threads") | ||
| threads = [] | ||
|
|
||
|
|
@@ -486,7 +489,9 @@ cdef object _construct_threads_from_interpreter_state( | |
| current_thread.isGilHolder(), | ||
| current_thread.isGCCollecting(), | ||
| python_version, | ||
| interpreter_id, | ||
| name=get_thread_name(pid, current_thread.Tid()), | ||
| stack_anchor=current_thread.StackAnchor(), | ||
| ) | ||
| ) | ||
| current_thread = ( | ||
|
|
@@ -495,6 +500,78 @@ cdef object _construct_threads_from_interpreter_state( | |
|
|
||
| return threads | ||
|
|
||
|
|
||
| def _entry_frame_count(thread: PyThread) -> int: | ||
| return sum(1 for frame in thread.all_frames if frame.is_entry) | ||
|
|
||
|
|
||
| def _slice_native_stacks_for_same_tid_threads(threads) -> None: | ||
| canonical = next((thread for thread in threads if thread.native_frames), None) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I follow this at all. Won't each of the threads in a group have its own copy of exactly the same list of native frames? I understand how this could ever return anything but the first thread in the list (unless |
||
| if canonical is None: | ||
| return | ||
|
|
||
| canonical_frames = list(canonical.native_frames) | ||
| eval_positions = [ | ||
| index | ||
| for index, native_frame in enumerate(canonical_frames) | ||
| if frame_type(native_frame, canonical.python_version) == NativeFrame.FrameType.EVAL | ||
| ] | ||
| if not eval_positions: | ||
| return | ||
|
|
||
| entry_counts = [_entry_frame_count(thread) for thread in threads] | ||
| if sum(entry_counts) != len(eval_positions): | ||
| LOGGER.debug( | ||
| "Skipping same-tid native slicing for tid %s due to mismatched counts: " | ||
| "entry=%s eval=%s", | ||
| threads[0].tid, | ||
| sum(entry_counts), | ||
| len(eval_positions), | ||
| ) | ||
| return | ||
|
|
||
| ordered_threads = sorted( | ||
| enumerate(threads), | ||
| key=lambda item: ( | ||
| item[1].stack_anchor is None, # Everything with a stack anchor before everything without one | ||
| -(item[1].stack_anchor or 0), # The stack anchor in descending order | ||
| item[0], # The index of the item in the list in ascending order | ||
| ), | ||
scopreon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
| cursor = 0 | ||
| for _, thread in ordered_threads: | ||
| required_eval_frames = _entry_frame_count(thread) | ||
| if required_eval_frames == 0: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How could this happen? If there is a Python stack, there will always be at least 1 entry frame, right? I guess this could happen if we've acquired the GIL (so there's a thread state) but we don't yet have any Python stack for that thread state? |
||
| thread.native_frames = [] | ||
| continue | ||
|
|
||
| group_start = cursor | ||
| group_end = cursor + required_eval_frames | ||
| prev_eval = eval_positions[group_start - 1] if group_start > 0 else -1 | ||
| next_eval = ( | ||
| eval_positions[group_end] | ||
| if group_end < len(eval_positions) | ||
| else len(canonical_frames) | ||
|
Comment on lines
+553
to
+555
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, how could
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is handling the end of the positions, when it is False it means we want the rest of the |
||
| ) | ||
| thread.native_frames = canonical_frames[prev_eval + 1 : next_eval] | ||
| cursor = group_end | ||
|
|
||
|
|
||
| def _normalize_python_threads(threads, native_mode: NativeReportingMode): | ||
| if native_mode == NativeReportingMode.OFF: | ||
| return threads | ||
|
|
||
| threads_by_tid = {} | ||
| for thread in threads: | ||
| threads_by_tid.setdefault(thread.tid, []).append(thread) | ||
|
|
||
| for group in threads_by_tid.values(): | ||
| if len(group) <= 1: | ||
| continue | ||
| _slice_native_stacks_for_same_tid_threads(group) | ||
| return threads | ||
|
|
||
| cdef object _construct_os_thread( | ||
| shared_ptr[AbstractProcessManager] manager, int pid, int tid | ||
| ): | ||
|
|
@@ -622,7 +699,8 @@ def _get_process_threads( | |
| ) | ||
|
|
||
| all_tids = list(manager.get().Tids()) | ||
| if head: | ||
| threads = [] | ||
| while head: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a minor nit, but - this winds up printing the subinterpreters from the most recently spawned to the least, printing the main interpreter last. I wonder if it would be more useful to reverse the list of interpreters, so that the main interpreter's threads are printed first... What do you think, @pablogsal ? |
||
| add_native_traces = native_mode != NativeReportingMode.OFF | ||
| for thread in _construct_threads_from_interpreter_state( | ||
| manager, | ||
|
|
@@ -634,7 +712,11 @@ def _get_process_threads( | |
| ): | ||
| if thread.tid in all_tids: | ||
| all_tids.remove(thread.tid) | ||
| yield thread | ||
| threads.append(thread) | ||
| head = InterpreterUtils.getNextInterpreter(manager, head) | ||
|
|
||
| for thread in _normalize_python_threads(threads, native_mode): | ||
| yield thread | ||
scopreon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if native_mode == NativeReportingMode.ALL: | ||
| yield from _construct_os_threads(manager, pid, all_tids) | ||
|
|
@@ -768,15 +850,25 @@ def _get_process_threads_for_core( | |
| ) | ||
|
|
||
| all_tids = list(manager.get().Tids()) | ||
| threads = [] | ||
|
|
||
| if head: | ||
| native = native_mode in {NativeReportingMode.PYTHON, NativeReportingMode.ALL} | ||
| while head: | ||
| add_native_traces = native_mode != NativeReportingMode.OFF | ||
| for thread in _construct_threads_from_interpreter_state( | ||
| manager, head, pymanager.pid, pymanager.python_version, native, locals | ||
| manager, | ||
| head, | ||
| pymanager.pid, | ||
| pymanager.python_version, | ||
| add_native_traces, | ||
| locals, | ||
| ): | ||
| if thread.tid in all_tids: | ||
| all_tids.remove(thread.tid) | ||
| yield thread | ||
| threads.append(thread) | ||
| head = InterpreterUtils.getNextInterpreter(manager, head) | ||
|
|
||
| for thread in _normalize_python_threads(threads, native_mode): | ||
| yield thread | ||
scopreon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if native_mode == NativeReportingMode.ALL: | ||
| yield from _construct_os_threads(manager, pymanager.pid, all_tids) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||
| #include <memory> | ||||||
|
|
||||||
| #include "interpreter.h" | ||||||
| #include "logging.h" | ||||||
| #include "process.h" | ||||||
| #include "structure.h" | ||||||
| #include "version.h" | ||||||
|
|
||||||
| namespace pystack { | ||||||
|
|
||||||
| remote_addr_t | ||||||
| InterpreterUtils::getNextInterpreter( | ||||||
| const std::shared_ptr<const AbstractProcessManager>& manager, | ||||||
| remote_addr_t interpreter_addr) | ||||||
| { | ||||||
| Structure<py_is_v> is(manager, interpreter_addr); | ||||||
| return is.getField(&py_is_v::o_next); | ||||||
| } | ||||||
|
|
||||||
| int64_t | ||||||
| InterpreterUtils::getInterpreterId( | ||||||
| const std::shared_ptr<const AbstractProcessManager>& manager, | ||||||
| remote_addr_t interpreter_addr) | ||||||
| { | ||||||
| if (!manager->versionIsAtLeast(3, 7)) { | ||||||
| // No support for subinterpreters so the only interpreter is ID 0. | ||||||
| return 0; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. This feels like the wrong way to handle this... It's technically possible to have multiple interpreters even for 3.6, though I don't think we'd support it particularly well if someone did it. But perhaps we should just use the address, or something like that, as a reasonable fallback? Maybe just
Suggested change
Though we might want the function to return a Or maybe we should just make up a synthetic ID, counting up from 0... I guess this is fine for now, maybe we can just punt on it unless and until someone complains... |
||||||
| } | ||||||
|
|
||||||
| Structure<py_is_v> is(manager, interpreter_addr); | ||||||
| int64_t id_value = is.getField(&py_is_v::o_id); | ||||||
|
|
||||||
| return id_value; | ||||||
| } | ||||||
|
|
||||||
| } // namespace pystack | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <memory> | ||
|
|
||
| #include "mem.h" | ||
| #include "process.h" | ||
|
|
||
| namespace pystack { | ||
|
|
||
| class InterpreterUtils | ||
| { | ||
| public: | ||
| // Static Methods | ||
| static remote_addr_t getNextInterpreter( | ||
| const std::shared_ptr<const AbstractProcessManager>& manager, | ||
| remote_addr_t interpreter_addr); | ||
|
|
||
| static int64_t getInterpreterId( | ||
| const std::shared_ptr<const AbstractProcessManager>& manager, | ||
| remote_addr_t interpreter_addr); | ||
| }; | ||
|
|
||
| } // namespace pystack |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from _pystack.mem cimport remote_addr_t | ||
| from _pystack.process cimport AbstractProcessManager | ||
| from libc.stdint cimport int64_t | ||
| from libcpp.memory cimport shared_ptr | ||
|
|
||
|
|
||
| cdef extern from "interpreter.h" namespace "pystack": | ||
| cdef cppclass InterpreterUtils: | ||
scopreon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @staticmethod | ||
| remote_addr_t getNextInterpreter(shared_ptr[AbstractProcessManager] manager, remote_addr_t interpreter_addr) except + | ||
|
|
||
| @staticmethod | ||
| int64_t getInterpreterId(shared_ptr[AbstractProcessManager] manager, remote_addr_t interpreter_addr) except + | ||
Uh oh!
There was an error while loading. Please reload this page.