* [PATCH bpf-next v1 1/6] bpftool: Compile using -std=gnu89
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 2/6] libbpf: fix non-C89 loop variable declaration in gen_loader.c Kumar Kartikeya Dwivedi
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
The minimum supported C standard version is C89, with use of GNU
extensions, hence make sure to catch any instances that would break
the build for this mode by passing -std=gnu89.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
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 c0c30e56988f..71a7b7194c2c 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -65,7 +65,7 @@ $(LIBBPF_BOOTSTRAP)-clean: FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
prefix ?= /usr/local
bash_compdir ?= /usr/share/bash-completion/completions
-CFLAGS += -O2
+CFLAGS += -std=gnu89 -O2
CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
CFLAGS += $(filter-out -Wswitch-enum -Wnested-externs,$(EXTRA_WARNINGS))
CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ \
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next v1 2/6] libbpf: fix non-C89 loop variable declaration in gen_loader.c
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 1/6] bpftool: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 3/6] libbpf: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
From: Andrii Nakryiko <andrii@kernel.org>
Fix the `int i` declaration inside the for statement. This is non-C89
compliant. See [0] for user report breaking BCC build.
[0] https://github.com/libbpf/libbpf/issues/403
Fixes: 18f4fccbf314 ("libbpf: Update gen_loader to emit BTF_KIND_FUNC relocations")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/gen_loader.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 502dea53a742..2e10776b6d85 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -584,8 +584,9 @@ void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
static struct ksym_desc *get_ksym_desc(struct bpf_gen *gen, struct ksym_relo_desc *relo)
{
struct ksym_desc *kdesc;
+ int i;
- for (int i = 0; i < gen->nr_ksyms; i++) {
+ for (i = 0; i < gen->nr_ksyms; i++) {
if (!strcmp(gen->ksyms[i].name, relo->name)) {
gen->ksyms[i].ref++;
return &gen->ksyms[i];
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next v1 3/6] libbpf: Compile using -std=gnu89
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 1/6] bpftool: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 2/6] libbpf: fix non-C89 loop variable declaration in gen_loader.c Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 4/6] selftests/bpf: Fix non-C89 loop variable declaration instances Kumar Kartikeya Dwivedi
` (3 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
The minimum supported C standard version is C89, with use of GNU
extensions, hence make sure to catch any instances that would break
the build for this mode by passing -std=gnu89.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index b393b5e82380..5f7086fae31c 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -84,6 +84,7 @@ else
endif
# Append required CFLAGS
+override CFLAGS += -std=gnu89
override CFLAGS += $(EXTRA_WARNINGS) -Wno-switch-enum
override CFLAGS += -Werror -Wall
override CFLAGS += $(INCLUDES)
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next v1 4/6] selftests/bpf: Fix non-C89 loop variable declaration instances
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
` (2 preceding siblings ...)
2021-11-05 23:42 ` [PATCH bpf-next v1 3/6] libbpf: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 5/6] selftests/bpf: Switch to non-unicode character in output Kumar Kartikeya Dwivedi
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
Fix the `int i` declaration inside the for statement. This is non-C89
compliant. Doing all of them in this change since they are trivial.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
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 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
index 0a577a248d34..cc787ad68081 100644
--- a/tools/testing/selftests/bpf/prog_tests/d_path.c
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -103,7 +103,7 @@ void test_d_path(void)
{
struct test_d_path__bss *bss;
struct test_d_path *skel;
- int err;
+ int err, i;
skel = test_d_path__open_and_load();
if (CHECK(!skel, "setup", "d_path skeleton failed\n"))
@@ -130,7 +130,7 @@ void test_d_path(void)
"trampoline for filp_close was not called\n"))
goto cleanup;
- for (int i = 0; i < MAX_FILES; i++) {
+ for (i = 0; i < MAX_FILES; i++) {
CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
"check",
"failed to get stat path[%d]: %s vs %s\n",
diff --git a/tools/testing/selftests/bpf/prog_tests/timer_mim.c b/tools/testing/selftests/bpf/prog_tests/timer_mim.c
index 949a0617869d..f12536c32e2d 100644
--- a/tools/testing/selftests/bpf/prog_tests/timer_mim.c
+++ b/tools/testing/selftests/bpf/prog_tests/timer_mim.c
@@ -6,9 +6,9 @@
static int timer_mim(struct timer_mim *timer_skel)
{
+ int err, prog_fd, key1 = 1, i;
__u32 duration = 0, retval;
__u64 cnt1, cnt2;
- int err, prog_fd, key1 = 1;
err = timer_mim__attach(timer_skel);
if (!ASSERT_OK(err, "timer_attach"))
@@ -23,7 +23,7 @@ static int timer_mim(struct timer_mim *timer_skel)
/* check that timer_cb[12] are incrementing 'cnt' */
cnt1 = READ_ONCE(timer_skel->bss->cnt);
- for (int i = 0; i < 100; i++) {
+ for (i = 0; i < 100; i++) {
cnt2 = READ_ONCE(timer_skel->bss->cnt);
if (cnt2 != cnt1)
break;
@@ -41,7 +41,7 @@ static int timer_mim(struct timer_mim *timer_skel)
/* check that timer_cb[12] are no longer running */
cnt1 = READ_ONCE(timer_skel->bss->cnt);
- for (int i = 0; i < 100; i++) {
+ for (i = 0; i < 100; i++) {
usleep(200); /* 100 times more than interval */
cnt2 = READ_ONCE(timer_skel->bss->cnt);
if (cnt2 == cnt1)
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
index faa22b84f2ee..e5b8666e59eb 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -335,7 +335,7 @@ static void test_xdp_bonding_redirect_multi(struct skeletons *skeletons)
{
static const char * const ifaces[] = {"bond2", "veth2_1", "veth2_2"};
int veth1_1_rx, veth1_2_rx;
- int err;
+ int err, i;
if (bonding_setup(skeletons, BOND_MODE_ROUNDROBIN, BOND_XMIT_POLICY_LAYER23,
BOND_ONE_NO_ATTACH))
@@ -346,7 +346,7 @@ static void test_xdp_bonding_redirect_multi(struct skeletons *skeletons)
goto out;
/* populate the devmap with the relevant interfaces */
- for (int i = 0; i < ARRAY_SIZE(ifaces); i++) {
+ for (i = 0; i < ARRAY_SIZE(ifaces); i++) {
int ifindex = if_nametoindex(ifaces[i]);
int map_fd = bpf_map__fd(skeletons->xdp_redirect_multi_kern->maps.map_all);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c65986bd9d07..0096051e7560 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1146,7 +1146,7 @@ static int server_main(void)
/* run serial tests */
save_netns();
- for (int i = 0; i < prog_test_cnt; i++) {
+ for (i = 0; i < prog_test_cnt; i++) {
struct prog_test_def *test = &prog_test_defs[i];
struct test_result *result = &test_results[i];
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next v1 5/6] selftests/bpf: Switch to non-unicode character in output
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
` (3 preceding siblings ...)
2021-11-05 23:42 ` [PATCH bpf-next v1 4/6] selftests/bpf: Fix non-C89 loop variable declaration instances Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-05 23:42 ` [PATCH bpf-next v1 6/6] selftests/bpf: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
2021-11-06 16:33 ` [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Alexei Starovoitov
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
GNU89 doesn't allow unicode characters in printf output, and build gives
the following warning:
warning: universal character names are only valid in C++ and C99
Fix by using ASCII +/-.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/testing/selftests/bpf/bench.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index cc4722f693e9..58a32ff2eb1b 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -126,11 +126,11 @@ void hits_drops_report_final(struct bench_res res[], int res_cnt)
drops_stddev = sqrt(drops_stddev);
total_ops_stddev = sqrt(total_ops_stddev);
}
- printf("Summary: hits %8.3lf \u00B1 %5.3lfM/s (%7.3lfM/prod), ",
+ printf("Summary: hits %8.3lf +/- %5.3lfM/s (%7.3lfM/prod), ",
hits_mean, hits_stddev, hits_mean / env.producer_cnt);
- printf("drops %8.3lf \u00B1 %5.3lfM/s, ",
+ printf("drops %8.3lf +/- %5.3lfM/s, ",
drops_mean, drops_stddev);
- printf("total operations %8.3lf \u00B1 %5.3lfM/s\n",
+ printf("total operations %8.3lf +/- %5.3lfM/s\n",
total_ops_mean, total_ops_stddev);
}
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH bpf-next v1 6/6] selftests/bpf: Compile using -std=gnu89
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
` (4 preceding siblings ...)
2021-11-05 23:42 ` [PATCH bpf-next v1 5/6] selftests/bpf: Switch to non-unicode character in output Kumar Kartikeya Dwivedi
@ 2021-11-05 23:42 ` Kumar Kartikeya Dwivedi
2021-11-06 16:33 ` [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Alexei Starovoitov
6 siblings, 0 replies; 13+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-11-05 23:42 UTC (permalink / raw)
To: bpf; +Cc: Andrii Nakryiko
The minimum supported C standard version is C89, with use of GNU
extensions, hence make sure to catch any instances that would break
the build for this mode by passing -std=gnu89.
Also, copy out CFLAGS for C++ test so that we don't end up passing
-std=gnu89 to g++, otherwise the build generates a (harmless) warning:
cc1plus: warning: command-line option ‘-std=gnu90’ is valid for C/ObjC but not for C++
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 54b0a41a3775..6239e51c310f 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -33,6 +33,9 @@ ifneq ($(LLVM),)
CFLAGS += -Wno-unused-command-line-argument
endif
+CXXFLAGS := $(CFLAGS)
+CFLAGS += -std=gnu89
+
# Order correspond to 'make run_tests' order
TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
test_verifier_log test_dev_cgroup \
@@ -519,7 +522,7 @@ $(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
# 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) $(CXXFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
# Benchmark runner
$(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h $(BPFOBJ)
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-05 23:42 [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Kumar Kartikeya Dwivedi
` (5 preceding siblings ...)
2021-11-05 23:42 ` [PATCH bpf-next v1 6/6] selftests/bpf: Compile using -std=gnu89 Kumar Kartikeya Dwivedi
@ 2021-11-06 16:33 ` Alexei Starovoitov
2021-11-06 20:02 ` Andrii Nakryiko
6 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2021-11-06 16:33 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi; +Cc: bpf, Andrii Nakryiko
On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-06 16:33 ` [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode Alexei Starovoitov
@ 2021-11-06 20:02 ` Andrii Nakryiko
2021-11-06 23:20 ` Alexei Starovoitov
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-06 20:02 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Kumar Kartikeya Dwivedi, bpf, Andrii Nakryiko
On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-06 20:02 ` Andrii Nakryiko
@ 2021-11-06 23:20 ` Alexei Starovoitov
2021-11-08 22:15 ` Andrii Nakryiko
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2021-11-06 23:20 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: Kumar Kartikeya Dwivedi, bpf, Andrii Nakryiko
On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-06 23:20 ` Alexei Starovoitov
@ 2021-11-08 22:15 ` Andrii Nakryiko
2021-11-09 21:40 ` Alexei Starovoitov
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-08 22:15 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Kumar Kartikeya Dwivedi, bpf, Andrii Nakryiko
On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-08 22:15 ` Andrii Nakryiko
@ 2021-11-09 21:40 ` Alexei Starovoitov
2021-11-09 23:16 ` Andrii Nakryiko
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2021-11-09 21:40 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: Kumar Kartikeya Dwivedi, bpf, Andrii Nakryiko
On Mon, Nov 8, 2021 at 2:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 0/6] Change bpftool, libbpf, selftests to force GNU89 mode
2021-11-09 21:40 ` Alexei Starovoitov
@ 2021-11-09 23:16 ` Andrii Nakryiko
0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-09 23:16 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Kumar Kartikeya Dwivedi, bpf, Andrii Nakryiko
On Tue, Nov 9, 2021 at 1:40 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 8, 2021 at 2:15 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Nov 6, 2021 at 4:20 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sat, Nov 6, 2021 at 1:02 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Sat, Nov 6, 2021 at 9:34 AM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Fri, Nov 5, 2021 at 6:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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
^ permalink raw reply [flat|nested] 13+ messages in thread