lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzYQenNtKPmWV=P3EsnqBsjNuAeXpC5ypL1k2z-H60i0=w@mail.gmail.com>
Date: Thu, 10 Oct 2024 18:48:26 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Ian Rogers <irogers@...gle.com>, 
	Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>, 
	linux-perf-users@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH 1/2] perf tools: Fix possible compiler warnings in hashmap

On Wed, Oct 9, 2024 at 1:20 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> The hashmap__for_each_entry[_safe] is accessing 'map' as if it's a
> pointer.  But it does without parentheses so passing a static hash map
> with an ampersand (like &slab_hash below) caused compiler warnings due
> to unmatched types.
>
>   In file included from util/bpf_lock_contention.c:5:
>   util/bpf_lock_contention.c: In function ‘exit_slab_cache_iter’:
>   linux/tools/perf/util/hashmap.h:169:32: error: invalid type argument of ‘->’ (have ‘struct hashmap’)
>     169 |         for (bkt = 0; bkt < map->cap; bkt++)                                \
>         |                                ^~
>   util/bpf_lock_contention.c:105:9: note: in expansion of macro ‘hashmap__for_each_entry’
>     105 |         hashmap__for_each_entry(&slab_hash, cur, bkt)
>         |         ^~~~~~~~~~~~~~~~~~~~~~~
>   /home/namhyung/project/linux/tools/perf/util/hashmap.h:170:31: error: invalid type argument of ‘->’ (have ‘struct hashmap’)
>     170 |                 for (cur = map->buckets[bkt]; cur; cur = cur->next)
>         |                               ^~
>   util/bpf_lock_contention.c:105:9: note: in expansion of macro ‘hashmap__for_each_entry’
>     105 |         hashmap__for_each_entry(&slab_hash, cur, bkt)
>         |         ^~~~~~~~~~~~~~~~~~~~~~~
>
> Cc: bpf@...r.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
> I've discovered this while prototyping the slab symbolization for perf
> lock contention.  So this code is not available yet but I'd like to fix
> the problem first.
>
> Also noticed bpf has the same code and the same problem.  I'll send a
> separate patch for them.
>

Yep, please do. Fixes look good, thanks.

>  tools/perf/util/hashmap.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/hashmap.h b/tools/perf/util/hashmap.h
> index c12f8320e6682d50..0c4f155e8eb745d9 100644
> --- a/tools/perf/util/hashmap.h
> +++ b/tools/perf/util/hashmap.h
> @@ -166,8 +166,8 @@ bool hashmap_find(const struct hashmap *map, long key, long *value);
>   * @bkt: integer used as a bucket loop cursor
>   */
>  #define hashmap__for_each_entry(map, cur, bkt)                             \
> -       for (bkt = 0; bkt < map->cap; bkt++)                                \
> -               for (cur = map->buckets[bkt]; cur; cur = cur->next)
> +       for (bkt = 0; bkt < (map)->cap; bkt++)                              \
> +               for (cur = (map)->buckets[bkt]; cur; cur = cur->next)
>
>  /*
>   * hashmap__for_each_entry_safe - iterate over all entries in hashmap, safe
> @@ -178,8 +178,8 @@ bool hashmap_find(const struct hashmap *map, long key, long *value);
>   * @bkt: integer used as a bucket loop cursor
>   */
>  #define hashmap__for_each_entry_safe(map, cur, tmp, bkt)                   \
> -       for (bkt = 0; bkt < map->cap; bkt++)                                \
> -               for (cur = map->buckets[bkt];                               \
> +       for (bkt = 0; bkt < (map)->cap; bkt++)                              \
> +               for (cur = (map)->buckets[bkt];                             \
>                      cur && ({tmp = cur->next; true; });                    \
>                      cur = tmp)
>
> @@ -190,19 +190,19 @@ bool hashmap_find(const struct hashmap *map, long key, long *value);
>   * @key: key to iterate entries for
>   */
>  #define hashmap__for_each_key_entry(map, cur, _key)                        \
> -       for (cur = map->buckets                                             \
> -                    ? map->buckets[hash_bits(map->hash_fn((_key), map->ctx), map->cap_bits)] \
> +       for (cur = (map)->buckets                                           \
> +                    ? (map)->buckets[hash_bits((map)->hash_fn((_key), (map)->ctx), (map)->cap_bits)] \
>                      : NULL;                                                \
>              cur;                                                           \
>              cur = cur->next)                                               \
> -               if (map->equal_fn(cur->key, (_key), map->ctx))
> +               if ((map)->equal_fn(cur->key, (_key), (map)->ctx))
>
>  #define hashmap__for_each_key_entry_safe(map, cur, tmp, _key)              \
> -       for (cur = map->buckets                                             \
> -                    ? map->buckets[hash_bits(map->hash_fn((_key), map->ctx), map->cap_bits)] \
> +       for (cur = (map)->buckets                                           \
> +                    ? (map)->buckets[hash_bits((map)->hash_fn((_key), (map)->ctx), (map)->cap_bits)] \
>                      : NULL;                                                \
>              cur && ({ tmp = cur->next; true; });                           \
>              cur = tmp)                                                     \
> -               if (map->equal_fn(cur->key, (_key), map->ctx))
> +               if ((map)->equal_fn(cur->key, (_key), (map)->ctx))
>
>  #endif /* __LIBBPF_HASHMAP_H */
> --
> 2.47.0.rc0.187.ge670bccf7e-goog
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ