On Fri, Nov 11, 2022 at 9:19 AM <sdf@xxxxxxxxxx> wrote:
>
> On 11/11, Eduard Zingerman wrote:
> > On Thu, 2022-11-10 at 17:34 -0800, sdf@xxxxxxxxxx wrote:
> > > On 11/10, Stanislav Fomichev wrote:
> > > > On 11/11, Eduard Zingerman wrote:
> > > > > A fix for the LLVM compilation error while building bpftool.
> > > > > Replaces the expression:
> > > > >
> > > > > _Static_assert((p) == NULL || ...)
> > > > >
> > > > > by expression:
> > > > >
> > > > > _Static_assert((__builtin_constant_p((p)) ? (p) == NULL : 0) |
> > | ...)
> > >
> > > > IIUC, when __builtin_constant_p(p) returns false, we just ignore the
> > NULL
> > > > check?
> > > > Do we have cases like that? If no, maybe it's safer to fail?
> > >
> > > > s/(p) == NULL : 0/(p) == NULL : 1/ ?
> > >
> > > I'm probably missing something, can you pls clarify? So the error is as
> > > follows:
> > >
> > > > > btf_dump.c:1546:2: error: static_assert expression is not an
> > integral
> > > > > constant expression
> > > hashmap__find(name_map, orig_name, &dup_cnt);
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./hashmap.h:169:35: note: expanded from macro 'hashmap__find'
> > > hashmap_find((map), (long)(key), hashmap_cast_ptr(value))
> > > ^~~~~~~~~~~~~~~~~~~~~~~
> > > ./hashmap.h:126:17: note: expanded from macro 'hashmap_cast_ptr'
> > > _Static_assert((p) == NULL || sizeof(*(p)) ==
> > > sizeof(long),
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > btf_dump.c:1546:2: note: cast from 'void *' is not allowed in a
> > constant
> > > expression
> > > ./hashmap.h:169:35: note: expanded from macro 'hashmap__find'
> > > hashmap_find((map), (long)(key), hashmap_cast_ptr(value))
> > >
> > > This line in particular:
> > >
> > > btf_dump.c:1546:2: note: cast from 'void *' is not allowed in a
> > constant
> > > expression
> > >
> > > And the code does:
> > >
> > > size_t dup_cnt = 0;
> > > hashmap__find(name_map, orig_name, &dup_cnt);
> > >
> > > So where is that cast from 'void *' is happening? Is it the NULL check
> > > itself?
> > >
> > > Are we simply guarding against the user calling hashmap_cast_ptr with
> > > explicit NULL argument?
>
> > In case if (p) is not a constant I want the second part of the || to
> > kick-in.
> > The complete condition looks as follows:
>
> > _Static_assert((__builtin_constant_p((p)) ? (p) == NULL : 0) || \
> > sizeof(*(p)) == sizeof(long), "...error...")
>
> > The intent is to check that (p) is either NULL or a pointer to
> > something of size long. So, if (p) is not a constant the expression
> > would be equivalent to:
>
> > _Static_assert(0 || sizeof(*(p)) == sizeof(long), "...error...")
>
> > I just tried the following:
>
> > struct hashmap *name_map;
> > char x; // not a constant, wrong pointer size
> > ...
> > hashmap__find(name_map, orig_name, &x);
>
> > And it fails with an error message as intended:
>
> > btf_dump.c:1548:2: error: static_assert failed due to
> > requirement '(__builtin_constant_p((&x)) ? (&x) == ((void *)0) : 0) ||
> > sizeof (*(&x)) == sizeof(long)' "&x pointee should be a long-sized
> > integer or a pointer"
> > hashmap__find(name_map, orig_name, &x);
> > ./hashmap.h:170:35: note: expanded from macro 'hashmap__find'
> > hashmap_find((map), (long)(key), hashmap_cast_ptr(value))
> > ./hashmap.h:126:2: note: expanded from macro 'hashmap_cast_ptr'
> > _Static_assert((__builtin_constant_p((p)) ? (p) == NULL : 0) ||
>
> Awesome, thanks for clarifying!
>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>
Added
Fixes: c302378bc157 ("libbpf: Hashmap interface update to allow both
long and void* keys/values")
and moved links to be before Signed-off-by (they should be last).
Pushed to bpf-next, thanks.
> > >
> > > > > When "p" is not a constant the former is not considered to be a
> > > > > constant expression by LLVM 14.
> > > > >
> > > > > The error was introduced in the following patch-set: [1].
> > > > > The error was reported here: [2].
> > > > >
> > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > > > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > > > >
> > > > > [1]
> > > > https://lore.kernel.org/bpf/20221109142611.879983-1-eddyz87@xxxxxxxxx/
> > > > > [2] https://lore.kernel.org/all/202211110355.BcGcbZxP-lkp@xxxxxxxxx/
> > > > > ---
> > > > > tools/lib/bpf/hashmap.h | 3 ++-
> > > > > tools/perf/util/hashmap.h | 3 ++-
> > > > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
> > > > > index 3fe647477bad..0a5bf1937a7c 100644
> > > > > --- a/tools/lib/bpf/hashmap.h
> > > > > +++ b/tools/lib/bpf/hashmap.h
> > > > > @@ -123,7 +123,8 @@ enum hashmap_insert_strategy {
> > > > > };
> > > > >
> > > > > #define hashmap_cast_ptr(p) ({ \
> > > > > - _Static_assert((p) == NULL || sizeof(*(p)) == sizeof(long), \
> > > > > + _Static_assert((__builtin_constant_p((p)) ? (p) == NULL : 0) ||
> > \
> > > > > + sizeof(*(p)) == sizeof(long), \
> > > > > #p " pointee should be a long-sized integer or a
> > pointer"); \
> > > > > (long *)(p); \
> > > > > })
> > > > > diff --git a/tools/perf/util/hashmap.h b/tools/perf/util/hashmap.h
> > > > > index 3fe647477bad..0a5bf1937a7c 100644
> > > > > --- a/tools/perf/util/hashmap.h
> > > > > +++ b/tools/perf/util/hashmap.h
> > > > > @@ -123,7 +123,8 @@ enum hashmap_insert_strategy {
> > > > > };
> > > > >
> > > > > #define hashmap_cast_ptr(p) ({ \
> > > > > - _Static_assert((p) == NULL || sizeof(*(p)) == sizeof(long), \
> > > > > + _Static_assert((__builtin_constant_p((p)) ? (p) == NULL : 0) ||
> > \
> > > > > + sizeof(*(p)) == sizeof(long), \
> > > > > #p " pointee should be a long-sized integer or a
> > pointer"); \
> > > > > (long *)(p); \
> > > > > })
> > > > > --
> > > > > 2.34.1
> > > > >
>
