Re: [PATCH v2 bpf-next 4/9] libbpf: ensure btf_dump__new() and btf_dump_opts are future-proof

[Jiri Olsa <jolsa@redhat.com>]

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@kernel.org>
> ---
>  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();