Skip to content
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

Allow displaying co_firstlineno in -F mode #424

Closed
Jongy opened this issue Jul 21, 2021 · 0 comments · Fixed by #428
Closed

Allow displaying co_firstlineno in -F mode #424

Jongy opened this issue Jul 21, 2021 · 0 comments · Fixed by #428

Comments

@Jongy
Copy link
Contributor

Jongy commented Jul 21, 2021

In --function mode, py-spy doesn't write line numbers (to avoid aggregating by the "last instruction" line numbers - and instead, by function name).
Function name is not always enough to identify a function in a file - for example, __init__ in a file with multiple classes.
I suggest adding py-spy the ability to to log the co_firstlineno when in --function mode (or at least, make it an option with an extra flag).

The business logic is simple:

 /// Gets a stack trace for an individual thread
-pub fn get_stack_trace<T>(thread: &T, process: &Process, copy_locals: bool) -> Result<StackTrace, Error>
+pub fn get_stack_trace<T>(thread: &T, process: &Process, copy_locals: bool, line_no: LineNo) -> Result<StackTrace, Error>
         where T: ThreadState {
     // TODO: just return frames here? everything else probably should be returned out of scope
     let mut frames = Vec::new();
@@ -93,15 +99,19 @@ pub fn get_stack_trace<T>(thread: &T, process: &Process, copy_locals: bool) -> R
         let filename = copy_string(code.filename(), process).context("Failed to copy filename")?;
         let name = copy_string(code.name(), process).context("Failed to copy function name")?;
 
-        let line = match get_line_number(&code, frame.lasti(), process) {
-            Ok(line) => line,
-            Err(e) => {
-                // Failling to get the line number really shouldn't be fatal here, but
-                // can happen in extreme cases (https://github.com/benfred/py-spy/issues/164)
-                // Rather than fail set the linenumber to 0. This is used by the native extensions
-                // to indicate that we can't load a line number and it should be handled gracefully
-                warn!("Failed to get line number from {}.{}: {}", filename, name, e);
-                0
+        let line = match line_no {
+            NoLine => 0,
+            FirstLineNo => code.first_lineno(),
+            LastInstruction => match get_line_number(&code, frame.lasti(), process) {
+                Ok(line) => line,
+                Err(e) => {
+                    // Failling to get the line number really shouldn't be fatal here, but
+                    // can happen in extreme cases (https://github.com/benfred/py-spy/issues/164)
+                    // Rather than fail set the linenumber to 0. This is used by the native extensions
+                    // to indicate that we can't load a line number and it should be handled gracefully
+                    warn!("Failed to get line number from {}.{}: {}", filename, name, e);
+                    0
+                }
             }

I'm wondering how to add it with respect to py-spy existing flags (config.show_line_numbers). I can change show_line_number to LineNo and pass it over in py-spy instead? Or add it as an extra option?

Wdyt @benfred ?

Jongy added a commit to Jongy/py-spy that referenced this issue Jul 31, 2021
Before this PR, we had 2 modes:
* --function NOT passed: attach the line number from frame->f_lasti to the frame name
* --function passed: do not attach any line number

The main reason to avoid attaching f_lasti is to have frames aggregated by the function name only;
so different "lines" of the same function are aggregated as a single frame.

However, line numbers are useful as identifiers for functions, especially in large files / for common
function names such as "__init__", which are likely to appear multiple times in the same file.
This PR allows attaching code->co_firstlineno instead, which can serve to help in identification,
while not preventing frames of the same function from being aggregated together.

After this PR, we have these options:
* --function, --nolineno NOT passed: same as before - attach f_lasti to the frame name
* --function passed: attach co_firstlineno to the frame name
* --nolineno: do not attach any line number (also, do not spend time on retrieving the line number from
  the remote process).

Closes: benfred#424
benfred pushed a commit that referenced this issue Aug 18, 2021
Before this PR, we had 2 modes:
* --function NOT passed: attach the line number from frame->f_lasti to the frame name
* --function passed: do not attach any line number

The main reason to avoid attaching f_lasti is to have frames aggregated by the function name only;
so different "lines" of the same function are aggregated as a single frame.

However, line numbers are useful as identifiers for functions, especially in large files / for common
function names such as "__init__", which are likely to appear multiple times in the same file.
This PR allows attaching code->co_firstlineno instead, which can serve to help in identification,
while not preventing frames of the same function from being aggregated together.

After this PR, we have these options:
* --function, --nolineno NOT passed: same as before - attach f_lasti to the frame name
* --function passed: attach co_firstlineno to the frame name
* --nolineno: do not attach any line number (also, do not spend time on retrieving the line number from
  the remote process).

Closes: #424
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 a pull request may close this issue.

1 participant