[PATCH bpf-next] bpf: disable some `attribute ignored' warnings in GCC

[PATCH bpf-next] bpf: disable some `attribute ignored' warnings in GCC

[Jose E. Marchesi]

This patch modifies selftests/bpf/Makefile to pass -Wno-attributes to
GCC.  This is because of the following attributes which are ignored:

- btf_decl_tag
- btf_type_tag

  There are many of these.  At the moment none of these are
  recognized/handled by gcc-bpf.

  We are aware that btf_decl_tag is necessary for some of the
  selftest harness to communicate test failure/success.  Support for
  it is in progress in GCC upstream:

  https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650482.html

  However, the GCC master branch is not yet open, so the series
  above (currently under review upstream) wont be able to make it
  there until 14.1 gets released, probably mid next week.

  As for btf_type_tag, more extensive work will be needed in GCC
  upstream to support it in both BTF and DWARF.  We have a WIP big
  patch for that, but that is not needed to compile/build the
  selftests.

- used

  There are SEC macros defined in the selftests as:

  #define SEC(N) __attribute__((section(N),used))

  The SEC macro is used for both functions and global variables.
  According to the GCC documentation `used' attribute is really only
  meaningful for functions, and it warns when the attribute is used
  for other global objects, like for example ctl_array in
  test_xdp_noinline.c.

  Ignoring this is bening.

- visibility

  In progs/cpumask_common.h:13 there is:

    #define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
    private(MASK) static struct bpf_cpumask __kptr * global_mask;

  The __hidden macro defines to:

  tools/lib/bpf/bpf_helpers.h:#define __hidden __attribute__((visibility("hidden")))

  GCC emits an "attribute ignored" warning because static implies
  hidden visibility.

  Ignoring this warning is benign.  An alternative would be to make
  global_mask as non-static.

- align_value

  In progs/test_cls_redirect.c:127 there is:

  typedef uint8_t *net_ptr __attribute__((align_value(8)));

  GCC warns that it is ignoring this attribute, because it is not
  implemented by GCC.

  I think ignoring this attribute in GCC is bening, because according
  to the clang documentation [1] its purpose seems to be merely
  declarative and doesn't seem to translate into extra checks at
  run-time, only to pehaps better optimized code ("runtime behavior is
  undefined if the pointed memory object is not aligned to the
  specified alignment").

  [1] https://clang.llvm.org/docs/AttributeReference.html#align-value

Tested in bpf-next master.

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: david.faust@oracle.com
Cc: cupertino.miranda@oracle.com
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ba28d42b74db..5d9c906bc3cb 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -431,7 +431,7 @@ endef
 # Build BPF object using GCC
 define GCC_BPF_BUILD_RULE
 	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(BPF_GCC) $3 -O2 -c $1 -o $2
+	$(Q)$(BPF_GCC) $3 -Wno-attributes -O2 -c $1 -o $2
 endef
 
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
-- 
2.30.2

Re: [PATCH bpf-next] bpf: disable some `attribute ignored' warnings in GCC

[Yonghong Song]

On 5/3/24 5:32 AM, Jose E. Marchesi wrote:
> This patch modifies selftests/bpf/Makefile to pass -Wno-attributes to
> GCC.  This is because of the following attributes which are ignored:
>
> - btf_decl_tag
> - btf_type_tag
>
>    There are many of these.  At the moment none of these are
>    recognized/handled by gcc-bpf.
>
>    We are aware that btf_decl_tag is necessary for some of the
>    selftest harness to communicate test failure/success.  Support for
>    it is in progress in GCC upstream:
>
>    https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650482.html
>
>    However, the GCC master branch is not yet open, so the series
>    above (currently under review upstream) wont be able to make it
>    there until 14.1 gets released, probably mid next week.

Thanks. It would be great if the patch can be merged soon.

>
>    As for btf_type_tag, more extensive work will be needed in GCC
>    upstream to support it in both BTF and DWARF.  We have a WIP big
>    patch for that, but that is not needed to compile/build the
>    selftests.

Thanks. Eduard has implemented in llvm with agreed new format. Since
the old phabricator becomes readonly, he will upstream the original
patch to llvm-project soon.

>
> - used
>
>    There are SEC macros defined in the selftests as:
>
>    #define SEC(N) __attribute__((section(N),used))
>
>    The SEC macro is used for both functions and global variables.
>    According to the GCC documentation `used' attribute is really only
>    meaningful for functions, and it warns when the attribute is used
>    for other global objects, like for example ctl_array in
>    test_xdp_noinline.c.
>
>    Ignoring this is bening.

Bening -> Benign?

>
> - visibility
>
>    In progs/cpumask_common.h:13 there is:
>
>      #define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
>      private(MASK) static struct bpf_cpumask __kptr * global_mask;
>
>    The __hidden macro defines to:
>
>    tools/lib/bpf/bpf_helpers.h:#define __hidden __attribute__((visibility("hidden")))
>
>    GCC emits an "attribute ignored" warning because static implies
>    hidden visibility.
>
>    Ignoring this warning is benign.  An alternative would be to make
>    global_mask as non-static.

In the above, let us just remove __hidden from the '#define'.
As you mentioned, the 'global_mask' is already a static variable,
adding '__hidden' is not really needed at all.

>
> - align_value
>
>    In progs/test_cls_redirect.c:127 there is:
>
>    typedef uint8_t *net_ptr __attribute__((align_value(8)));
>
>    GCC warns that it is ignoring this attribute, because it is not
>    implemented by GCC.
>
>    I think ignoring this attribute in GCC is bening, because according

Bening -> Benign?

>    to the clang documentation [1] its purpose seems to be merely
>    declarative and doesn't seem to translate into extra checks at
>    run-time, only to pehaps better optimized code ("runtime behavior is
>    undefined if the pointed memory object is not aligned to the
>    specified alignment").

Yes, the attribute does not really enforce at runtime. It merely
give a declarative requirement.

>
>    [1] https://clang.llvm.org/docs/AttributeReference.html#align-value
>
> Tested in bpf-next master.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/testing/selftests/bpf/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index ba28d42b74db..5d9c906bc3cb 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -431,7 +431,7 @@ endef
>   # Build BPF object using GCC
>   define GCC_BPF_BUILD_RULE
>   	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
> -	$(Q)$(BPF_GCC) $3 -O2 -c $1 -o $2
> +	$(Q)$(BPF_GCC) $3 -Wno-attributes -O2 -c $1 -o $2

LGTM with above a few nits.
Acked-by: Yonghong Song <yonghong.song@linux.dev>

>   endef
>   
>   SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c

Re: [PATCH bpf-next] bpf: disable some `attribute ignored' warnings in GCC

[David Faust]

On 5/6/24 11:32 AM, Yonghong Song wrote:
> 
> On 5/3/24 5:32 AM, Jose E. Marchesi wrote:
>> This patch modifies selftests/bpf/Makefile to pass -Wno-attributes to
>> GCC.  This is because of the following attributes which are ignored:
>>
>> - btf_decl_tag
>> - btf_type_tag
>>
>>    There are many of these.  At the moment none of these are
>>    recognized/handled by gcc-bpf.
>>
>>    We are aware that btf_decl_tag is necessary for some of the
>>    selftest harness to communicate test failure/success.  Support for
>>    it is in progress in GCC upstream:
>>
>>    https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650482.html
>>
>>    However, the GCC master branch is not yet open, so the series
>>    above (currently under review upstream) wont be able to make it
>>    there until 14.1 gets released, probably mid next week.
> 
> Thanks. It would be great if the patch can be merged soon.

A small note here - the above series does not itself contain the patch
to support decl_tag, it is just some prerequisite structural changes and
the option to prune BTF before emission similar to clang to slim the
selftest (and other) program sizes down.

The patch to enable decl_tag for functions in BTF, enough for the
selftest harness, can go up after that. But, it will require some
approvals from the C front-end maintainers, since it is a new attribute,
so it may take longer, and may be contentious.

> 
>>
>>    As for btf_type_tag, more extensive work will be needed in GCC
>>    upstream to support it in both BTF and DWARF.  We have a WIP big
>>    patch for that, but that is not needed to compile/build the
>>    selftests.
> 
> Thanks. Eduard has implemented in llvm with agreed new format. Since
> the old phabricator becomes readonly, he will upstream the original
> patch to llvm-project soon.
> 
>>
>> - used
>>
>>    There are SEC macros defined in the selftests as:
>>
>>    #define SEC(N) __attribute__((section(N),used))
>>
>>    The SEC macro is used for both functions and global variables.
>>    According to the GCC documentation `used' attribute is really only
>>    meaningful for functions, and it warns when the attribute is used
>>    for other global objects, like for example ctl_array in
>>    test_xdp_noinline.c.
>>
>>    Ignoring this is bening.
> 
> Bening -> Benign?
> 
>>
>> - visibility
>>
>>    In progs/cpumask_common.h:13 there is:
>>
>>      #define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
>>      private(MASK) static struct bpf_cpumask __kptr * global_mask;
>>
>>    The __hidden macro defines to:
>>
>>    tools/lib/bpf/bpf_helpers.h:#define __hidden __attribute__((visibility("hidden")))
>>
>>    GCC emits an "attribute ignored" warning because static implies
>>    hidden visibility.
>>
>>    Ignoring this warning is benign.  An alternative would be to make
>>    global_mask as non-static.
> 
> In the above, let us just remove __hidden from the '#define'.
> As you mentioned, the 'global_mask' is already a static variable,
> adding '__hidden' is not really needed at all.
> 
>>
>> - align_value
>>
>>    In progs/test_cls_redirect.c:127 there is:
>>
>>    typedef uint8_t *net_ptr __attribute__((align_value(8)));
>>
>>    GCC warns that it is ignoring this attribute, because it is not
>>    implemented by GCC.
>>
>>    I think ignoring this attribute in GCC is bening, because according
> 
> Bening -> Benign?
> 
>>    to the clang documentation [1] its purpose seems to be merely
>>    declarative and doesn't seem to translate into extra checks at
>>    run-time, only to pehaps better optimized code ("runtime behavior is
>>    undefined if the pointed memory object is not aligned to the
>>    specified alignment").
> 
> Yes, the attribute does not really enforce at runtime. It merely
> give a declarative requirement.
> 
>>
>>    [1] https://clang.llvm.org/docs/AttributeReference.html#align-value
>>
>> Tested in bpf-next master.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> ---
>>   tools/testing/selftests/bpf/Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index ba28d42b74db..5d9c906bc3cb 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -431,7 +431,7 @@ endef
>>   # Build BPF object using GCC
>>   define GCC_BPF_BUILD_RULE
>>   	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
>> -	$(Q)$(BPF_GCC) $3 -O2 -c $1 -o $2
>> +	$(Q)$(BPF_GCC) $3 -Wno-attributes -O2 -c $1 -o $2
> 
> LGTM with above a few nits.
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> 
>>   endef
>>   
>>   SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c

Re: [PATCH bpf-next] bpf: disable some `attribute ignored' warnings in GCC

[Yonghong Song]

On 5/6/24 12:09 PM, David Faust wrote:
>
> On 5/6/24 11:32 AM, Yonghong Song wrote:
>> On 5/3/24 5:32 AM, Jose E. Marchesi wrote:
>>> This patch modifies selftests/bpf/Makefile to pass -Wno-attributes to
>>> GCC.  This is because of the following attributes which are ignored:
>>>
>>> - btf_decl_tag
>>> - btf_type_tag
>>>
>>>     There are many of these.  At the moment none of these are
>>>     recognized/handled by gcc-bpf.
>>>
>>>     We are aware that btf_decl_tag is necessary for some of the
>>>     selftest harness to communicate test failure/success.  Support for
>>>     it is in progress in GCC upstream:
>>>
>>>     https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650482.html
>>>
>>>     However, the GCC master branch is not yet open, so the series
>>>     above (currently under review upstream) wont be able to make it
>>>     there until 14.1 gets released, probably mid next week.
>> Thanks. It would be great if the patch can be merged soon.
> A small note here - the above series does not itself contain the patch
> to support decl_tag, it is just some prerequisite structural changes and
> the option to prune BTF before emission similar to clang to slim the
> selftest (and other) program sizes down.
>
> The patch to enable decl_tag for functions in BTF, enough for the
> selftest harness, can go up after that. But, it will require some
> approvals from the C front-end maintainers, since it is a new attribute,
> so it may take longer, and may be contentious.

Thanks for the update! Let me know if I can help from llvm/kernel/bpf
perspective.

>
>>>     As for btf_type_tag, more extensive work will be needed in GCC
>>>     upstream to support it in both BTF and DWARF.  We have a WIP big
>>>     patch for that, but that is not needed to compile/build the
>>>     selftests.
>> Thanks. Eduard has implemented in llvm with agreed new format. Since
>> the old phabricator becomes readonly, he will upstream the original
>> patch to llvm-project soon.
[...]