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

[Namhyung Kim <namhyung@kernel.org>]

Hi Andi,

On Tue, Aug 18, 2026 at 11:15:36PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Please fix.

> 
> > 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.

I don't remember where 18 came from but it seems we need to check
different annotation mode like IPC or branch counters with this change.

> 
> > 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?

Right, I think you can pass an evsel and get a session then e_machine
from it.

Thanks,
Namhyung

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