Re: [PATCH v1 20/26] perf annotate: Use generic hashmap for sym_samples

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf annotate: unused-but-set variable key in
  hashmap__for_each() loops
- [Low] perf annotate: DECLARE_HASHMAP() placed inside struct
  annotated_source kdoc
--

commit 0f2a111b069905a59af5949de2e5dc82c8c9ba96
Author: Ian Rogers <irogers@google.com>

perf annotate: Use generic hashmap for sym_samples

This commit converts annotated_source::samples to a typed hashmap
declared with DECLARE_HASHMAP(sym_samples_map, long,
struct sym_hist_entry *) and updates histogram entry freeing to use
hashmap__for_each().

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index d11add6424dc3..c6325040452f0 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -112,16 +112,16 @@ static struct annotated_source *annotated_source__new(void)
>  
>  static __maybe_unused void annotated_source__delete(struct annotated_source *src)
>  {
> -	struct hashmap_entry *cur;
> -	size_t bkt;
> +	long key;
> +	struct sym_hist_entry *val;
>  
>  	if (src == NULL)
>  		return;
>  
>  	if (src->samples) {
> -		hashmap__for_each_entry(src->samples, cur, bkt)
> -			zfree(&cur->pvalue);
> -		hashmap__free(src->samples);
> +		hashmap__for_each(src->samples, key, val)
> +			free(val);
> +		sym_samples_map__free(src->samples);
>  	}
>  	zfree(&src->histograms);
>  	free(src);
[ ... ]
> @@ -163,11 +163,11 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
>  		memset(notes->src->histograms, 0,
>  		       notes->src->nr_histograms * sizeof(*notes->src->histograms));
>  		if (notes->src->samples) {
> -			struct hashmap_entry *cur;
> -			size_t bkt;
> +			long key;
> +			struct sym_hist_entry *val;
>  
> -			hashmap__for_each_entry(notes->src->samples, cur, bkt)
> -				zfree(&cur->pvalue);
> +			hashmap__for_each(notes->src->samples, key, val)
> +				free(val);
>  			hashmap__clear(notes->src->samples);
>  		}
>  	}

[Severity: Medium]
Does declaring key without __maybe_unused in annotated_source__delete()
and symbol__annotate_zero_histograms() break the build with
-Werror=unused-but-set-variable?

hashmap__for_each() assigns to key on each iteration, and neither
function reads it.

> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 1b47c1156b9a7..495c8602864cf 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -312,10 +312,13 @@ struct cyc_hist {
>   * presented. It is deallocated right after symbol__{tui,tty,etc}_annotate
>   * returns.
>   */
> +struct sym_hist_entry;
> +DECLARE_HASHMAP(sym_samples_map, long, struct sym_hist_entry *);
> +
>  struct annotated_source {
>  	struct list_head	source;
>  	struct sym_hist		*histograms;
> -	struct hashmap	   	*samples;
> +	struct sym_samples_map	*samples;

[Severity: Low]
Does placing DECLARE_HASHMAP() between the struct annotated_source
kernel-doc comment and the struct definition break scripts/kernel-doc?

Moving DECLARE_HASHMAP(sym_samples_map, ...) above the kernel-doc comment
keeps the comment attached to struct annotated_source, and the forward
declaration of struct sym_hist_entry can be dropped because it is already
defined earlier in this header.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113214850.3329699-1-irogers@google.com?part=20