Re: [PATCH v2 1/2] perf python: New treport script

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The script uses multiple undefined attributes and methods from the `perf` Python module, guaranteeing an immediate crash.
- [Low] Unconditional import of the third-party `textual` package will cause a raw `ModuleNotFoundError` stack trace if it is not installed.
- [Low] Extracting the event name via hardcoded string slicing on the string representation of an object is extremely brittle.
- [Low] The commit message demonstrates usage with a path that does not match the actual location of the script.
--

--- Patch [1]: [PATCH v2 1/2] perf python: New treport script ---
commit c8886e7317972e7aa7d9b4370a2487dc827a9013
Author: Alice Rogers <[email protected]>

perf python: New treport script

This commit introduces a new textual-based application, treport.py, which
displays the results of perf sample processing in an interactive tree format.

> With perf script treport:
> 
> $ perf record -e cycles:u -g perf test -w brstack
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.008 MB perf.data (57 samples) ]
> $ perf script tools/perf/scripts/python/treport.py

[Severity: Low]
This isn't a bug, but the commit message demonstrates usage with
tools/perf/scripts/python/treport.py, while the patch creates the file
at tools/perf/python/treport.py. Could this cause confusion for users
trying to copy and paste the command?

> diff --git a/tools/perf/python/treport.py b/tools/perf/python/treport.py
> new file mode 100755
> index 0000000000000..528a640e4d356
> --- /dev/null
> +++ b/tools/perf/python/treport.py
> @@ -0,0 +1,207 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: MIT
> +"""treport.py - perf report like tool written using textual."""
> +from typing import Dict, Optional
> +import argparse
> +import os
> +import sys
> +import perf
> +from textual.app import App, ComposeResult

[Severity: Low]
Would it be helpful to catch the ImportError for the textual module and print
a user-friendly instruction to install it? Currently, users without this
third-party dependency installed will hit a raw ModuleNotFoundError stack
trace.

[ ... ]

> +class ProfileBuilder:
> +    """Constructs a profile tree from a stream of events."""
> +    def __init__(self):
> +        self.root = ProfileNode("root", parent=None)
> +
> +    def process_event(self, sample) -> None:
> +        """Called by session.process_events to update the profile tree."""
> +        ev_name = str(sample.evsel)[6:-1]

[Severity: High]
Does the perf Python C extension currently expose the evsel attribute on
sample objects? It appears this might raise an AttributeError at runtime
since sample.evsel doesn't seem to be implemented in tools/perf/util/python.c.

[Severity: Low]
Also, is it safe to rely on hardcoded string slicing [6:-1] here? If the
string representation format of the event changes slightly, could this rigid
slicing fail to correctly isolate the event name?

> +        ev_root = self.root.find_or_create_node(ev_name)
> +        ev_root.process_event(sample)

[ ... ]

> +    input_file = args.input or "perf.data"
> +    if not os.path.exists(input_file):
> +        print(f"Error: {input_file} not found. (try 'perf record' first)", file=sys.stderr)
> +        sys.exit(1)
> +
> +    profile = ProfileBuilder()
> +    try:
> +        session = perf.session(perf.data(input_file), sample=profile.process_event)

[Severity: High]
Are perf.session() and perf.data() supported in the current python API?
Looking at the existing perf python bindings, it doesn't appear these
methods are implemented. Will this unconditionally crash with an
AttributeError when executed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1