| Message ID | 20241009202009.884884-1-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] perf tools: Fix possible compiler warnings in hashmap | expand |
On Wed, Oct 9, 2024 at 1:20 PM Namhyung Kim <namhyung@kernel.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@vger.kernel.org > Signed-off-by: Namhyung Kim <namhyung@kernel.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 > >
On Thu, Oct 10, 2024 at 06:48:26PM -0700, Andrii Nakryiko wrote: > On Wed, Oct 9, 2024 at 1:20 PM Namhyung Kim <namhyung@kernel.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@vger.kernel.org > > Signed-off-by: Namhyung Kim <namhyung@kernel.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. Sure will do, can I get your Acked-by for this patch? Thanks, Namhyung > > > 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 > > > >
On Fri, Oct 11, 2024 at 9:44 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Thu, Oct 10, 2024 at 06:48:26PM -0700, Andrii Nakryiko wrote: > > On Wed, Oct 9, 2024 at 1:20 PM Namhyung Kim <namhyung@kernel.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@vger.kernel.org > > > Signed-off-by: Namhyung Kim <namhyung@kernel.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. > > Sure will do, can I get your Acked-by for this patch? > Sure: Acked-by: Andrii Nakryiko <andrii@kernel.org> > Thanks, > Namhyung > > > > > > 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 > > > > > >
On Wed, 09 Oct 2024 13:20:08 -0700, Namhyung Kim 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) > | ^~~~~~~~~~~~~~~~~~~~~~~ > > [...] Applied to perf-tools-next, thanks! Best regards, Namhyung
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 */
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@vger.kernel.org Signed-off-by: Namhyung Kim <namhyung@kernel.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. tools/perf/util/hashmap.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)