Message ID | 20200608152052.898491-1-jean-philippe@linaro.org |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf] libbpf: Fix BTF-to-C conversion of noreturn function pointers | expand |
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(-) > [...]
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
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
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);
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(-)