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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Variable shadowing by `for_each_weight` macro in `annotation_line__print` causes either premature loop termination or an infinite loop.
- [Medium] Asynchronous global modification of `symbol_conf.weight_mask` by the background thread in `perf top` causes data races and defeats the user's hotkey toggle.
--

--- Patch [3]: [PATCH v1 3/4] perf tools: Add support for displaying weights in annotate ---
commit 7947c9d694355b6721ccd7944dc41891a9968db4
Author: Andi Kleen <[email protected]>
Date:   Thu Aug 13 13:29:06 2026 +0000

    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.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 2888ac3b7b926..32bd6e23f6fdd 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -827,6 +827,7 @@ static void perf_event__process_sample(const struct perf_tool *tool,
>  			top->vmlinux_warned = true;
>  		}
>  	}
> +	symbol_conf__set_weight_mode(&symbol_conf, sample);
>  
>  	if (al.sym == NULL ||
>  	    !symbol__is_idle(al.sym, al.map ? map__dso(al.map) : NULL, machine->env)) {

[ ... ]

> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e47a467750890..dada5fa6d29f2 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c

[ ... ]

> @@ -1091,6 +1092,14 @@ 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.hide_weight = !symbol_conf.hide_weight;
> +			symbol_conf.weight_mask = symbol_conf.hide_weight ? 0 :
> +				annotation__histogram(notes, evsel)->weight_mask;
> +			browser->b.width = notes->src->widths.max_line_len +
> +				annotation__pcnt_width(notes);
> +			ui_browser__refresh_dimensions(&browser->b);
> +			continue;

[ ... ]

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 94f9c8faeddab..4b04320e35ecb 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c

[ ... ]

> @@ -2763,3 +2763,14 @@ char *dso__demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
>  
>  	return java_demangle_sym(elf_name, JAVA_DEMANGLE_NORET);
>  }
> +
> +void symbol_conf__set_weight_mode(struct symbol_conf *c,
> +				  struct perf_sample *sample)
> +{
> +	if (sample->weight)
> +		c->weight_mask |= BIT(WEIGHT_WEIGHT);
> +	if (sample->ins_lat)
> +		c->weight_mask |= BIT(WEIGHT_INSNLAT);
> +	if (sample->weight3)
> +		c->weight_mask |= BIT(WEIGHT_WEIGHT3);
> +}

[Severity: Medium]
In perf top, event processing runs in a background thread while the UI
runs in the foreground. When a user presses the 'w' hotkey, it sets
symbol_conf.weight_mask to 0.

However, the background thread constantly calls symbol_conf__set_weight_mode()
for new samples, which unconditionally modifies symbol_conf.weight_mask
without checking if symbol_conf.hide_weight is set.

Could this data race cause the TUI to tear or miscalculate column widths,
effectively defeating the user's attempt to hide the weight columns?

[ ... ]

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index df70e95a84704..6be6ec47f298b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c

[ ... ]

> @@ -845,6 +892,12 @@ annotation_line__print(struct annotation_line *al, struct annotation_print_data
>  					      data->he.nr_samples);
>  			else
>  				color_fprintf(stdout, color, " %7.2f", percent);
> +			for_each_weight(i) {
> +				if (symbol_conf.weight_mask & BIT(i))
> +					color_fprintf(stdout, color, " %7" PRIu64,
> +							data->he.weight_num[i] ?
> +							data->he.weight_sum[i] / data->he.weight_num[i] : 0);
> +			}
>  		}

[Severity: Medium]
Will this for_each_weight(i) macro silently shadow and corrupt the outer
loop's iterator variable 'i'?

The outer loop in annotation_line__print() uses 'i' to iterate up to nr_percent:

    for (i = 0; i < nr_percent; i++) {
        struct annotation_data *data = &al->data[i];
        ...
        for_each_weight(i) {
            ...
        }
    }

Since for_each_weight() expands to a for loop that reuses and resets 'i'
from WEIGHT_WEIGHT to WEIGHT_WEIGHT3, won't this cause either premature loop
termination (if nr_percent <= 5) or an infinite loop (if nr_percent > 5)
when processing event groups?

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