Re: [PATCH bpf-next] compiler-gcc: Ignore -Wmissing-prototypes warning for older GCC

Yafang Shao
2023-11-05T13:51:07Z

On Sun, Nov 5, 2023 at 9:01 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Nov 5, 2023, at 12:54, Yafang Shao wrote:
> > On Sun, Nov 5, 2023 at 4:24 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Sun, Nov 5, 2023, at 07:22, Yafang Shao wrote:
> >> > To address this, we should also suppress the "-Wmissing-prototypes" warning
> >> > for older GCC versions. Since "#pragma GCC diagnostic push" is supported as
> >> > of GCC 4.6, it is acceptable to ignore these warnings for GCC >= 5.1.0.
> >>
> >> Not sure why these need to be suppressed like this at all,
> >> can't you just add the prototype somewhere?
> >
> > BPF kfuncs are intended for use within BPF programs, and they should
> > not be called from other parts of the kernel. Consequently, it is not
> > appropriate to include their prototypes in a kernel header file.
>
> How does the caller in the BPF program get the prototype?

BPF programs will get the prototype directly from BTF, for example,
see also the prototypes declared using "__ksym" in
tools/testing/selftests/bpf/progs/ for examples.

>
> >> > @@ -131,14 +131,14 @@
> >> > #define __diag_str(s) __diag_str1(s)
> >> > #define __diag(s) _Pragma(__diag_str(GCC diagnostic s))
> >> >
> >> > -#if GCC_VERSION >= 80000
> >> > -#define __diag_GCC_8(s) __diag(s)
> >> > +#if GCC_VERSION >= 50100
> >> > +#define __diag_GCC_5(s) __diag(s)
> >> > #else
> >> > -#define __diag_GCC_8(s)
> >> > +#define __diag_GCC_5(s)
> >> > #endif
> >> >
> >>
> >> This breaks all uses of __diag_ignore that specify
> >> version 8 directly. Just add the macros for each version
> >> from 5 to 14 here.
> >
> > It seems that __diag_GCC_8() or __diag_GCC() are not directly used
> > anywhere in the kernel, right?
>
> I see three instances:
>
> drivers/net/ethernet/renesas/sh_eth.c:__diag_ignore(GCC, 8, "-Woverride-init",
> include/linux/compat.h: __diag_ignore(GCC, 8, "-Wattribute-alias", include/linux/syscalls.h: __diag_ignore(GCC, 8, "-Wattribute-alias",

Thanks for pointing them out.

>
> The override-init one should probably use version 5 as well,
> but I think the -Wattribute-alias ones require GCC 8 and otherwise
> cause a warning about an unknown warning option.

Right. -Wattribute-alias requires GCC 8.

>
> __diag_ignore_all() would also be wrong for the override-init
> because the option has a different name in clang
> (-Winitializer-overrides).
>
> > Therefore it won't break anything if we just replace __diag_GCC_8()
> > with __diag_GCC_5().
> > It may be cumbersome to add the macrocs for every GCC version if they
> > aren't actively used.
>
> For the _all variant, I would prefer to completely remove
> the version logic and just use __diag() directly. I think the
> entire point of this is that it is used on all supported
> versions.

Good suggestion.
will do it.

--
Regards
Yafang