On Fri, May 17, 2024 at 12:06 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > Add several API functions to allow more flexibility with btf dump: > - int btf_dump__order_type(struct btf_dump *d, __u32 id); > adds a type and all it's dependencies to the emit queue > in topological order; > - struct btf_dump_emit_queue_item *btf_dump__emit_queue(struct btf_dump *d); > __u32 btf_dump__emit_queue_cnt(struct btf_dump *d); > provide access to the emit queue owned by btf_dump object; > - int btf_dump__dump_one_type(struct btf_dump *d, __u32 id, bool fwd); > prints a given type in C format (skipping any dependencies). > > This API should allow to do the following on the libbpf client side: > - filter printed types using arbitrary criteria; > - add arbitrary type attributes or pre-processor statements for > selected types. > > This is a follow-up to the following discussion: > https://lore.kernel.org/bpf/20240503111836.25275-1-jose.marchesi@xxxxxxxxxx/ > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- > tools/lib/bpf/btf.h | 33 ++++++++++++++++++++++ > tools/lib/bpf/btf_dump.c | 61 ++++++++++++++++++++++------------------ > tools/lib/bpf/libbpf.map | 4 +++ > 3 files changed, 71 insertions(+), 27 deletions(-) > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h > index 8e6880d91c84..81d70ac35562 100644 > --- a/tools/lib/bpf/btf.h > +++ b/tools/lib/bpf/btf.h > @@ -249,6 +249,39 @@ LIBBPF_API void btf_dump__free(struct btf_dump *d); > > LIBBPF_API int btf_dump__dump_type(struct btf_dump *d, __u32 id); > > +/* Dumps C language definition or forward declaration for type **id**: > + * - returns 1 if type is printable; > + * - returns 0 if type is non-printable. does it also return <0 on error? > + */ let's follow the format of doc comments, see other APIs. There is @brief, @param, @return and so on. pw-bot: cr > +LIBBPF_API int btf_dump__dump_one_type(struct btf_dump *d, __u32 id, bool fwd); not a fan of a name, how about we do `btf_dump__emit_type(struct btf_dump *d, __u32 id, struct btf_dump_emit_type_opts *opts)` and have forward declaration flag as options? We have btf_dump__emit_type_decl(), this one could be called btf_dump__emit_type_def() as well. WDYT? > + > +/* **struct btf_dump** tracks a list of types that should be dumped, > + * these types are sorted in the topological order satisfying C language semantics: > + * - if type A includes type B (e.g. A is a struct with a field of type B), > + * then B comes before A; > + * - if type A references type B via a pointer > + * (e.g. A is a struct with a field of type pointer to B), > + * then B's forward declaration comes before A. > + * > + * **struct btf_dump_emit_queue_item** represents a single entry of the emit queue. > + */ > +struct btf_dump_emit_queue_item { > + __u32 id:31; > + __u32 fwd:1; > +}; as mentioned on patch #1, I'd add this type in patch #1 (and just move it to public API header here). And instead of bit fields, let's use two fields. Those few bytes extra doesn't really matter much in practice. > + > +/* Adds type **id** and it's dependencies to the emit queue. */ typo: its > +LIBBPF_API int btf_dump__order_type(struct btf_dump *d, __u32 id); > + > +/* Provides access to currently accumulated emit queue, > + * returned pointer is owned by **struct btf_dump** and should not be > + * freed explicitly. > + */ > +LIBBPF_API struct btf_dump_emit_queue_item *btf_dump__emit_queue(struct btf_dump *d); > + > +/* Returns the size of currently accumulated emit queue */ > +LIBBPF_API __u32 btf_dump__emit_queue_cnt(struct btf_dump *d); > + I'm a bit on the fence here. But I feel like having just one access to the queue which returns size as out parameter is probably a bit better. Having queue pointer + size returned in one API communicates them being both tied together and sort of ephemeral. We should also document what's the lifetime of this pointer and when it can be invalidated. Speaking of which, for the next revision, can you also integrate all these new APIs into bpftool to handle the problem that Jose tried to solve? This might also expose any of the potential issues with API usage. > struct btf_dump_emit_type_decl_opts { > /* size of this struct, for forward/backward compatiblity */ > size_t sz; > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > index 10532ae9ff14..c3af6bb606a0 100644 > --- a/tools/lib/bpf/btf_dump.c > +++ b/tools/lib/bpf/btf_dump.c > @@ -85,10 +85,7 @@ struct btf_dump { > size_t cached_names_cap; > > /* topo-sorted list of dependent type definitions */ > - struct { > - __u32 id:31; > - __u32 fwd:1; > - } *emit_queue; > + struct btf_dump_emit_queue_item *emit_queue; > int emit_queue_cap; > int emit_queue_cnt; > > @@ -250,7 +247,6 @@ void btf_dump__free(struct btf_dump *d) > } > > static int btf_dump_order_type(struct btf_dump *d, __u32 id, __u32 cont_id, bool through_ptr); > -static void btf_dump_emit_type(struct btf_dump *d, __u32 id, bool fwd); > > /* > * Dump BTF type in a compilable C syntax, including all the necessary > @@ -296,12 +292,32 @@ int btf_dump__dump_type(struct btf_dump *d, __u32 id) > break; > }; > > - for (i = 0; i < d->emit_queue_cnt; i++) > - btf_dump_emit_type(d, d->emit_queue[i].id, d->emit_queue[i].fwd); > + for (i = 0; i < d->emit_queue_cnt; i++) { > + err = btf_dump__dump_one_type(d, d->emit_queue[i].id, d->emit_queue[i].fwd); > + if (err < 0) > + return libbpf_err(err); > + if (err > 0) > + btf_dump_printf(d, ";\n\n"); > + } > > return 0; > } > > +int btf_dump__order_type(struct btf_dump *d, __u32 id) > +{ > + return btf_dump_order_type(d, id, id, false); make sure that btf_dump_order_type() either sets errno, or use libbpf_err() helpers here > +} > + > +struct btf_dump_emit_queue_item *btf_dump__emit_queue(struct btf_dump *d) > +{ > + return d->emit_queue; > +} > + > +__u32 btf_dump__emit_queue_cnt(struct btf_dump *d) > +{ > + return d->emit_queue_cnt; > +} > + > /* > * Mark all types that are referenced from any other type. This is used to > * determine top-level anonymous enums that need to be emitted as an > @@ -382,7 +398,7 @@ static int btf_dump_mark_referenced(struct btf_dump *d) > > static int __btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id, bool fwd) > { > - typeof(d->emit_queue[0]) *new_queue = NULL; > + struct btf_dump_emit_queue_item *new_queue = NULL; > size_t new_cap; > > if (d->emit_queue_cnt >= d->emit_queue_cap) { > @@ -733,7 +749,7 @@ static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map, > * that doesn't comply to C rules completely), algorithm will try to proceed > * and produce as much meaningful output as possible. > */ > -static void btf_dump_emit_type(struct btf_dump *d, __u32 id, bool fwd) > +int btf_dump__dump_one_type(struct btf_dump *d, __u32 id, bool fwd) double check libbpf_err(), libbpf promises to set errno properly for all public APIs > { > const struct btf_type *t; > __u16 kind; [...] > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index c1ce8aa3520b..137e4cbaa7a7 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -422,4 +422,8 @@ LIBBPF_1.5.0 { > bpf_program__attach_sockmap; > ring__consume_n; > ring_buffer__consume_n; > + btf_dump__emit_queue; > + btf_dump__emit_queue_cnt; > + btf_dump__order_type; > + btf_dump__dump_one_type; this has to be ordered > } LIBBPF_1.4.0; > -- > 2.34.1 >
