Re: [PATCH bpf-next v1 1/2] bpftool: Add -Wformat-signedness flag to detect format errors

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

March 10, 2025 at 23:49, "Quentin Monnet" <qmo@kernel.org> wrote:

> 
> 2025-03-10 22:20 UTC+0800 ~ Jiayuan Chen <jiayuan.chen@linux.dev>
> 
> > 
> > This commit adds the -Wformat-signedness compiler flag to detect and
> > 
> >  prevent printf format errors, where signed or unsigned types are
> > 
> >  mismatched with format specifiers. This helps to catch potential issues at
> > 
> >  compile-time, ensuring that our code is more robust and reliable. With
> > 
> >  this flag, the compiler will now warn about incorrect format strings, such
> > 
> >  as using %d with unsigned types or %u with signed types.
> > 
> >  
> > 
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> > 
> 
> Acked-by: Quentin Monnet <qmo@kernel.org>
> 
> Thanks for that. Have you looked into enabling the flag along with the
> 
> other EXTRA_WARNINGS in tools/scripts/Makefile.include? It would be
> 
> ideal to have it there, but I suppose it raises too many warnings across
> 
> tools/? (I didn't try myself.) No objection to taking it in bpftool only.
> 

Thanks for reminding me of these.

I tried adding it to Makefile.include, and indeed there were a lot of warnings,
but this part is not part of bpftool. I'll take a look later and try to enable this
options to fix these warnings as well. For now, I'll only fix the warnings related
to bpftool and these changes can also be easily synced to the bpftool on github.

> > ---
> > 
> >  tools/bpf/bpftool/Makefile | 2 +-
> > 
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> >  
> > 
> >  diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > 
> >  index dd9f3ec84201..d9f3eb51a48f 100644
> > 
> >  --- a/tools/bpf/bpftool/Makefile
> > 
> >  +++ b/tools/bpf/bpftool/Makefile
> > 
> >  @@ -71,7 +71,7 @@ prefix ?= /usr/local
> > 
> >  bash_compdir ?= /usr/share/bash-completion/completions
> > 
> >  
> > 
> >  CFLAGS += -O2
> > 
> >  -CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
> > 
> >  +CFLAGS += -W -Wall -Wextra -Wformat-signedness -Wno-unused-parameter -Wno-missing-field-initializers
> > 
> 
> Nit: This line is becoming long enough that I'd consider moving each
> 
> flag to its own line, for better reading:
> 
>  CFLAGS += -W
> 
>  CFLAGS += -Wall
> 
>  CFLAGS += -Wextra
> 
>  CFLAGS += -Wformat-signedness
> 
>  ...
OK，I will do this.