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

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

[Stanislav Fomichev]

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 $@
 
 # Benchmark runner
 $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h $(BPFOBJ)
-- 
2.45.2

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

[Jakub Kicinski]

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.

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

[Jiri Olsa]

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
> 

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

[Andrii Nakryiko]

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
>

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

[Jakub Kicinski]

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 :)

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

[Stanislav Fomichev]

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.

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

[Yonghong Song]

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.

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html