Re: [PATCH v4 3/4] perf tools: Add support for displaying weights in annotate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The TUI browser width calculation omits cycle and branch counter widths, leading to visual truncation.
- [Medium] Toggling weight columns globally freezes the weight mask for all symbols, breaking accurate display.
- [Medium] Use of compile-time host architecture macros breaks cross-platform analysis for weight modes.
- [Low] Typographical error in the `--weights` command-line option description.
--

--- Patch [3]: [PATCH v4 3/4] perf tools: Add support for displaying weights in annotate ---
commit 9eb5862996d191c9f87138b1476aa94808cc5008
Author: Andi Kleen <ak@linux.intel.com>

perf tools: Add support for displaying weights in annotate

Add support for showing all the three possible per IP weights in
annotate. The weights are shown by defaults if any are non zero. This
is useful, especially with the new insn lat statistics, but also
for all the existing weights.

Add a hotkey to the interactive browser to turn them off (w), as well
as a perf annotate command line option.

> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 69cb72b2082a6..998f472152e2f 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
[ ... ]
> @@ -719,6 +719,8 @@ int cmd_annotate(int argc, const char **argv)
>  	OPT_BOOLEAN(0, "tui", &annotate.use_tui, "Use the TUI interface"),
>  #endif
>  	OPT_BOOLEAN(0, "stdio", &annotate.use_stdio, "Use the stdio interface"),
> +	OPT_BOOLEAN(0, "weights", &symbol_conf.annotate_weight,
> +		    "Hide or hide weight columns in annotation. Default show if non zero."),

[Severity: Low]
This isn't a bug, but there is a typo in the help text here. "Hide or hide"
should probably be "Show or hide" to match the identical option added to
builtin-report.c.

> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e47a467750890..16713d85db7c5 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
[ ... ]
> @@ -1091,6 +1093,15 @@ static int annotate_browser__run(struct annotate_browser *browser,
>  				symbol_conf.show_total_period = true;
>  			annotation__update_column_widths(notes);
>  			continue;
> +		case 'w':
> +			symbol_conf.annotate_weight = !symbol_conf.annotate_weight;
> +			symbol_conf.weight_mask_user = true;
> +			symbol_conf.weight_mask = !symbol_conf.annotate_weight ?
> +				0 : sym_hist__weight_mask(annotation__histogram(notes, evsel));

[Severity: Medium]
Does toggling weight columns globally freeze the weight mask for all symbols?

Because symbol_conf is global, when the 'w' hotkey sets
symbol_conf.weight_mask_user to true, all subsequently viewed symbols will
use the frozen mask from this current symbol.

If a newly opened symbol has different weights (for example, WEIGHT_INSNLAT
instead of WEIGHT_WEIGHT), those relevant columns might incorrectly remain
hidden.

[ ... ]
> @@ -1228,7 +1239,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
>  	browser.b.width = notes->src->widths.max_line_len;
>  	browser.b.nr_entries = notes->src->nr_entries;
>  	browser.b.entries = &notes->src->source;
> -	browser.b.width += 18; /* Percentage */
> +	browser.b.width += annotation__pcnt_width(notes, evsel);

[Severity: Medium]
Could this width calculation result in visual truncation of the TUI browser?

By replacing the hardcoded 18 with annotation__pcnt_width() in
__hist_entry__tui_annotate(), the browser width calculation remains strictly
bound to the percentage and weight columns. It doesn't appear to make any
allowance for the IPC, cycles, or branch counters that might be displayed.

If no weights are present, the padding drops significantly, which might set
the horizontal scroll limit too narrow to show the full annotated line.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index df70e95a84704..d4886e18275ef 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -778,6 +797,47 @@ static bool needs_type_info(struct annotated_data_type *data_type)
>  	return (data_type != &stackop_type) && (data_type != &canary_type);
>  }
>  
> +static const char *annotation__weight_mode_str(enum symbol__weight_mode mode)
> +{
> +	switch (mode) {
> +	case WEIGHT_NONE:
> +		return "";
> +	case WEIGHT_WEIGHT:
> +		return "Weight";
> +	case WEIGHT_INSNLAT:
> +		return "InsnLat";
> +	case WEIGHT_WEIGHT3:
> +#ifdef __powerpc__
> +		return "PCycleLat";
> +#elif defined(__x86_64__)
> +		return "RetireLat";
> +#else
> +		return "Weight3";
> +#endif

[Severity: Medium]
Does using compile-time host architecture macros break cross-platform
analysis for weight modes?

If an ARM64 host machine analyzes an x86_64 perf.data file, it will
incorrectly display "Weight3" instead of "RetireLat".

Would it be better to dynamically inspect the ELF machine constant
(e_machine) via the evsel or session structures instead of relying on host
compile-time macros?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818230229.1491871-1-ak@linux.intel.com?part=3