[bpf-next,v1,0/6] Change bpftool, libbpf, selftests to force GNU89 mode

Message

Kumar Kartikeya Dwivedi Nov. 5, 2021, 11:42 p.m. UTC

Fix any remaining instances that fail the build in this mode.  For selftests, we
also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
would generate a warning when used with g++.

This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
introducing new invalid usage of C99 features.

Andrii Nakryiko (1):
  libbpf: fix non-C89 loop variable declaration in gen_loader.c

Kumar Kartikeya Dwivedi (5):
  bpftool: Compile using -std=gnu89
  libbpf: Compile using -std=gnu89
  selftests/bpf: Fix non-C89 loop variable declaration instances
  selftests/bpf: Switch to non-unicode character in output
  selftests/bpf: Compile using -std=gnu89

 tools/bpf/bpftool/Makefile                           | 2 +-
 tools/lib/bpf/Makefile                               | 1 +
 tools/lib/bpf/gen_loader.c                           | 3 ++-
 tools/testing/selftests/bpf/Makefile                 | 5 ++++-
 tools/testing/selftests/bpf/bench.c                  | 6 +++---
 tools/testing/selftests/bpf/prog_tests/d_path.c      | 4 ++--
 tools/testing/selftests/bpf/prog_tests/timer_mim.c   | 6 +++---
 tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 4 ++--
 tools/testing/selftests/bpf/test_progs.c             | 2 +-
 9 files changed, 19 insertions(+), 14 deletions(-)

Comments

Alexei Starovoitov Nov. 6, 2021, 4:33 p.m. UTC | #1

On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
>
> Fix any remaining instances that fail the build in this mode.  For selftests, we
> also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> would generate a warning when used with g++.
>
> This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> introducing new invalid usage of C99 features.
>
> Andrii Nakryiko (1):
>   libbpf: fix non-C89 loop variable declaration in gen_loader.c
>
> Kumar Kartikeya Dwivedi (5):
>   bpftool: Compile using -std=gnu89
>   libbpf: Compile using -std=gnu89
>   selftests/bpf: Fix non-C89 loop variable declaration instances
>   selftests/bpf: Switch to non-unicode character in output
>   selftests/bpf: Compile using -std=gnu89

Please don't.
I'd rather go the other way and drop gnu89 from everywhere.
for (int i = 0
is so much cleaner.

Andrii Nakryiko Nov. 6, 2021, 8:02 p.m. UTC | #2

On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> >
> > Fix any remaining instances that fail the build in this mode.  For selftests, we
> > also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> > would generate a warning when used with g++.
> >
> > This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> > introducing new invalid usage of C99 features.
> >
> > Andrii Nakryiko (1):
> >   libbpf: fix non-C89 loop variable declaration in gen_loader.c
> >
> > Kumar Kartikeya Dwivedi (5):
> >   bpftool: Compile using -std=gnu89
> >   libbpf: Compile using -std=gnu89
> >   selftests/bpf: Fix non-C89 loop variable declaration instances
> >   selftests/bpf: Switch to non-unicode character in output
> >   selftests/bpf: Compile using -std=gnu89
>
> Please don't.
> I'd rather go the other way and drop gnu89 from everywhere.
> for (int i = 0
> is so much cleaner.

I agree that for (int i) is better, but it's kernel code style which
we followed so far pretty closely for libbpf and bpftool. So I think
this is the right move for bpftool and libbpf.

Selftests are less consistent in styling and lack of unicode character
in bench is annoying, so I don't mind leaving selftest more
permissive.

And even with all that, we've managed to keep BPF program code
consistently in C89 here in selftests and in bcc/libbpf-tools. The
code style uniformity is nice.

Whether to relax BCC compilation flags is a separate discussion (and I
don't have any strong opinion). I'd still enforce -std=gnu89 for BPF
source code for consistency across many BPF projects.

Alexei Starovoitov Nov. 6, 2021, 11:20 p.m. UTC | #3

On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> > >
> > > Fix any remaining instances that fail the build in this mode.  For selftests, we
> > > also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> > > would generate a warning when used with g++.
> > >
> > > This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> > > introducing new invalid usage of C99 features.
> > >
> > > Andrii Nakryiko (1):
> > >   libbpf: fix non-C89 loop variable declaration in gen_loader.c
> > >
> > > Kumar Kartikeya Dwivedi (5):
> > >   bpftool: Compile using -std=gnu89
> > >   libbpf: Compile using -std=gnu89
> > >   selftests/bpf: Fix non-C89 loop variable declaration instances
> > >   selftests/bpf: Switch to non-unicode character in output
> > >   selftests/bpf: Compile using -std=gnu89
> >
> > Please don't.
> > I'd rather go the other way and drop gnu89 from everywhere.
> > for (int i = 0
> > is so much cleaner.
>
> I agree that for (int i) is better, but it's kernel code style which
> we followed so far pretty closely for libbpf and bpftool. So I think
> this is the right move for bpftool and libbpf.

The kernel coding style is not white and black.
Certain style preferences are archaic to say the least.
It's not the right move to follow it blindly.

Andrii Nakryiko Nov. 8, 2021, 10:15 p.m. UTC | #4

On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > <[e-mail]> wrote:
> > >
> > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> > > >
> > > > Fix any remaining instances that fail the build in this mode.  For selftests, we
> > > > also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> > > > would generate a warning when used with g++.
> > > >
> > > > This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> > > > introducing new invalid usage of C99 features.
> > > >
> > > > Andrii Nakryiko (1):
> > > >   libbpf: fix non-C89 loop variable declaration in gen_loader.c
> > > >
> > > > Kumar Kartikeya Dwivedi (5):
> > > >   bpftool: Compile using -std=gnu89
> > > >   libbpf: Compile using -std=gnu89
> > > >   selftests/bpf: Fix non-C89 loop variable declaration instances
> > > >   selftests/bpf: Switch to non-unicode character in output
> > > >   selftests/bpf: Compile using -std=gnu89
> > >
> > > Please don't.
> > > I'd rather go the other way and drop gnu89 from everywhere.
> > > for (int i = 0
> > > is so much cleaner.
> >
> > I agree that for (int i) is better, but it's kernel code style which
> > we followed so far pretty closely for libbpf and bpftool. So I think
> > this is the right move for bpftool and libbpf.
>
> The kernel coding style is not white and black.
> Certain style preferences are archaic to say the least.
> It's not the right move to follow it blindly.

Can we at least add -std=gnu89 for the libbpf? It's a library, so
being conservative with compiler versions and language features makes
sense there. I'll add a similar flag to Github's Makefile. I'd rather
catch this at patch submission time rather than at the Github sync
time.

Alexei Starovoitov Nov. 9, 2021, 9:40 p.m. UTC | #5

On Mon, Nov 8, 2021 at 2:15 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> > <[e-mail]> wrote:
> > >
> > > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > > <[e-mail]> wrote:
> > > >
> > > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> > > > >
> > > > > Fix any remaining instances that fail the build in this mode.  For selftests, we
> > > > > also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> > > > > would generate a warning when used with g++.
> > > > >
> > > > > This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> > > > > introducing new invalid usage of C99 features.
> > > > >
> > > > > Andrii Nakryiko (1):
> > > > >   libbpf: fix non-C89 loop variable declaration in gen_loader.c
> > > > >
> > > > > Kumar Kartikeya Dwivedi (5):
> > > > >   bpftool: Compile using -std=gnu89
> > > > >   libbpf: Compile using -std=gnu89
> > > > >   selftests/bpf: Fix non-C89 loop variable declaration instances
> > > > >   selftests/bpf: Switch to non-unicode character in output
> > > > >   selftests/bpf: Compile using -std=gnu89
> > > >
> > > > Please don't.
> > > > I'd rather go the other way and drop gnu89 from everywhere.
> > > > for (int i = 0
> > > > is so much cleaner.
> > >
> > > I agree that for (int i) is better, but it's kernel code style which
> > > we followed so far pretty closely for libbpf and bpftool. So I think
> > > this is the right move for bpftool and libbpf.
> >
> > The kernel coding style is not white and black.
> > Certain style preferences are archaic to say the least.
> > It's not the right move to follow it blindly.
>
> Can we at least add -std=gnu89 for the libbpf? It's a library, so
> being conservative with compiler versions and language features makes
> sense there. I'll add a similar flag to Github's Makefile. I'd rather
> catch this at patch submission time rather than at the Github sync
> time.

Sure. Applied Kumar's patch 3.
With CO-RE in the kernel the pieces of libbpf will be part
of the kernel for real, so for libbpf as a whole would make sense
to conform to the language standards as parts of libbpf have to do.
As far as other parts of kernel git the language standard
can be decided whichever way.
perf and libsubcmd (part of objtool) have no issue using 'for (int'
while being part of the kernel tree.
We can adopt strong gnu89 in bpftool, but I'd rather not rush
such a decision right now.
selftests are certainly not gnu89.
All bpf programs are written in C-2021 "standard".
Lots of C extensions in there.

Andrii Nakryiko Nov. 9, 2021, 11:16 p.m. UTC | #6

On Tue, Nov 9, 2021 at 1:40 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Mon, Nov 8, 2021 at 2:15 PM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
> > <[e-mail]> wrote:
> > >
> > > On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> > > <[e-mail]> wrote:
> > > >
> > > > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> > > > > >
> > > > > > Fix any remaining instances that fail the build in this mode.  For selftests, we
> > > > > > also need to separate CXXFLAGS from CFLAGS, since adding it to CFLAGS simply
> > > > > > would generate a warning when used with g++.
> > > > > >
> > > > > > This also cherry-picks Andrii's patch to fix the instance in libbpf. Also tested
> > > > > > introducing new invalid usage of C99 features.
> > > > > >
> > > > > > Andrii Nakryiko (1):
> > > > > >   libbpf: fix non-C89 loop variable declaration in gen_loader.c
> > > > > >
> > > > > > Kumar Kartikeya Dwivedi (5):
> > > > > >   bpftool: Compile using -std=gnu89
> > > > > >   libbpf: Compile using -std=gnu89
> > > > > >   selftests/bpf: Fix non-C89 loop variable declaration instances
> > > > > >   selftests/bpf: Switch to non-unicode character in output
> > > > > >   selftests/bpf: Compile using -std=gnu89
> > > > >
> > > > > Please don't.
> > > > > I'd rather go the other way and drop gnu89 from everywhere.
> > > > > for (int i = 0
> > > > > is so much cleaner.
> > > >
> > > > I agree that for (int i) is better, but it's kernel code style which
> > > > we followed so far pretty closely for libbpf and bpftool. So I think
> > > > this is the right move for bpftool and libbpf.
> > >
> > > The kernel coding style is not white and black.
> > > Certain style preferences are archaic to say the least.
> > > It's not the right move to follow it blindly.
> >
> > Can we at least add -std=gnu89 for the libbpf? It's a library, so
> > being conservative with compiler versions and language features makes
> > sense there. I'll add a similar flag to Github's Makefile. I'd rather
> > catch this at patch submission time rather than at the Github sync
> > time.
>
> Sure. Applied Kumar's patch 3.
> With CO-RE in the kernel the pieces of libbpf will be part
> of the kernel for real, so for libbpf as a whole would make sense
> to conform to the language standards as parts of libbpf have to do.
> As far as other parts of kernel git the language standard
> can be decided whichever way.
> perf and libsubcmd (part of objtool) have no issue using 'for (int'
> while being part of the kernel tree.
> We can adopt strong gnu89 in bpftool, but I'd rather not rush
> such a decision right now.
> selftests are certainly not gnu89.
> All bpf programs are written in C-2021 "standard".
> Lots of C extensions in there.

sgtm