diff mbox series

[bpf-next,v2,2/4] libbpf: API to access btf_dump emit queue and print single type

Message ID 20240517190555.4032078-3-eddyz87@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series API to access btf_dump emit queue and print single type | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 fail Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 fail Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 fail Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 fail Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 fail Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers warning 6 maintainers not CCed: jolsa@kernel.org john.fastabend@gmail.com kpsingh@kernel.org haoluo@google.com song@kernel.org sdf@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 24 this patch: 24
netdev/source_inline success Was 0 now: 0

Commit Message

Eduard Zingerman May 17, 2024, 7:05 p.m. UTC 
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@oracle.com/

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 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(-)

Comments

Andrii Nakryiko May 28, 2024, 10:18 p.m. UTC | #1 
On Fri, May 17, 2024 at 12:06 PM Eduard Zingerman <eddyz87@gmail.com> 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@oracle.com/
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  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
>
Eduard Zingerman May 28, 2024, 10:53 p.m. UTC | #2 
[...]

> > +/* 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?

Right

> 
> > + */
> 
> let's follow the format of doc comments, see other APIs. There is
> @brief, @param, @return and so on.

Will do

> 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?

`btf_dump__emit_type_def` seems good and I can make it accept options
with forward as a flag.

However, in such a case the following is also a contender:

struct btf_dump_type_opts {
	__u32 sz;
        bool skip_deps;		/* flags picked so that by default	 */
        bool forward_only;	/* the behavior matches non-opts variant */
};

LIBBPF_API int btf_dump__dump_type_opts(struct btf_dump *d, __u32 id,
                                        struct btf_dump_type_opts *opts);


I find this contender more ugly but a bit more consistent.
Wdyt?

[...]
Andrii Nakryiko May 28, 2024, 11:19 p.m. UTC | #3 
On Tue, May 28, 2024 at 3:53 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> [...]
>
> > > +/* 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?
>
> Right
>
> >
> > > + */
> >
> > let's follow the format of doc comments, see other APIs. There is
> > @brief, @param, @return and so on.
>
> Will do
>
> > 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?
>
> `btf_dump__emit_type_def` seems good and I can make it accept options
> with forward as a flag.
>
> However, in such a case the following is also a contender:
>
> struct btf_dump_type_opts {
>         __u32 sz;
>         bool skip_deps;         /* flags picked so that by default       */
>         bool forward_only;      /* the behavior matches non-opts variant */
> };
>
> LIBBPF_API int btf_dump__dump_type_opts(struct btf_dump *d, __u32 id,
>                                         struct btf_dump_type_opts *opts);
>
>
> I find this contender more ugly but a bit more consistent.
> Wdyt?

You'll also need "skip_semicolon" which makes this even uglier. Which
is why I'd not do it as an extension to btf_dump__dump_type() API.


>
> [...]
Eduard Zingerman May 28, 2024, 11:39 p.m. UTC | #4 
On Tue, 2024-05-28 at 16:19 -0700, Andrii Nakryiko wrote:

[...]

> > > > +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?
> > 
> > `btf_dump__emit_type_def` seems good and I can make it accept options
> > with forward as a flag.
> > 
> > However, in such a case the following is also a contender:
> > 
> > struct btf_dump_type_opts {
> >         __u32 sz;
> >         bool skip_deps;         /* flags picked so that by default       */
> >         bool forward_only;      /* the behavior matches non-opts variant */
> > };
> > 
> > LIBBPF_API int btf_dump__dump_type_opts(struct btf_dump *d, __u32 id,
> >                                         struct btf_dump_type_opts *opts);
> > 
> > 
> > I find this contender more ugly but a bit more consistent.
> > Wdyt?
> 
> You'll also need "skip_semicolon" which makes this even uglier. Which
> is why I'd not do it as an extension to btf_dump__dump_type() API.

Right, forgot about this one.
Ok, let's make it `btf_dump__emit_type_def`, thank you.
Eduard Zingerman June 1, 2024, 7:22 a.m. UTC | #5 
On Tue, 2024-05-28 at 15:18 -0700, Andrii Nakryiko wrote:

[...]

> 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.

Hi Andrii,

Good foresight requesting to re-implement Jose's patch on top of the
new API. I did the changes you requested for v1 + tried to make the
bpftool changes, results are here:

https://github.com/eddyz87/bpf/tree/libbpf-sort-for-dump-api-2

The attempt falls flat for the following pattern:

  #define __pai __attribute__((preserve_access_index))
  typedef struct { int x; } foo  __pai;

With the following clang error:

  t.c:2:31: error: 'preserve_access_index' attribute only applies to structs, unions, and classes
    2 | typedef struct { int x; } foo __pai;

The correct syntax for this definition is as below:

  typedef struct { int x; } __pai foo;

This cannot be achieved unless printing of typedefs is done by some
custom code in bpftool.

So, it looks like we won't be able to ditch callbacks in the end.
Maybe the code for emit queue could be salvaged for the module thing
you talked about, please provide a bit more context about it.

Thanks,
Eduard
Andrii Nakryiko June 4, 2024, 5:39 p.m. UTC | #6 
On Sat, Jun 1, 2024 at 12:22 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-05-28 at 15:18 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > 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.
>
> Hi Andrii,
>
> Good foresight requesting to re-implement Jose's patch on top of the

yay, I guess it was :)

> new API. I did the changes you requested for v1 + tried to make the
> bpftool changes, results are here:
>
> https://github.com/eddyz87/bpf/tree/libbpf-sort-for-dump-api-2
>
> The attempt falls flat for the following pattern:
>
>   #define __pai __attribute__((preserve_access_index))
>   typedef struct { int x; } foo  __pai;
>
> With the following clang error:
>
>   t.c:2:31: error: 'preserve_access_index' attribute only applies to structs, unions, and classes
>     2 | typedef struct { int x; } foo __pai;
>
> The correct syntax for this definition is as below:
>
>   typedef struct { int x; } __pai foo;
>
> This cannot be achieved unless printing of typedefs is done by some
> custom code in bpftool.

Right, though in this case it probably is still achieved with using
btf_dump__emit_type_decl() if bpftool detects TYPEDEF -> (anon) STRUCT
pattern.

But we can get deeper, thanks to horrendous C syntax:

typedef struct { int x; } struct_arr[10];

I think it still is achievable with btf_dump__emit_type_decl() setting
.field_name option to "__pai struct_arr". It does feel like a hack, of
course, but should work.

In general, typedef is equivalent to field definition (which is
intentional by original C syntax inventors, I believe), so maybe
that's one way to address this.

>
> So, it looks like we won't be able to ditch callbacks in the end.

hopefully we can avoid this still, let's give it some more thought
before we give up

> Maybe the code for emit queue could be salvaged for the module thing
> you talked about, please provide a bit more context about it.

We talked offline, but for others. The idea here is when we have split
BTF of a kernel module, we'd like to be able to dump it just like we
do it for vmlinux BTF. But for kernel module we'd like to get
<module>.h which would include only types defined in kernel module,
skipping types that should be in base BTF (and thus come from
vmlinux.h).

The idea is that in practice you'd have something like:

#include <vmlinux.h>
#include <module1.h>
#include <module2.h>

and that will work together and won't conflict with vmlinux.h.

>
> Thanks,
> Eduard
diff mbox series

Patch

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.
+ */
+LIBBPF_API int btf_dump__dump_one_type(struct btf_dump *d, __u32 id, bool fwd);
+
+/* **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;
+};
+
+/* Adds type **id** and it's dependencies to the emit queue. */
+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);
+
 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);
+}
+
+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)
 {
 	const struct btf_type *t;
 	__u16 kind;
@@ -746,8 +762,7 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, bool fwd)
 		case BTF_KIND_STRUCT:
 		case BTF_KIND_UNION:
 			btf_dump_emit_struct_fwd(d, id, t);
-			btf_dump_printf(d, ";\n\n");
-			break;
+			return 1;
 		case BTF_KIND_TYPEDEF:
 			/*
 			 * for typedef fwd_emitted means typedef definition
@@ -755,29 +770,23 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, bool fwd)
 			 * references through pointer only, not for embedding
 			 */
 			btf_dump_emit_typedef_def(d, id, t, 0);
-			btf_dump_printf(d, ";\n\n");
-			break;
+			return 1;
 		default:
-			break;
+			return 0;
 		}
-
-		return;
 	}
 
 	switch (kind) {
 	case BTF_KIND_INT:
 		/* Emit type alias definitions if necessary */
-		btf_dump_emit_missing_aliases(d, id, false);
-		break;
+		return btf_dump_emit_missing_aliases(d, id, false);
 	case BTF_KIND_ENUM:
 	case BTF_KIND_ENUM64:
 		btf_dump_emit_enum_def(d, id, t, 0);
-		btf_dump_printf(d, ";\n\n");
-		break;
+		return 1;
 	case BTF_KIND_FWD:
 		btf_dump_emit_fwd_def(d, id, t);
-		btf_dump_printf(d, ";\n\n");
-		break;
+		return 1;
 	case BTF_KIND_TYPEDEF:
 		/*
 		 * typedef can server as both definition and forward
@@ -787,15 +796,13 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, bool fwd)
 		 * emit typedef as a forward declaration
 		 */
 		btf_dump_emit_typedef_def(d, id, t, 0);
-		btf_dump_printf(d, ";\n\n");
-		break;
+		return 1;
 	case BTF_KIND_STRUCT:
 	case BTF_KIND_UNION:
 		btf_dump_emit_struct_def(d, id, t, 0);
-		btf_dump_printf(d, ";\n\n");
-		break;
+		return 1;
 	default:
-		break;
+		return 0;
 	}
 }
 
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;
 } LIBBPF_1.4.0;