Re: [PATCH v2 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:

New issues:
- [Medium] Toggling the 'w' hotkey caches the current event's weight mask globally, corrupting weight column display for all subsequent symbols and events.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is an out-of-bounds heap memory write in `symbol__annotate_zero_histogram()` when zeroing an event's histogram.
--

--- Patch [3]: [PATCH v2 3/4] perf tools: Add support for displaying weights in annotate ---
commit 89fdbcaac6998b08ac6d1c7549a509183b024849
Author: Andi Kleen <[email protected]>

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/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e47a467750890..d6126a7d51e4a 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.hide_weight = !symbol_conf.hide_weight;
> +			symbol_conf.weight_mask_user = true;
> +			symbol_conf.weight_mask = symbol_conf.hide_weight ?
> +				0 : sym_hist__weight_mask(annotation__histogram(notes, evsel));
> +			browser->b.width = notes->src->widths.max_line_len +
> +					   annotation__pcnt_width(notes, evsel);
> +			ui_browser__refresh_dimensions(&browser->b);
> +			continue;

[ ... ]

> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index fa08d09b80f76..63bce7a920f32 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h

[ ... ]

> @@ -376,9 +391,22 @@ static inline int annotation__cycles_width(struct annotation *notes)
>  	return notes->branch ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
>  }
>  
> -static inline int annotation__pcnt_width(struct annotation *notes)
> +static inline u8 annotation__weight_mask(struct annotation *notes,
> +					 const struct evsel *evsel)
> +{
> +	if (symbol_conf.weight_mask_user)
> +		return symbol_conf.weight_mask;

[Severity: Medium]
Will this globally cache the current event's dynamically generated weight mask?

In annotate_browser__run(), pressing 'w' sets symbol_conf.weight_mask_user to
true and saves the mask. Returning that cached mask here seems to force all
subsequently viewed symbols and events to use this exact column layout.

If a different event has different weight types, or if in perf top new weight
types arrive after 'w' is pressed, would the UI incorrectly hide valid weight
columns or display empty zero-filled columns for missing weights?

> +	if (symbol_conf.hide_weight)
> +		return 0;
> +	return sym_hist__weight_mask(&notes->src->histograms[evsel->core.idx]);
> +}

[ ... ]

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

[ ... ]

> @@ -1448,7 +1523,8 @@ int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
>  	return err;
>  }
>  
> -void symbol__annotate_zero_histogram(struct symbol *sym, struct evsel *evsel)
> +void symbol__annotate_zero_histogram(struct symbol *sym,
> +				     struct evsel *evsel)
>  {
>  	struct annotation *notes = symbol__annotation(sym);
>  	struct sym_hist *h = annotation__histogram(notes, evsel);

[Severity: High]
This is a pre-existing issue, but does the memset in this function cause an
out-of-bounds heap memory write?

When profiling multiple events (e.g. perf top -e cycles,instructions) and
zeroing the histogram (e.g. pressing 'z'), this gets called for top->sym_evsel:

void symbol__annotate_zero_histogram(...)
{
	...
	struct sym_hist *h = annotation__histogram(notes, evsel);

	memset(h, 0, sizeof(*notes->src->histograms) * notes->src->nr_histograms);
}

The pointer h is offset to the specific event's index, but the memset zeroes
an area equal to the size of the entire histograms array. Could this write
past the end of the heap allocation?

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