Re: [PATCH] bpf: generate const static pointers for kernel helpers

[Yonghong Song <yonghong.song@linux.dev>]

On 1/27/24 10:50 AM, Jose E. Marchesi wrote:
> The generated bpf_helper_defs.h file currently contains definitions
> like this for the kernel helpers, which are static objects:
>
>    static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>
> These work well in both clang and GCC because both compilers do
> constant propagation with -O1 and higher optimization, resulting in
> `call 1' BPF instructions being generated, which are calls to kernel
> helpers.
>
> However, there is a discrepancy on how the -Wunused-variable
> warning (activated by -Wall) is handled in these compilers:
>
> - clang will not emit -Wunused-variable warnings for static variables
>    defined in C header files, be them constant or not constant.
>
> - GCC will not emit -Wunused-variable warnings for _constant_ static
>    variables defined in header files, but it will emit warnings for
>    non-constant static variables defined in header files.
>
> There is no reason for these bpf_helpers_def.h pointers to not be
> declared constant, and it is actually desirable to do so, since their
> values are not to be changed.  So this patch modifies bpf_doc.py to
> generate prototypes like:
>
>    static void *(* const bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>
> This allows GCC to not error while compiling BPF programs with `-Wall
> -Werror', while still being able to detect and error on legitimate
> unused variables in the program themselves.
>
> This change doesn't impact the desired constant propagation in neither
> Clang nor GCC with -O1 and higher.  On the contrary, being declared as
> constant may increase the odds they get constant folded when
> used/referred to in certain circumstances.
>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: alexei.starovoitov@gmail.com
> Cc: yonghong.song@linux.dev
> Cc: eddyz87@gmail.com
> Cc: cupertino.miranda@oracle.com
> Cc: david.faust@oracle.com

LGTM. Please add proper tag in the commit subject, e.g.,
"[PATCH bpf-next]" instead of "[PATCH]", so CI can pick
up the patch and do proper test.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   scripts/bpf_doc.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index 61b7dddedc46..2b94749c99cc 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -827,7 +827,7 @@ class PrinterHelpers(Printer):
>                   print(' *{}{}'.format(' \t' if line else '', line))
>   
>           print(' */')
> -        print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
> +        print('static %s %s(* const %s)(' % (self.map_type(proto['ret_type']),
>                                         proto['ret_star'], proto['name']), end='')
>           comma = ''
>           for i, a in enumerate(proto['args']):