Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.

This mirrors what tools/perf and other directories in tools do to handle
-Werror rather than adding it unconditionally.

Signed-off-by: Sam James <sam@gentoo.org>
---
 tools/lib/bpf/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 168140f8e646..9563d37265da 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -77,10 +77,15 @@ else
   CFLAGS := -g -O2
 endif
 
+# Treat warnings as errors unless directed not to
+ifneq ($(WERROR),0)
+  CFLAGS += -Werror
+endif
+
 # Append required CFLAGS
 override CFLAGS += -std=gnu89
 override CFLAGS += $(EXTRA_WARNINGS) -Wno-switch-enum
-override CFLAGS += -Werror -Wall
+override CFLAGS += -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
-- 
2.50.0

On 7/5/25 12:43 PM, Sam James wrote:
> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
> 
> This mirrors what tools/perf and other directories in tools do to handle
> -Werror rather than adding it unconditionally.

Could you also add to the commit desc why you need it? Are there particular
warnings you specifically need to suppress when building under gentoo?

> Signed-off-by: Sam James <sam@gentoo.org>
> ---
>   tools/lib/bpf/Makefile | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 168140f8e646..9563d37265da 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -77,10 +77,15 @@ else
>     CFLAGS := -g -O2
>   endif
>   
> +# Treat warnings as errors unless directed not to
> +ifneq ($(WERROR),0)
> +  CFLAGS += -Werror
> +endif

Should we also add sth similar to tools/bpf/bpftool/Makefile and by default
enforce with -Werror with the option to disable?

>   # Append required CFLAGS
>   override CFLAGS += -std=gnu89
>   override CFLAGS += $(EXTRA_WARNINGS) -Wno-switch-enum
> -override CFLAGS += -Werror -Wall
> +override CFLAGS += -Wall
>   override CFLAGS += $(INCLUDES)
>   override CFLAGS += -fvisibility=hidden
>   override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 7/5/25 12:43 PM, Sam James wrote:
>> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
>> This mirrors what tools/perf and other directories in tools do to
>> handle
>> -Werror rather than adding it unconditionally.
>
> Could you also add to the commit desc why you need it? Are there particular
> warnings you specifically need to suppress when building under gentoo?

Sure. In this case, it was https://bugs.gentoo.org/959293 where I think
it's fixed by
https://github.com/libbpf/libbpf/commit/715808d3e2d8c54f3001ce3d7fcda0844f765969
(and the corresponding commit in the kernel tree proper). Backporting
that was a bit too big for our tastes.

The real issue is just that -Werror when we have users who might be
testing with in-development compilers or with alternative options
results in a build failure when you didn't expect one.

>
>> Signed-off-by: Sam James <sam@gentoo.org>
>> ---
>>   tools/lib/bpf/Makefile | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> index 168140f8e646..9563d37265da 100644
>> --- a/tools/lib/bpf/Makefile
>> +++ b/tools/lib/bpf/Makefile
>> @@ -77,10 +77,15 @@ else
>>     CFLAGS := -g -O2
>>   endif
>>   +# Treat warnings as errors unless directed not to
>> +ifneq ($(WERROR),0)
>> +  CFLAGS += -Werror
>> +endif
>
> Should we also add sth similar to tools/bpf/bpftool/Makefile and by default
> enforce with -Werror with the option to disable?

Yes, that sounds good to me, though I was nervous of stumbling onto a
philosophical debate about -Werror and wasn't sure what y'all preferred
:)

I can send v2 with an updated commit message and this change. I'll wait
a bit for further comments based on my two replies here.

>
>>   # Append required CFLAGS
>>   override CFLAGS += -std=gnu89
>>   override CFLAGS += $(EXTRA_WARNINGS) -Wno-switch-enum
>> -override CFLAGS += -Werror -Wall
>> +override CFLAGS += -Wall
>>   override CFLAGS += $(INCLUDES)
>>   override CFLAGS += -fvisibility=hidden
>>   override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64

sam

On Sat, Jul 12, 2025 at 11:24 PM Sam James <sam@gentoo.org> wrote:
>
> Daniel Borkmann <daniel@iogearbox.net> writes:
>
> > On 7/5/25 12:43 PM, Sam James wrote:
> >> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
> >> This mirrors what tools/perf and other directories in tools do to
> >> handle
> >> -Werror rather than adding it unconditionally.
> >
> > Could you also add to the commit desc why you need it? Are there particular
> > warnings you specifically need to suppress when building under gentoo?
>
> Sure. In this case, it was https://bugs.gentoo.org/959293 where I think

I don't recall it was reported on bpf mailing list.

> it's fixed by
> https://github.com/libbpf/libbpf/commit/715808d3e2d8c54f3001ce3d7fcda0844f765969

and looks like it was fixed by accident, so..

> (and the corresponding commit in the kernel tree proper). Backporting
> that was a bit too big for our tastes.
>
> The real issue is just that -Werror when we have users who might be
> testing with in-development compilers or with alternative options
> results in a build failure when you didn't expect one.
>
> >
> >> Signed-off-by: Sam James <sam@gentoo.org>
> >> ---
> >>   tools/lib/bpf/Makefile | 7 ++++++-
> >>   1 file changed, 6 insertions(+), 1 deletion(-)
> >> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> >> index 168140f8e646..9563d37265da 100644
> >> --- a/tools/lib/bpf/Makefile
> >> +++ b/tools/lib/bpf/Makefile
> >> @@ -77,10 +77,15 @@ else
> >>     CFLAGS := -g -O2
> >>   endif
> >>   +# Treat warnings as errors unless directed not to
> >> +ifneq ($(WERROR),0)
> >> +  CFLAGS += -Werror
> >> +endif
> >
> > Should we also add sth similar to tools/bpf/bpftool/Makefile and by default
> > enforce with -Werror with the option to disable?
>
> Yes, that sounds good to me, though I was nervous of stumbling onto a
> philosophical debate about -Werror and wasn't sure what y'all preferred
> :)
>
> I can send v2 with an updated commit message and this change. I'll wait
> a bit for further comments based on my two replies here.

No.
We want Werror to be there by default and it shouldn't be trivial to turn off,
so that people report and fix issues with new compilers.
Like in this case, looks like it was a legitimate error of
in-development gcc-16.
If it was reported to us and turned out to be not a libbpf issue than
gentoo should have reported it back to gcc devs to make sure they don't
add bogus warnings to the compiler. Win-win.

You're right, in many ways it is a philosophical debate.
We cleaned up libbpf and selftests/bpf from warnings and
we don't want them to reappear. So we don't want an easy way
to silence them. Report issues instead.

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Sat, Jul 12, 2025 at 11:24 PM Sam James <sam@gentoo.org> wrote:
>>
>> Daniel Borkmann <daniel@iogearbox.net> writes:
>>
>> > On 7/5/25 12:43 PM, Sam James wrote:
>> >> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
>> >> This mirrors what tools/perf and other directories in tools do to
>> >> handle
>> >> -Werror rather than adding it unconditionally.
>> >
>> > Could you also add to the commit desc why you need it? Are there particular
>> > warnings you specifically need to suppress when building under gentoo?
>>
>> Sure. In this case, it was https://bugs.gentoo.org/959293 where I think
>
> I don't recall it was reported on bpf mailing list.
>
>> it's fixed by
>> https://github.com/libbpf/libbpf/commit/715808d3e2d8c54f3001ce3d7fcda0844f765969
>
> and looks like it was fixed by accident, so..

I'll note that I've sent patches that have been merged for these
before. It's just that they're sensitive to optimisation level and prone
to false positives. Especially with e.g. -Og.

>
>> (and the corresponding commit in the kernel tree proper). Backporting
>> that was a bit too big for our tastes.
>>
>> The real issue is just that -Werror when we have users who might be
>> testing with in-development compilers or with alternative options
>> results in a build failure when you didn't expect one.
>>
>> >
>> >> Signed-off-by: Sam James <sam@gentoo.org>
>> >> ---
>> >>   tools/lib/bpf/Makefile | 7 ++++++-
>> >>   1 file changed, 6 insertions(+), 1 deletion(-)
>> >> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> >> index 168140f8e646..9563d37265da 100644
>> >> --- a/tools/lib/bpf/Makefile
>> >> +++ b/tools/lib/bpf/Makefile
>> >> @@ -77,10 +77,15 @@ else
>> >>     CFLAGS := -g -O2
>> >>   endif
>> >>   +# Treat warnings as errors unless directed not to
>> >> +ifneq ($(WERROR),0)
>> >> +  CFLAGS += -Werror
>> >> +endif
>> >
>> > Should we also add sth similar to tools/bpf/bpftool/Makefile and by default
>> > enforce with -Werror with the option to disable?
>>
>> Yes, that sounds good to me, though I was nervous of stumbling onto a
>> philosophical debate about -Werror and wasn't sure what y'all preferred
>> :)
>>
>> I can send v2 with an updated commit message and this change. I'll wait
>> a bit for further comments based on my two replies here.
>
> No.
> We want Werror to be there by default and it shouldn't be trivial to turn off,
> so that people report and fix issues with new compilers.
> Like in this case, looks like it was a legitimate error of
> in-development gcc-16.
> If it was reported to us and turned out to be not a libbpf issue than
> gentoo should have reported it back to gcc devs to make sure they don't
> add bogus warnings to the compiler. Win-win.
>
> You're right, in many ways it is a philosophical debate.
> We cleaned up libbpf and selftests/bpf from warnings and
> we don't want them to reappear. So we don't want an easy way
> to silence them. Report issues instead.

OK then.

On Sun, Jul 13, 2025 at 3:58 PM Sam James <sam@gentoo.org> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> > On Sat, Jul 12, 2025 at 11:24 PM Sam James <sam@gentoo.org> wrote:
> >>
> >> Daniel Borkmann <daniel@iogearbox.net> writes:
> >>
> >> > On 7/5/25 12:43 PM, Sam James wrote:
> >> >> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
> >> >> This mirrors what tools/perf and other directories in tools do to
> >> >> handle
> >> >> -Werror rather than adding it unconditionally.
> >> >
> >> > Could you also add to the commit desc why you need it? Are there particular
> >> > warnings you specifically need to suppress when building under gentoo?
> >>
> >> Sure. In this case, it was https://bugs.gentoo.org/959293 where I think
> >
> > I don't recall it was reported on bpf mailing list.
> >
> >> it's fixed by
> >> https://github.com/libbpf/libbpf/commit/715808d3e2d8c54f3001ce3d7fcda0844f765969
> >
> > and looks like it was fixed by accident, so..
>
> I'll note that I've sent patches that have been merged for these
> before. It's just that they're sensitive to optimisation level and prone
> to false positives. Especially with e.g. -Og.

Yeah. Compilers do produce false positives and, like any tool,
any warning is not authoritative, but we prefer people reporting
them instead of silencing and moving on.

2025-07-07 15:18 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
> On 7/5/25 12:43 PM, Sam James wrote:
>> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
>>
>> This mirrors what tools/perf and other directories in tools do to handle
>> -Werror rather than adding it unconditionally.
> 
> Could you also add to the commit desc why you need it? Are there particular
> warnings you specifically need to suppress when building under gentoo?


And if you need to disable the flag on a particular target, have you
considered using the existing variables and pass EXTRA_CFLAGS=-Wno-error
rather than adding another flag?

Quentin

Quentin Monnet <qmo@kernel.org> writes:

> 2025-07-07 15:18 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
>> On 7/5/25 12:43 PM, Sam James wrote:
>>> Check the 'WERROR' variable and suppress adding '-Werror' if WERROR=0.
>>>
>>> This mirrors what tools/perf and other directories in tools do to handle
>>> -Werror rather than adding it unconditionally.
>> 
>> Could you also add to the commit desc why you need it? Are there particular
>> warnings you specifically need to suppress when building under gentoo?
>
>
> And if you need to disable the flag on a particular target, have you
> considered using the existing variables and pass EXTRA_CFLAGS=-Wno-error
> rather than adding another flag?

I think inconsistency with other tools/ is unfortunate, but yes, I guess
we could.

>
> Quentin