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@xxxxxxxxxx> >> Cc: david.faust@xxxxxxxxxx >> Cc: cupertino.miranda@xxxxxxxxxx >> Cc: Yonghong Song <yonghong.song@xxxxxxxxx> >> Cc: Eduard Zingerman <eddyz87@xxxxxxxxx> >> --- >> 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@xxxxxxxxx> > >> endef >> >> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
