On Wed, Nov 10, 2021 at 09:36:19PM -0800, Andrii Nakryiko wrote: > 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@xxxxxxxxxx> > --- > tools/lib/bpf/btf.h | 51 ++++++++++++++++++++++++++++++++++++---- > tools/lib/bpf/btf_dump.c | 31 +++++++++++++++++------- > tools/lib/bpf/libbpf.map | 2 ++ > 3 files changed, 71 insertions(+), 13 deletions(-) > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > index 6aae4f62ee0b..45310c65e865 100644 > --- a/tools/lib/bpf/btf.h > +++ b/tools/lib/bpf/btf.h > @@ -267,15 +267,58 @@ 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)) hi, this change breaks bpftrace g++ build that includes btf.h, because there's no typeof and __builtin_types_compatible_p in c++ I guess there could be some c++ solution doing similar check, but I wonder we want to polute btf.h with that, I'll need to check on that I think I can add some detection code to bpftrace, to find out which version of btf_dump__new to use the build error can be generated with test_cpp.cpp below, so far I'm using __cplusplus ifdef in btf.h to workaround the issue thoughts? thanks, jirka --- diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 742a2bf71c5e..bd2d77979571 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -314,11 +314,13 @@ LIBBPF_API struct btf_dump *btf_dump__new_deprecated(const struct btf *btf, * The rest works just like in case of ___libbpf_override() usage with symbol * versioning. */ +#ifndef __cplusplus #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)) +#endif LIBBPF_API void btf_dump__free(struct btf_dump *d); diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp index a8d2e9a87fbf..e00201de2890 100644 --- a/tools/testing/selftests/bpf/test_cpp.cpp +++ b/tools/testing/selftests/bpf/test_cpp.cpp @@ -7,9 +7,15 @@ /* do nothing, just make sure we can link successfully */ +static void dump_printf(void *ctx, const char *fmt, va_list args) +{ +} + int main(int argc, char *argv[]) { + struct btf_dump_opts opts = { }; struct test_core_extern *skel; + struct btf *btf; /* libbpf.h */ libbpf_set_print(NULL); @@ -18,7 +24,8 @@ int main(int argc, char *argv[]) bpf_prog_get_fd_by_id(0); /* btf.h */ - btf__new(NULL, 0); + btf = btf__new(NULL, 0); + btf_dump__new(btf, dump_printf, nullptr, &opts); /* BPF skeleton */ skel = test_core_extern__open_and_load();
