On Wed, Oct 9, 2024 at 1:20 PM Namhyung Kim <namhyung@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> 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
>
>
