Re: [PATCH v2 30/58] perf flamegraph: Port flamegraph to use python module

sashiko-bot
2026-04-23T04:14:29Z

Sashiko AI review found 5 potential issue(s):
- [High] The `--event` filtering logic is completely broken because it compares the raw event name with the stringified `evsel` object, which are never equal.
- [High] The fallback logic for missing callchains incorrectly resolves the missing `symbol` property to `None` instead of `"[unknown]"`, injecting `null` strings into the output JSON.
- [High] The type annotation `list[Dict]` causes a `TypeError` at module import time on Python versions older than 3.9.
- [Medium] An MD5 mismatch on the downloaded template prints the entire HTML file inside the user prompt, causing severe console flooding and a potential terminal injection vector.
- [Low] Local custom template files whose names start with 'http' will fail to load, erroneously treating them as network URLs.
--

commit 6b91c1b1c1fbf8eb29a0d7bb8458776c8e77b1bb
Author: Ian Rogers <irogers@google.com>

perf flamegraph: Port flamegraph to use python module

Add a port of the flamegraph script that uses the perf python module
directly. This approach is significantly faster than using perf script
callbacks as it avoids creating intermediate dictionaries for all
event fields.

> diff --git a/tools/perf/python/flamegraph.py b/tools/perf/python/flamegraph.py
> new file mode 100755
> index 0000000000000..f3f69e5a88c24
> --- /dev/null
> +++ b/tools/perf/python/flamegraph.py

[ ... ]

> + def to_json(self) -> Dict[str, Union[str, int, list[Dict]]]:
> + """Convert the node to a JSON-serializable dictionary."""

Can this type annotation cause issues on older Python versions? On Python
versions prior to 3.9, the built-in list is not subscriptable, which causes a
TypeError at module import time.

[ ... ]

> + def process_event(self, sample) -> None:
> + """Process a single perf sample event."""
> + if self.args.event_name and str(sample.evsel) != self.args.event_name:
> + return

Is the event name comparison correct? Since str(sample.evsel) typically
formats as evsel(name) via the Python C extension, wouldn't comparing it
directly to the event name string (like cycles) always fail and discard
all samples?

[ ... ]

> + callchain = sample.callchain
> + if callchain:
> + # We want to traverse from root to leaf.
> + # perf callchain iterator gives leaf to root.
> + # We collect them and reverse.
> + frames = list(callchain)
> + for entry in reversed(frames):
> + name = entry.symbol or "[unknown]"
> + libtype = self.get_libtype_from_dso(entry.dso)
> + node = self.find_or_create_node(node, name, libtype)
> + else:
> + # Fallback if no callchain
> + name = getattr(sample, "symbol", "[unknown]")

Does this correctly handle missing symbols?

If sample has a symbol attribute but its value is None (which
pyrf_sample_event__get_symbol returns for unresolved symbols), getattr will
return None rather than [unknown]. This would result in null in the JSON
output.

[ ... ]

> + use_minimal = False
> + try:
> + if not template:
> + use_minimal = True
> + elif template.startswith("http"):

This isn't a bug, but if a user provides a local file named something like
http-template.html, will it erroneously be treated as a network URL and fail
to load?

> + with urllib.request.urlopen(template) as url_template:
> + output_str = "".join([l.decode("utf-8") for l in url_template.readlines()])
> + else:
> + with open(template, "r", encoding="utf-8") as f:
> + output_str = f.read()

[ ... ]

> + elif template_md5sum:
> + assert output_str is not None
> + download_md5sum = hashlib.md5(output_str.encode("utf-8")).hexdigest()
> + if download_md5sum != template_md5sum:
> + s = None
> + while s not in ["y", "n"]:
> + s = input(f"""Unexpected template md5sum.
> +{download_md5sum} != {template_md5sum}, for:
> +{output_str}
> +continue?[yn] """).lower()

If there's a checksum mismatch, this prints the entire downloaded HTML string
inside the input prompt.

Does this flood the terminal with thousands of lines, making the prompt hard
to read? Also, could printing unverified network content directly expose the
terminal to ANSI escape sequence injection?

> + if s == "n":
> + sys.exit(1)

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260423035526.1537178-1-irogers@google.com?part=30