diff mbox series

[bpf] selftests/bpf: Filter out _GNU_SOURCE when compiling test_cpp

Message ID 20240725214029.1760809-1-sdf@fomichev.me (mailing list archive)
State Accepted
Commit 41c24102af7b6236277a214428b203d51a3462df
Delegated to: BPF
Headers show
Series [bpf] selftests/bpf: Filter out _GNU_SOURCE when compiling test_cpp | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 273 this patch: 273
netdev/build_tools success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers warning 10 maintainers not CCed: shuah@kernel.org nathan@kernel.org llvm@lists.linux.dev justinstitt@google.com linux-kselftest@vger.kernel.org yonghong.song@linux.dev mykolal@fb.com morbo@google.com eddyz87@gmail.com ndesaulniers@google.com
netdev/build_clang success Errors and warnings before: 282 this patch: 282
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 287 this patch: 287
netdev/checkpatch warning WARNING: Unknown commit id 'cc937dad85ae', maybe rebased or not pulled? WARNING: Unknown link reference '0:', use 'Link:' or 'Closes:' instead
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-3 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-1 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-6 success Logs for x86_64-gcc / build-release

Commit Message

Stanislav Fomichev July 25, 2024, 9:40 p.m. UTC 
Jakub reports build failures when merging linux/master with net tree:

CXX      test_cpp
In file included from <built-in>:454:
<command line>:2:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
    2 | #define _GNU_SOURCE
      |         ^
<built-in>:445:9: note: previous definition is here
  445 | #define _GNU_SOURCE 1

The culprit is commit cc937dad85ae ("selftests: centralize -D_GNU_SOURCE= to
CFLAGS in lib.mk") which unconditionally added -D_GNU_SOUCE to CLFAGS.
Apparently clang++ also unconditionally adds it for the C++ targets [0]
which causes a conflict. Add small change in the selftests makefile
to filter it out for test_cpp.

Not sure which tree it should go via, targeting bpf for now, but net
might be better?

0: https://stackoverflow.com/questions/11670581/why-is-gnu-source-defined-by-default-and-how-to-turn-it-off

Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski July 25, 2024, 10:13 p.m. UTC | #1 
On Thu, 25 Jul 2024 14:40:29 -0700 Stanislav Fomichev wrote:
> Not sure which tree it should go via, targeting bpf for now, but net
> might be better?

Thanks! I can add this patch to our "local hacks" queue to avoid
netdev-triggred BPF CI runs failing, but if it's okay for us 
to apply directly that'd be cleaner.

FWIW the netdev trees have been forwarded now.
Jiri Olsa July 26, 2024, 5:15 p.m. UTC | #2 
On Thu, Jul 25, 2024 at 02:40:29PM -0700, Stanislav Fomichev wrote:
> Jakub reports build failures when merging linux/master with net tree:
> 
> CXX      test_cpp
> In file included from <built-in>:454:
> <command line>:2:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
>     2 | #define _GNU_SOURCE
>       |         ^
> <built-in>:445:9: note: previous definition is here
>   445 | #define _GNU_SOURCE 1
> 
> The culprit is commit cc937dad85ae ("selftests: centralize -D_GNU_SOURCE= to
> CFLAGS in lib.mk") which unconditionally added -D_GNU_SOUCE to CLFAGS.
> Apparently clang++ also unconditionally adds it for the C++ targets [0]
> which causes a conflict. Add small change in the selftests makefile
> to filter it out for test_cpp.
> 
> Not sure which tree it should go via, targeting bpf for now, but net
> might be better?
> 
> 0: https://stackoverflow.com/questions/11670581/why-is-gnu-source-defined-by-default-and-how-to-turn-it-off
> 
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  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 dd49c1d23a60..81d4757ecd4c 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -713,7 +713,7 @@ $(OUTPUT)/xdp_features: xdp_features.c $(OUTPUT)/network_helpers.o $(OUTPUT)/xdp
>  # Make sure we are able to include and link libbpf against c++.
>  $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
>  	$(call msg,CXX,,$@)
> -	$(Q)$(CXX) $(CFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
> +	$(Q)$(CXX) $(subst -D_GNU_SOURCE=,,$(CFLAGS)) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@

nit, seems like we use filter-out for cases like that (but just one instance of -static option)
anyway the fix works for me

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

>  
>  # Benchmark runner
>  $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h $(BPFOBJ)
> -- 
> 2.45.2
>
Andrii Nakryiko July 27, 2024, 12:45 a.m. UTC | #3 
On Thu, Jul 25, 2024 at 2:40 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Jakub reports build failures when merging linux/master with net tree:
>
> CXX      test_cpp
> In file included from <built-in>:454:
> <command line>:2:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
>     2 | #define _GNU_SOURCE
>       |         ^
> <built-in>:445:9: note: previous definition is here
>   445 | #define _GNU_SOURCE 1
>
> The culprit is commit cc937dad85ae ("selftests: centralize -D_GNU_SOURCE= to
> CFLAGS in lib.mk") which unconditionally added -D_GNU_SOUCE to CLFAGS.
> Apparently clang++ also unconditionally adds it for the C++ targets [0]
> which causes a conflict. Add small change in the selftests makefile
> to filter it out for test_cpp.
>
> Not sure which tree it should go via, targeting bpf for now, but net
> might be better?
>
> 0: https://stackoverflow.com/questions/11670581/why-is-gnu-source-defined-by-default-and-how-to-turn-it-off
>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  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 dd49c1d23a60..81d4757ecd4c 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -713,7 +713,7 @@ $(OUTPUT)/xdp_features: xdp_features.c $(OUTPUT)/network_helpers.o $(OUTPUT)/xdp
>  # Make sure we are able to include and link libbpf against c++.
>  $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
>         $(call msg,CXX,,$@)
> -       $(Q)$(CXX) $(CFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
> +       $(Q)$(CXX) $(subst -D_GNU_SOURCE=,,$(CFLAGS)) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@

or we could

#ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif

(though we have 61 places with that...) so as to not have to update
every target in Makefile.


>
>  # Benchmark runner
>  $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h $(BPFOBJ)
> --
> 2.45.2
>
Jakub Kicinski July 27, 2024, 1:10 a.m. UTC | #4 
On Fri, 26 Jul 2024 17:45:06 -0700 Andrii Nakryiko wrote:
> or we could
> 
> #ifndef _GNU_SOURCE
> #define _GNU_SOURCE
> #endif
> 
> (though we have 61 places with that...) so as to not have to update
> every target in Makefile.

AFAIU we have -D_GNU_SOURCE= twice _in the command line args_ :(
One is from the Makefile which now always adds it to CFLAGS,
the other is "built-in" in g++ for some weird reason.

FWIW I have added this patch to the netdev "hack queue" so no
preference any more where the patch lands :)
Stanislav Fomichev July 27, 2024, 3:32 a.m. UTC | #5 
On 07/26, Jakub Kicinski wrote:
> On Fri, 26 Jul 2024 17:45:06 -0700 Andrii Nakryiko wrote:
> > or we could
> > 
> > #ifndef _GNU_SOURCE
> > #define _GNU_SOURCE
> > #endif
> > 
> > (though we have 61 places with that...) so as to not have to update
> > every target in Makefile.
> 
> AFAIU we have -D_GNU_SOURCE= twice _in the command line args_ :(
> One is from the Makefile which now always adds it to CFLAGS,
> the other is "built-in" in g++ for some weird reason.
> 
> FWIW I have added this patch to the netdev "hack queue" so no
> preference any more where the patch lands :)

Yeah, it can't be fixed with an ifdef because the conflict happens a bit
earlier:

$ echo "int main(int argc, char *argv[]){return 0;}" > test.cpp
$ clang++ -Wall -Werror -D_GNU_SOURCE= test.cpp
In file included from <built-in>:454:
<command line>:1:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
    1 | #define _GNU_SOURCE
      |         ^
<built-in>:445:9: note: previous definition is here
  445 | #define _GNU_SOURCE 1
      |         ^
1 error generated.
Yonghong Song July 28, 2024, 4:27 a.m. UTC | #6 
On 7/26/24 8:32 PM, Stanislav Fomichev wrote:
> On 07/26, Jakub Kicinski wrote:
>> On Fri, 26 Jul 2024 17:45:06 -0700 Andrii Nakryiko wrote:
>>> or we could
>>>
>>> #ifndef _GNU_SOURCE
>>> #define _GNU_SOURCE
>>> #endif
>>>
>>> (though we have 61 places with that...) so as to not have to update
>>> every target in Makefile.
>> AFAIU we have -D_GNU_SOURCE= twice _in the command line args_ :(
>> One is from the Makefile which now always adds it to CFLAGS,
>> the other is "built-in" in g++ for some weird reason.
>>
>> FWIW I have added this patch to the netdev "hack queue" so no
>> preference any more where the patch lands :)
> Yeah, it can't be fixed with an ifdef because the conflict happens a bit
> earlier:
>
> $ echo "int main(int argc, char *argv[]){return 0;}" > test.cpp
> $ clang++ -Wall -Werror -D_GNU_SOURCE= test.cpp
> In file included from <built-in>:454:
> <command line>:1:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
>      1 | #define _GNU_SOURCE
>        |         ^
> <built-in>:445:9: note: previous definition is here
>    445 | #define _GNU_SOURCE 1
>        |         ^

The above _GNU_SOURCE definition is defined by clang itself in the very beginning
of compilation.
See https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/OSTargets.h

// Linux target
template <typename Target>
class LLVM_LIBRARY_VISIBILITY LinuxTargetInfo : public OSTargetInfo<Target> {
protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const override {
     // Linux defines; list based off of gcc output
     DefineStd(Builder, "unix", Opts);
     DefineStd(Builder, "linux", Opts);
     if (Triple.isAndroid()) {
       Builder.defineMacro("__ANDROID__", "1");
       this->PlatformName = "android";
       this->PlatformMinVersion = Triple.getEnvironmentVersion();
       const unsigned Maj = this->PlatformMinVersion.getMajor();
       if (Maj) {
         Builder.defineMacro("__ANDROID_MIN_SDK_VERSION__", Twine(Maj));
         // This historical but ambiguous name for the minSdkVersion macro. Keep
         // defined for compatibility.
         Builder.defineMacro("__ANDROID_API__", "__ANDROID_MIN_SDK_VERSION__");
       }
     } else {
         Builder.defineMacro("__gnu_linux__");
     }
     if (Opts.POSIXThreads)
       Builder.defineMacro("_REENTRANT");
     if (Opts.CPlusPlus)
       Builder.defineMacro("_GNU_SOURCE");
     if (this->HasFloat128)
       Builder.defineMacro("__FLOAT128__");
   }
...

This caused a conflict with -D_GNU_SOURCE= and hence compilation failure.

> 1 error generated.
patchwork-bot+netdevbpf@kernel.org July 29, 2024, 9:05 p.m. UTC | #7 
Hello:

This patch was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu, 25 Jul 2024 14:40:29 -0700 you wrote:
> Jakub reports build failures when merging linux/master with net tree:
> 
> CXX      test_cpp
> In file included from <built-in>:454:
> <command line>:2:9: error: '_GNU_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
>     2 | #define _GNU_SOURCE
>       |         ^
> <built-in>:445:9: note: previous definition is here
>   445 | #define _GNU_SOURCE 1
> 
> [...]

Here is the summary with links:
  - [bpf] selftests/bpf: Filter out _GNU_SOURCE when compiling test_cpp
    https://git.kernel.org/bpf/bpf/c/41c24102af7b

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index dd49c1d23a60..81d4757ecd4c 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -713,7 +713,7 @@  $(OUTPUT)/xdp_features: xdp_features.c $(OUTPUT)/network_helpers.o $(OUTPUT)/xdp
 # Make sure we are able to include and link libbpf against c++.
 $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
 	$(call msg,CXX,,$@)
-	$(Q)$(CXX) $(CFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
+	$(Q)$(CXX) $(subst -D_GNU_SOURCE=,,$(CFLAGS)) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
 
 # Benchmark runner
 $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h $(BPFOBJ)