Re: [RFC PATCH 1/1] perf scripts flamegraph: Add --asm option

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `--asm` option directly opens unverified `dso` paths on the host filesystem, bypassing perf's build-id cache.
- [Medium] The script invokes a new `objdump` process via `subprocess.run` for every unique sampled function, creating a severe performance bottleneck.
- [Medium] The script crashes with a raw `FileNotFoundError` if the configured or default `objdump` executable is missing.
--

--- Patch [1]: [RFC PATCH 1/1] perf scripts flamegraph: Add --asm option ---
commit e7196726e4cd528492356aed32496e94223294a5
Author: Tudor-Stefan Magirescu <[email protected]>

perf scripts flamegraph: Add --asm option

The flamegraph script reports samples at function granularity, so a wide
frame shows which function is hot but not which part of it. This patch
adds an --asm option to emit more fine-grained flame graphs by including
instruction-level information.

> diff --git a/tools/perf/scripts/python/flamegraph.py b/tools/perf/scripts/python/flamegraph.py
> index ad735990c5bec..424919e24aed7 100755
> --- a/tools/perf/scripts/python/flamegraph.py
> +++ b/tools/perf/scripts/python/flamegraph.py
[ ... ]
> @@ -67,10 +76,101 @@ class Node:
>          }
>  
>  
> +@dataclass
> +class Symbol:
> +    start: int
> +    size: int
> +
> +
> +class Instructions:
> +    def __init__(self):
> +        self.symbols:      dict[str, dict[str, Symbol]] = {}
> +        self.instructions: dict[tuple[str, str], dict[int, str]] = {}
> +        self.objdump_line: re.Pattern = re.compile(r"^\s+([0-9a-f]+):\s+(.*)")
> +        self.symbol_line: re.Pattern = re.compile(
> +            r"^([0-9a-f]+) (.{7})\s+\S+\s+([0-9a-f]+)\s+(.*)$")
> +        self.objdump:      str = default_objdump()
> +
> +    def load_symbols(self, dso: str) -> dict[str, Symbol]:
> +        result = subprocess.run(
> +            [self.objdump, "--demangle", "-t", dso],
> +            capture_output=True, text=True, check=False
> +        )

[Severity: Medium]
Does this code bypass perf's build-id cache by opening the dso path directly on
the host filesystem?

The objdump command attempts to open the dso string directly on the host
filesystem.

This can fail for kernel symbols like [kernel.kallsyms] or read incorrect host
binaries when analyzing cross-platform or container perf.data files.

[Severity: Medium]
Will this crash with a raw FileNotFoundError if the objdump executable is
missing?

Without an exception handler, if the configured or default objdump executable is
missing, the script will abruptly crash with a Python traceback.

It might be better to gracefully fall back to function-level nodes or display a
helpful error.

[ ... ]
> +    def load_instructions(self, dso: str, sym: Symbol) -> dict[int, str]:
> +        result = subprocess.run(
> +            [
> +                self.objdump, "-d",
> +                "--no-show-raw-insn",
> +                f"--start-address=0x{sym.start:x}",
> +                f"--stop-address=0x{sym.start + sym.size:x}",
> +                dso,
> +            ],
> +            capture_output=True, text=True, check=False
> +        )

[Severity: Medium]
Will this also crash with a raw FileNotFoundError if the objdump executable is
missing?

Like load_symbols(), this subprocess.run() call lacks an exception handler for
when objdump cannot be found on the system path.

[ ... ]
> +    def lookup_instruction(self, dso: str, func: str,
> +                           off: int) -> Optional[tuple[int, Optional[str]]]:
> +        if dso not in self.symbols:
> +            self.symbols[dso] = self.load_symbols(dso)
> +
> +        sym = self.symbols[dso].get(func)
> +        if sym is None:
> +            return None
> +
> +        if (dso, func) not in self.instructions:
> +            self.instructions[(dso, func)] = self.load_instructions(dso, sym)

[Severity: Medium]
Does this create a severe performance bottleneck by invoking a new objdump
process for every unique sampled function?

Profiling an application with many unique functions causes thousands of objdump
processes to be spawned sequentially, resulting in a severe performance
bottleneck and drastically increasing execution time.

[ ... ]
> @@ -119,6 +219,22 @@ class FlameGraphCLI:
>              name = event.get("symbol", "[unknown]")
>              libtype = self.get_libtype_from_dso(event.get("dso"))
>              node = self.find_or_create_node(node, name, libtype)
> +
> +        if self.args.asm:
> +            # use the sample IP directly rather than callchain[0], since with
> +            # precise event recording (i.e. :pp) the top callchain entry may
> +            # point to the next instruction rather than the sampled IP
> +            sym_name = event.get("symbol")
> +            sym_off  = event.get("symoff")
> +            dso      = event.get("dso")
> +
> +            if sym_name and sym_off is not None and dso:
> +                found = self.instructions.lookup_instruction(dso, sym_name, sym_off)

[Severity: Medium]
Does this pass the target's unresolved dso path directly to the instruction
lookup logic?

When analyzing perf.data files from other architectures, containers, or kernel
space, using the dso string directly on the host filesystem will lead to missing
or incorrect instruction mappings because it bypasses the build-id cache.

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