diff mbox series

[RFC] libbpf: Allow to emit all dependent definitions

Message ID 20191015130117.32292-1-jolsa@kernel.org
State RFC
Delegated to: BPF Maintainers
Headers show
Series [RFC] libbpf: Allow to emit all dependent definitions | expand

Commit Message

Jiri Olsa Oct. 15, 2019, 1:01 p.m. UTC 
Currently the bpf dumper does not emit definitions
of pointers to structs. It only emits forward type
declarations.

Having 2 structs like:

   struct B {
     int b;
   };

   struct A {
     struct B *ptr;
   };

the call to btf_dump__dump_type(id = struct A) dumps:

   struct B;
   struct A {
     struct B *ptr;
   };

It'd ease up bpftrace code if we could dump definitions
of all dependent types, like:

   struct B {
     int b;
   };
   struct A {
     struct B *ptr;
   };

So we could dereference all the pointers easily, instead
of searching for each access member's type and dumping it
separately.

Adding struct btf_dump_opts::emit_all to do that.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/btf_dump.c | 14 ++++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Andrii Nakryiko Oct. 15, 2019, 4:22 p.m. UTC | #1 
On Tue, Oct 15, 2019 at 6:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently the bpf dumper does not emit definitions
> of pointers to structs. It only emits forward type
> declarations.
>
> Having 2 structs like:
>
>    struct B {
>      int b;
>    };
>
>    struct A {
>      struct B *ptr;
>    };
>
> the call to btf_dump__dump_type(id = struct A) dumps:
>
>    struct B;
>    struct A {
>      struct B *ptr;
>    };
>
> It'd ease up bpftrace code if we could dump definitions
> of all dependent types, like:
>
>    struct B {
>      int b;
>    };
>    struct A {
>      struct B *ptr;
>    };
>
> So we could dereference all the pointers easily, instead
> of searching for each access member's type and dumping it
> separately.
>
> Adding struct btf_dump_opts::emit_all to do that.
>

Hey Jiri,

Yeah, Daniel Xu mentioned that this would be useful. I haven't thought
this through very well yet, but I suspect that this simple change
might not be enough to make this work. There are cases where you are
not yet allowed to emit definition and have to emit
forward-declaration first. I suggest trying to use this on vmlinux BTF
and see if resulting header files still compiles with both Clang and
GCC. Do you mind checking?

But also, as we learned over last few months, just adding extra field
to an opts struct is not backwards-compatible, so we'll need to add
new API and follow the pattern that we used for
bpf_object__open_{file,mem).

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

[...]
Jiri Olsa Oct. 15, 2019, 8:44 p.m. UTC | #2 
On Tue, Oct 15, 2019 at 09:22:35AM -0700, Andrii Nakryiko wrote:
> On Tue, Oct 15, 2019 at 6:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently the bpf dumper does not emit definitions
> > of pointers to structs. It only emits forward type
> > declarations.
> >
> > Having 2 structs like:
> >
> >    struct B {
> >      int b;
> >    };
> >
> >    struct A {
> >      struct B *ptr;
> >    };
> >
> > the call to btf_dump__dump_type(id = struct A) dumps:
> >
> >    struct B;
> >    struct A {
> >      struct B *ptr;
> >    };
> >
> > It'd ease up bpftrace code if we could dump definitions
> > of all dependent types, like:
> >
> >    struct B {
> >      int b;
> >    };
> >    struct A {
> >      struct B *ptr;
> >    };
> >
> > So we could dereference all the pointers easily, instead
> > of searching for each access member's type and dumping it
> > separately.
> >
> > Adding struct btf_dump_opts::emit_all to do that.
> >
> 
> Hey Jiri,
> 
> Yeah, Daniel Xu mentioned that this would be useful. I haven't thought
> this through very well yet, but I suspect that this simple change
> might not be enough to make this work. There are cases where you are
> not yet allowed to emit definition and have to emit
> forward-declaration first. I suggest trying to use this on vmlinux BTF
> and see if resulting header files still compiles with both Clang and
> GCC. Do you mind checking?

agh right, my test fails for vmlinux BTF

> 
> But also, as we learned over last few months, just adding extra field
> to an opts struct is not backwards-compatible, so we'll need to add
> new API and follow the pattern that we used for
> bpf_object__open_{file,mem).

will check, thanks
jirka
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 9cb44b4fbf60..2c0d3158efd4 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -113,6 +113,7 @@  struct btf_dump;
 
 struct btf_dump_opts {
 	void *ctx;
+	bool emit_all;
 };
 
 typedef void (*btf_dump_printf_fn_t)(void *ctx, const char *fmt, va_list args);
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 139812b46c7b..bab08f6428e7 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -132,6 +132,7 @@  struct btf_dump *btf_dump__new(const struct btf *btf,
 	d->btf_ext = btf_ext;
 	d->printf_fn = printf_fn;
 	d->opts.ctx = opts ? opts->ctx : NULL;
+	d->opts.emit_all = opts ? opts->emit_all : false;
 
 	d->type_names = hashmap__new(str_hash_fn, str_equal_fn, NULL);
 	if (IS_ERR(d->type_names)) {
@@ -612,7 +613,12 @@  static bool btf_dump_is_blacklisted(struct btf_dump *d, __u32 id)
 static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
 {
 	struct btf_dump_type_aux_state *tstate = &d->type_states[id];
-	bool top_level_def = cont_id == 0;
+	/*
+	 * Emit difinitions (instead of forward declarations) in case
+	 * we are top level id, or we are asked to emit all definitions
+	 * via options.
+	 */
+	bool emit_def = cont_id == 0 || d->opts.emit_all;
 	const struct btf_type *t;
 	__u16 kind;
 
@@ -668,7 +674,7 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
 		tstate->emit_state = EMITTED;
 		break;
 	case BTF_KIND_ENUM:
-		if (top_level_def) {
+		if (emit_def) {
 			btf_dump_emit_enum_def(d, id, t, 0);
 			btf_dump_printf(d, ";\n\n");
 		}
@@ -714,7 +720,7 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
 		 * members have necessary forward-declarations, where
 		 * applicable
 		 */
-		if (top_level_def || t->name_off == 0) {
+		if (emit_def || t->name_off == 0) {
 			const struct btf_member *m = btf_members(t);
 			__u16 vlen = btf_vlen(t);
 			int i, new_cont_id;
@@ -728,7 +734,7 @@  static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
 			tstate->fwd_emitted = 1;
 		}
 
-		if (top_level_def) {
+		if (emit_def) {
 			btf_dump_emit_struct_def(d, id, t, 0);
 			btf_dump_printf(d, ";\n\n");
 			tstate->emit_state = EMITTED;