[PATCH bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers

[PATCH bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers

[Jean-Philippe Brucker]

When trying to convert the BTF for a function pointer marked "noreturn"
to C code, bpftool currently generates a syntax error. This happens with
the exit() pointer in drivers/firmware/efi/libstub/efistub.h, in an
arm64 vmlinux. When dealing with this declaration:

	efi_status_t __noreturn (__efiapi *exit)(...);

bpftool produces the following output:

	efi_status_tvolatile  (*exit)(...);

Fix the error by inserting the space before the function modifier.

Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 tools/lib/bpf/btf_dump.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 0c28ee82834b7..003f305fc2342 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1111,9 +1111,11 @@ static void btf_dump_emit_type_decl(struct btf_dump *d, __u32 id,
 	d->decl_stack_cnt = stack_start;
 }
 
-static void btf_dump_emit_mods(struct btf_dump *d, struct id_stack *decl_stack)
+static void btf__dump_emit_mods(struct btf_dump *d, struct id_stack *decl_stack,
+				const char *prefix, const char *suffix)
 {
 	const struct btf_type *t;
+	const char *mod;
 	__u32 id;
 
 	while (decl_stack->cnt) {
@@ -1122,21 +1124,27 @@ static void btf_dump_emit_mods(struct btf_dump *d, struct id_stack *decl_stack)
 
 		switch (btf_kind(t)) {
 		case BTF_KIND_VOLATILE:
-			btf_dump_printf(d, "volatile ");
+			mod = "volatile";
 			break;
 		case BTF_KIND_CONST:
-			btf_dump_printf(d, "const ");
+			mod = "const";
 			break;
 		case BTF_KIND_RESTRICT:
-			btf_dump_printf(d, "restrict ");
+			mod = "restrict";
 			break;
 		default:
 			return;
 		}
+		btf_dump_printf(d, "%s%s%s", prefix, mod, suffix);
 		decl_stack->cnt--;
 	}
 }
 
+static void btf_dump_emit_mods(struct btf_dump *d, struct id_stack *decl_stack)
+{
+	btf__dump_emit_mods(d, decl_stack, "", " ");
+}
+
 static void btf_dump_emit_name(const struct btf_dump *d,
 			       const char *name, bool last_was_ptr)
 {
@@ -1270,7 +1278,7 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
 			__u16 vlen = btf_vlen(t);
 			int i;
 
-			btf_dump_emit_mods(d, decls);
+			btf__dump_emit_mods(d, decls, " ", "");
 			if (decls->cnt) {
 				btf_dump_printf(d, " (");
 				btf_dump_emit_type_chain(d, decls, fname, lvl);
-- 
2.27.0

Re: [PATCH bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers

[Andrii Nakryiko]

On Mon, Jun 8, 2020 at 8:23 AM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> When trying to convert the BTF for a function pointer marked "noreturn"
> to C code, bpftool currently generates a syntax error. This happens with
> the exit() pointer in drivers/firmware/efi/libstub/efistub.h, in an
> arm64 vmlinux. When dealing with this declaration:
>
>         efi_status_t __noreturn (__efiapi *exit)(...);
>
> bpftool produces the following output:
>
>         efi_status_tvolatile  (*exit)(...);


I'm curious where this volatile is coming from, I don't see it in
__efiapi. But even if it's there, shouldn't it be inside parens
instead:

efi_status_t (volatile *exit)(...);

?

>
> Fix the error by inserting the space before the function modifier.
>
> Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---

Can you please add tests for this case into selftests (probably
progs/btf_dump_test_case_syntax.c?) So it's clear what's the input and
what's the expected output.

>  tools/lib/bpf/btf_dump.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>

[...]

Re: [PATCH bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers

[Jean-Philippe Brucker]

On Mon, Jun 08, 2020 at 04:50:37PM -0700, Andrii Nakryiko wrote:
> On Mon, Jun 8, 2020 at 8:23 AM Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > When trying to convert the BTF for a function pointer marked "noreturn"
> > to C code, bpftool currently generates a syntax error. This happens with
> > the exit() pointer in drivers/firmware/efi/libstub/efistub.h, in an
> > arm64 vmlinux. When dealing with this declaration:
> >
> >         efi_status_t __noreturn (__efiapi *exit)(...);
> >
> > bpftool produces the following output:
> >
> >         efi_status_tvolatile  (*exit)(...);
> 
> 
> I'm curious where this volatile is coming from, I don't see it in
> __efiapi. But even if it's there, shouldn't it be inside parens
> instead:
> 
> efi_status_t (volatile *exit)(...);

It's the __noreturn attribute that becomes "volatile", not the __efiapi.
My reproducer is:

  struct my_struct {
          void __attribute__((noreturn)) (*fn)(int);
  };
  struct my_struct a;

When generating DWARF info for this, GCC inserts a DW_TAG_volatile_type.
Clang doesn't add a volatile tag, it just omits the noreturn qualifier.
From what I could find, it's due to legacy "noreturn" support in GCC [1]:
before version 2.5 the only way to declare a noreturn function was to
declare it volatile.

[1] https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html

Given that not all compilers turn "noreturn" into "volatile", and that I
haven't managed to insert any other modifier (volatile/const/restrict) in
this location (the efistub example above is the only issue on an
allyesconfig kernel), I was considering simply removing this call to
btf_dump_emit_mods(). But I'm not confident enough that it won't ever be
necessary.

> > Fix the error by inserting the space before the function modifier.
> >
> > Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> 
> Can you please add tests for this case into selftests (probably
> progs/btf_dump_test_case_syntax.c?) So it's clear what's the input and
> what's the expected output.

Those tests are built with clang, which doesn't emit the "volatile"
modifier. Should I add a separate test for GCC?

Thanks,
Jean

Re: [PATCH bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers

[Andrii Nakryiko]

On Tue, Jun 9, 2020 at 7:05 AM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Mon, Jun 08, 2020 at 04:50:37PM -0700, Andrii Nakryiko wrote:
> > On Mon, Jun 8, 2020 at 8:23 AM Jean-Philippe Brucker
> > <jean-philippe@linaro.org> wrote:
> > >
> > > When trying to convert the BTF for a function pointer marked "noreturn"
> > > to C code, bpftool currently generates a syntax error. This happens with
> > > the exit() pointer in drivers/firmware/efi/libstub/efistub.h, in an
> > > arm64 vmlinux. When dealing with this declaration:
> > >
> > >         efi_status_t __noreturn (__efiapi *exit)(...);
> > >
> > > bpftool produces the following output:
> > >
> > >         efi_status_tvolatile  (*exit)(...);
> >
> >
> > I'm curious where this volatile is coming from, I don't see it in
> > __efiapi. But even if it's there, shouldn't it be inside parens
> > instead:
> >
> > efi_status_t (volatile *exit)(...);
>
> It's the __noreturn attribute that becomes "volatile", not the __efiapi.
> My reproducer is:
>
>   struct my_struct {
>           void __attribute__((noreturn)) (*fn)(int);
>   };
>   struct my_struct a;
>
> When generating DWARF info for this, GCC inserts a DW_TAG_volatile_type.
> Clang doesn't add a volatile tag, it just omits the noreturn qualifier.
> From what I could find, it's due to legacy "noreturn" support in GCC [1]:
> before version 2.5 the only way to declare a noreturn function was to
> declare it volatile.

Ok, thanks a lot for extra context, it made it easier to understand
and reproduce. For some reason, I can't repro it even for allyesconfig
build, but your simple repro shows this pretty clearly.

>
> [1] https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html
>
> Given that not all compilers turn "noreturn" into "volatile", and that I
> haven't managed to insert any other modifier (volatile/const/restrict) in
> this location (the efistub example above is the only issue on an
> allyesconfig kernel), I was considering simply removing this call to
> btf_dump_emit_mods(). But I'm not confident enough that it won't ever be
> necessary.

Just removing btf_dump_emit_mods() won't be correct. But there is a
similar GCC-specific work-around for array modifiers. I've implemented
the same for func_proto ([0]), tested with my local allyesconfig.
There are no differences in generated vmlinux.h, so no regressions on
my side. Your repro also is fixed, though. I just sent a patch with
the alternative fix. Could you please test it with your setup and send
your Tested-by: to confirm it fixes your issue?

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200610052335.2862559-1-andriin@fb.com/

>
> > > Fix the error by inserting the space before the function modifier.
> > >
> > > Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > ---
> >
> > Can you please add tests for this case into selftests (probably
> > progs/btf_dump_test_case_syntax.c?) So it's clear what's the input and
> > what's the expected output.
>
> Those tests are built with clang, which doesn't emit the "volatile"
> modifier. Should I add a separate test for GCC?

Nah, I don't think it's worth it to try to introduce GCC-compiled
tests just for this.


>
> Thanks,
> Jean