-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
gh-151022: Fix remote debugging linetable reads #151036
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
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 |
|---|---|---|
|
|
@@ -438,6 +438,56 @@ def _extract_coroutine_stacks_lineno_only(self, stack_trace): | |
| # ============================================================================ | ||
|
|
||
|
|
||
| @requires_remote_subprocess_debugging() | ||
| class TestSelfStackTrace(RemoteInspectionTestBase): | ||
| @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_self_trace_with_large_linetable(self): | ||
| script = textwrap.dedent("""\ | ||
| import os | ||
| import _remote_debugging | ||
|
|
||
| assignments = "\\n".join( | ||
| f"value_{i} = {i}" for i in range(1000) | ||
| ) | ||
| expected_lineno = len(assignments.splitlines()) + 1 | ||
| source = ( | ||
| f"{assignments}\\n" | ||
| "stack_trace = " | ||
| "_remote_debugging.RemoteUnwinder(os.getpid()).get_stack_trace()\\n" | ||
| ) | ||
| code = compile(source, "large_linetable.py", "exec") | ||
| assert len(code.co_linetable) > 4096, len(code.co_linetable) | ||
| namespace = {"os": os, "_remote_debugging": _remote_debugging} | ||
| exec(code, namespace) | ||
| large_linetable_frames = [ | ||
| frame | ||
| for interpreter in namespace["stack_trace"] | ||
| for thread in interpreter.threads | ||
| for frame in thread.frame_info | ||
| if frame.filename == "large_linetable.py" | ||
| ] | ||
| assert len(large_linetable_frames) == 1, large_linetable_frames | ||
| assert large_linetable_frames[0].location.lineno == expected_lineno, ( | ||
| large_linetable_frames[0] | ||
| ) | ||
| """) | ||
|
|
||
| result = subprocess.run( | ||
| [sys.executable, "-c", script], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=SHORT_TIMEOUT, | ||
| ) | ||
| self.assertEqual( | ||
| result.returncode, 0, | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" | ||
|
Comment on lines
+485
to
+487
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. @goutamadwant It would be interesting to ensure that the returned linetable is also correct, not
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. @maurycy Updated this too. The test now checks the returned frame for large_linetable.py and asserts the resolved line number matches the generated RemoteUnwinder call line, so it should catch the -1 fallback case. |
||
| ) | ||
|
|
||
|
|
||
| @requires_remote_subprocess_debugging() | ||
| class TestGetStackTrace(RemoteInspectionTestBase): | ||
| @skip_if_not_supported | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix ``_remote_debugging`` stack traces for code objects with large line | ||
| tables. |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,9 @@ | |||||||||||||
|
|
||||||||||||||
| #include "_remote_debugging.h" | ||||||||||||||
|
|
||||||||||||||
| #define MAX_LINETABLE_SIZE (1024 * 1024) | ||||||||||||||
|
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. @goutamadwant Why exactly 1024 * 1024? :-) I know that's an arbitrary number but my hunch would be to try to check what sizes are we seeing in the wild, or at least in the stdlib. There's
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. You are right @maurycy :) .. 1024 * 1024 was a conservative cap but not really data driven. I ran a modified linetable scan against Lib. On current main I see max linetable size 37416 bytes, and the reported copyparty case was also lower, around 19536 bytes. So I think 64 * 1024 may be a better cap for this PR. It covers the stdlib cases and the reported third party case, while still keeping the remote bytes read bounded much tighter than 1 MiB. Does that sound reasonable to you ? I can make a change accordingly. |
||||||||||||||
| #define MAX_LINETABLE_ENTRIES 65536 | ||||||||||||||
|
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 did not check it but my guess would be that there's much more head room in the cpython/Modules/_remote_debugging/code_objects.c Lines 495 to 500 in 29a920e
To be honest, I'd assert (proportionally) Another solution would be along the lines of
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. @maurycy I agree this needs a bit more thought. From the same scan, the largest stdlib linetable I saw was Lib/html/entities.py with 37416 bytes and about 7465 parsed entries. So for the reported cases, the byte cap is the immediate problem. my thinking is to keep the entry cap as a guard for malformed or pathological linetables and keep this PR focused on the too small byte read limit... I also strengthened the test so it checks a real line number instead of allowing -1. Changing parse failure from unknown line to an exception feels like a separate behavior change to me. Happy to adjust if you and @pablogsal prefer that in this PR :) |
||||||||||||||
|
|
||||||||||||||
| /* ============================================================================ | ||||||||||||||
| * TLBC CACHING FUNCTIONS (Py_GIL_DISABLED only) | ||||||||||||||
| * ============================================================================ */ | ||||||||||||||
|
|
@@ -186,7 +189,6 @@ parse_linetable(const uintptr_t addrq, const char* linetable, Py_ssize_t linetab | |||||||||||||
| int computed_line = firstlineno; // Running accumulator, separate from output | ||||||||||||||
| int val; // Temporary for varint results | ||||||||||||||
| uint8_t byte; // Temporary for byte reads | ||||||||||||||
| const size_t MAX_LINETABLE_ENTRIES = 65536; | ||||||||||||||
| size_t entry_count = 0; | ||||||||||||||
|
|
||||||||||||||
| while (ptr < end && *ptr != '\0' && entry_count < MAX_LINETABLE_ENTRIES) { | ||||||||||||||
|
|
@@ -387,7 +389,8 @@ parse_code_object(RemoteUnwinderObject *unwinder, | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| linetable = read_py_bytes(unwinder, | ||||||||||||||
| GET_MEMBER(uintptr_t, code_object, unwinder->debug_offsets.code_object.linetable), 4096); | ||||||||||||||
| GET_MEMBER(uintptr_t, code_object, unwinder->debug_offsets.code_object.linetable), | ||||||||||||||
| MAX_LINETABLE_SIZE); | ||||||||||||||
| if (!linetable) { | ||||||||||||||
| set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to read linetable from code object"); | ||||||||||||||
| goto error; | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goutamadwant I think there's a missing
@requires_remote_subprocess_debugging()decorator :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed @maurycy.. added the missing @requires_remote_subprocess_debugging() decorator to the new test class.