On Tue, Nov 9, 2021 at 9:40 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Nov 09, 2021 at 07:37:48AM -0800, Andrii Nakryiko wrote:
> > On Mon, Nov 8, 2021 at 7:38 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Sun, Nov 07, 2021 at 10:13:11PM -0800, Andrii Nakryiko wrote:
> > > > +#define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr( \
> > > > + __builtin_types_compatible_p(typeof(a4), btf_dump_printf_fn_t) + \
> > > > + __builtin_types_compatible_p(typeof(a4), \
> > > > + void(void *, const char *, va_list)), \
> > > > + btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4),\
> > > > + btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4))
> > >
> > > why '+' in the above? The return type of __builtin_types_compatible_p() is bool.
> > > What is bool + bool ?
> > > It suppose to be logical 'OR', right?
> >
> > __builtin_types_compatible_p() is defined as returning 0 or 1 (not
> > true/false). And __builtin_choose_expr() is also defined as comparing
> > first argument against 0, not as true/false. But in practice it
> > doesn't matter because bool is converted to 0 or 1 in arithmetic
> > operations. So I can switch to || with no effect. Let me know if you
> > still prefer logical || and I'll change.
> >
> > But yes to your last question, it's logical OR.
>
> Interesting. Looking at LLVM code it does indeed returns 'int'.
> At least typeof(_builtin_types_compatible_p(..)) seems to be 'int'.
>
> At the same type LLVM tests are using this macro:
> #define check_same_type(type1, type2) __builtin_types_compatible_p(type1, type2) && __builtin_types_compatible_p(type1 *, type2 *)
>
> While kernel has this macro:
> #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>
> Guessing that extra typeof() may resolve the difference between fn and &fn ?
no, btf_dump_printf_fn_t is already a type and typeof() doesn't seem
to change anything. I had a test like below. It produces the same
results with or without typeof.
static void test1(btf_dump_printf_fn_t arg, int val1, int val2, int
val3, int val4)
{
fprintf(stderr, "TEST1 VAL %d %d %d %d\n", val1, val2, val3, val4);
}
static void test2(struct btf *arg, int val1, int val2, int val3, int val4)
{
fprintf(stderr, "TEST2 VAL %d %d %d %d\n", val1, val2, val3, val4);
}
#define test_variad(arg) \
__builtin_choose_expr(\
__builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)) + \
__builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
test1((void *)arg,\
__builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)),\
__builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
__builtin_types_compatible_p(typeof(arg), struct btf *),\
__builtin_types_compatible_p(typeof(arg), void *)\
), \
test2((void *)arg,\
__builtin_types_compatible_p(typeof(arg),
typeof(btf_dump_printf_fn_t)),\
__builtin_types_compatible_p(typeof(arg),
typeof(void(void *, const char *, va_list))),\
__builtin_types_compatible_p(typeof(arg), struct btf *),\
__builtin_types_compatible_p(typeof(arg), void *)\
)\
)
And then I call
test_variad(codegen_btf_dump_printf);
test_variad(&codegen_btf_dump_printf);
test_variad(btf);
test_variad(NULL);
I always get this (both with and without extra typeof()):
TEST1 VAL 0 1 0 0
TEST1 VAL 1 0 0 0
TEST2 VAL 0 0 1 0
TEST2 VAL 0 0 0 1
> Not sure why LLVM took that approach.
I think kernel's __same_type() doesn't handle this well. I've changed
kernel/bpf/hashtab.c like this:
BUILD_BUG_ON(!__same_type(&__htab_map_lookup_elem,
- (void *(*)(struct bpf_map *map, void *key))NULL));
+ void *(struct bpf_map *map, void *key)));
And it triggered compilation assertion.
Then I "fixed" it like this:
- BUILD_BUG_ON(!__same_type(&__htab_map_lookup_elem,
- (void *(*)(struct bpf_map *map, void *key))NULL));
+ BUILD_BUG_ON(!__same_type(__htab_map_lookup_elem,
+ void *(struct bpf_map *map, void *key)));
And it compiled just fine. So __same_type() is sensitive to &.
>
> Anyway it will be removed once we hit 1.0, so no need to dig too deep.
> I think changing + to || is still worth doing.
ok, I'll update in next revision
>
> > >
> > > Maybe checking for ops type would be more robust ?
> >
> > opts can be NULL. At which point it's actually only compatible with
> > void *.
>
> Assuming that fn pointer in btf_dump__new_deprecated() will never be NULL?
If fn pointer is NULL, btf_dump APIs will crash in runtime. I'll add
an explicit check and error out in such case with -EINVAL.
The way the check is written right now, if someone passes NULL we'll
choose non-deprecated btf_dump__new() variant, which seems to be a
good "default" (even though it will still crash later when calling a
callback).
