Skip to content

Formalize linetable parsing and propagate out-of-bounds errors #755

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

noahbkim
Copy link

@noahbkim noahbkim commented Apr 1, 2025

We've noticed a transient issue where py-spy will abort due to unmet assumptions about the contents of co_linetable (for example we've seen [231], which doesn't even satisfy the internal specification). To address this, I've patched the co_linetable parsing to propagate out-of-bounds errors so that the samples are simply ignored (and a warning is raised).

This change is not as minimal as it could be, for example it reorders a couple lines in python_interpreters.rs for clarity. If that's a problem, I'd be happy to go back and pare down such changes.

Taking a glance, I would guess this is the root cause for #735 and #569, although I have no explanation for why the co_linetable's appear invalid.

@noahbkim
Copy link
Author

noahbkim commented Apr 4, 2025

Updated the PR to include a fix for an off-by-one introduced by #635, trivially reproducible with the following:

# a.py
import b

# b.py
import time
time.sleep(10)

# Command
cargo run -F unwind --release -- record --native --rate 1 -- python3.12 a.py

@korniltsev
Copy link

This change is not as minimal as it could be, for example it reorders a couple lines in python_interpreters.rs for clarity. If that's a problem, I'd be happy to go back and pare down such changes.

Let's change only what's needed for the fix, so that it's easier to cherry-pick & revert & reason about

@Hnasar
Copy link

Hnasar commented Apr 22, 2025

@benfred can approve this workflow so the build can run?

Otherwise this LGTM, and we've manually built it and confirmed it's fixed the issues we were seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants