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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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