| Message ID | 20211108061316.203217-7-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Future-proof more tricky libbpf APIs | expand |
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? Maybe checking for ops type would be more robust ? Like: #define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr( \ __builtin_types_compatible_p(typeof(a4), const struct btf_dump_opts*), \ btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4), \ btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4))
On Mon, Nov 8, 2021 at 7:38 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> 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. > > Maybe checking for ops type would be more robust ? opts can be NULL. At which point it's actually only compatible with void *. The only non-null args are btf (same for both variants, so no go) or callback. So I have to check the callback. From my experimentations with various possible invocations (locally, not in any test), this check is quite robust. I need to check two types: pointer to func and func itself (C allows that, turns out; I learned it from [0]). [0] https://github.com/cilium/ebpf/issues/214 > Like: > #define btf_dump__new(a1, a2, a3, a4) __builtin_choose_expr( \ > __builtin_types_compatible_p(typeof(a4), const struct btf_dump_opts*), \ > btf_dump__new((void *)a1, (void *)a2, (void *)a3, (void *)a4), \ > btf_dump__new_deprecated((void *)a1, (void *)a2, (void *)a3, (void *)a4))
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@gmail.com> 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 ? Not sure why LLVM took that approach. 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. > > > > 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?
On Tue, Nov 9, 2021 at 9:40 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> 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@gmail.com> 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).
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 6aae4f62ee0b..a6e528faebf9 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -267,15 +267,59 @@ LIBBPF_API int btf__dedup_deprecated(struct btf *btf, struct btf_ext *btf_ext, c struct btf_dump; struct btf_dump_opts { - void *ctx; + union { + size_t sz; + void *ctx; /* DEPRECATED: will be gone in v1.0 */ + }; }; typedef void (*btf_dump_printf_fn_t)(void *ctx, const char *fmt, va_list args); LIBBPF_API struct btf_dump *btf_dump__new(const struct btf *btf, - const struct btf_ext *btf_ext, - const struct btf_dump_opts *opts, - btf_dump_printf_fn_t printf_fn); + btf_dump_printf_fn_t printf_fn, + void *ctx, + const struct btf_dump_opts *opts); + +LIBBPF_API struct btf_dump *btf_dump__new_v0_6_0(const struct btf *btf, + btf_dump_printf_fn_t printf_fn, + void *ctx, + const struct btf_dump_opts *opts); + +LIBBPF_API struct btf_dump *btf_dump__new_deprecated(const struct btf *btf, + const struct btf_ext *btf_ext, + const struct btf_dump_opts *opts, + btf_dump_printf_fn_t printf_fn); + +/* Choose either btf_dump__new() or btf_dump__new_deprecated() based on the + * type of 4th argument. If it's btf_dump's print callback, use deprecated + * API; otherwise, choose the new btf_dump__new(). ___libbpf_override() + * doesn't work here because both variants have 4 input arguments. + * + * (void *) casts are necessary to avoid compilation warnings about type + * mismatches, because even though __builtin_choose_expr() only ever evaluates + * one side the other side still has to satisfy type constraints (this is + * compiler implementation limitation which might be lifted eventually, + * according to the documentation). So passing struct btf_ext in place of + * btf_dump_printf_fn_t would be generating compilation warning. Casting to + * void * avoids this issue. + * + * Also, two type compatibility checks for a function and function pointer are + * required because passing function reference into btf_dump__new() as + * btf_dump__new(..., my_callback, ...) and as btf_dump__new(..., + * &my_callback, ...) (not explicit ampersand in the latter case) actually + * differs as far as __builtin_types_compatible_p() is concerned. Thus two + * checks are combined to detect callback argument. + * + * The rest works just like in case of ___libbpf_override() usage with symbol + * versioning. + */ +#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)) + LIBBPF_API void btf_dump__free(struct btf_dump *d); LIBBPF_API int btf_dump__dump_type(struct btf_dump *d, __u32 id); diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index 17db62b5002e..0ed9c2f93322 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -77,9 +77,8 @@ struct btf_dump_data { struct btf_dump { const struct btf *btf; - const struct btf_ext *btf_ext; btf_dump_printf_fn_t printf_fn; - struct btf_dump_opts opts; + void *cb_ctx; int ptr_sz; bool strip_mods; bool skip_anon_defs; @@ -138,17 +137,18 @@ static void btf_dump_printf(const struct btf_dump *d, const char *fmt, ...) va_list args; va_start(args, fmt); - d->printf_fn(d->opts.ctx, fmt, args); + d->printf_fn(d->cb_ctx, fmt, args); va_end(args); } static int btf_dump_mark_referenced(struct btf_dump *d); static int btf_dump_resize(struct btf_dump *d); -struct btf_dump *btf_dump__new(const struct btf *btf, - const struct btf_ext *btf_ext, - const struct btf_dump_opts *opts, - btf_dump_printf_fn_t printf_fn) +DEFAULT_VERSION(btf_dump__new_v0_6_0, btf_dump__new, LIBBPF_0.6.0) +struct btf_dump *btf_dump__new_v0_6_0(const struct btf *btf, + btf_dump_printf_fn_t printf_fn, + void *ctx, + const struct btf_dump_opts *opts) { struct btf_dump *d; int err; @@ -158,9 +158,8 @@ struct btf_dump *btf_dump__new(const struct btf *btf, return libbpf_err_ptr(-ENOMEM); d->btf = btf; - d->btf_ext = btf_ext; d->printf_fn = printf_fn; - d->opts.ctx = opts ? opts->ctx : NULL; + d->cb_ctx = ctx; d->ptr_sz = btf__pointer_size(btf) ? : sizeof(void *); d->type_names = hashmap__new(str_hash_fn, str_equal_fn, NULL); @@ -186,6 +185,15 @@ struct btf_dump *btf_dump__new(const struct btf *btf, return libbpf_err_ptr(err); } +COMPAT_VERSION(btf_dump__new_deprecated, btf_dump__new, LIBBPF_0.0.4) +struct btf_dump *btf_dump__new_deprecated(const struct btf *btf, + const struct btf_ext *btf_ext, + const struct btf_dump_opts *opts, + btf_dump_printf_fn_t printf_fn) +{ + return btf_dump__new_v0_6_0(btf, printf_fn, opts ? opts->ctx : NULL, opts); +} + static int btf_dump_resize(struct btf_dump *d) { int err, last_id = btf__type_cnt(d->btf) - 1; diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index edc40bc16f19..c51411d46c9e 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -405,4 +405,6 @@ LIBBPF_0.6.0 { btf__dedup_deprecated; btf__raw_data; btf__type_cnt; + btf_dump__new; + btf_dump__new_deprecated; } LIBBPF_0.5.0;
Change btf_dump__new() and corresponding struct btf_dump_ops structure to be extensible by using OPTS "framework" ([0]). Given we don't change the names, we use a similar approach as with bpf_prog_load(), but this time we ended up with two APIs with the same name and same number of arguments, so overloading based on number of arguments with ___libbpf_override() doesn't work. Instead, use "overloading" based on types. In this particular case, print callback has to be specified, so we detect which argument is a callback. If it's 4th (last) argument, old implementation of API is used by user code. If not, it must be 2nd, and thus new implementation is selected. The rest is handled by the same symbol versioning approach. btf_ext argument is dropped as it was never used and isn't necessary either. If in the future we'll need btf_ext, that will be added into OPTS-based struct btf_dump_opts. struct btf_dump_opts is reused for both old API and new APIs. ctx field is marked deprecated in v0.7+ and it's put at the same memory location as OPTS's sz field. Any user of new-style btf_dump__new() will have to set sz field and doesn't/shouldn't use ctx, as ctx is now passed along the callback as mandatory input argument, following the other APIs in libbpf that accept callbacks consistently. Again, this is quite ugly in implementation, but is done in the name of backwards compatibility and uniform and extensible future APIs (at the same time, sigh). And it will be gone in libbpf 1.0. [0] Closes: https://github.com/libbpf/libbpf/issues/283 Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/btf.h | 52 ++++++++++++++++++++++++++++++++++++---- tools/lib/bpf/btf_dump.c | 26 +++++++++++++------- tools/lib/bpf/libbpf.map | 2 ++ 3 files changed, 67 insertions(+), 13 deletions(-)