Re: [PATCH bpf-next] libbpf: hashmap.h update to fix build issues using LLVM14

[sdf@google.com]

On 11/11, Eduard Zingerman wrote:
> On Thu, 2022-11-10 at 17:34 -0800, sdf@google.com 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@google.com>

> >
> > > > 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@intel.com>
> > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > >
> > > > [1]
> > > https://lore.kernel.org/bpf/20221109142611.879983-1-eddyz87@gmail.com/
> > > > [2] https://lore.kernel.org/all/202211110355.BcGcbZxP-lkp@intel.com/
> > > > ---
> > > >  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
> > > >