| Message ID | 20211105191055.3324874-1-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: fix non-C89 loop variable declaration in gen_loader.c | expand |
On Sat, Nov 06, 2021 at 12:40:55AM IST, Andrii Nakryiko wrote: > 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> Thanks for the fix, and sorry about that. Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > [...] -- Kartikeya
On Fri, Nov 5, 2021 at 1:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sat, Nov 06, 2021 at 12:40:55AM IST, Andrii Nakryiko wrote: > > 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> > > Thanks for the fix, and sorry about that. > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > No worries, we just need to figure out which compiler flags we need to catch this. I'm surprised BCC build caught this and neither libbpf's Makefile nor selftest did. Selftests are definitely too permissive w.r.t. stuff like this. If you could take a look and see what we'll need to lock it down a bit, that would be great. I've also requested help from the original reporter of this issue (see issue on Github). > > [...] > > -- > Kartikeya
On Sat, Nov 06, 2021 at 03:19:38AM IST, Andrii Nakryiko wrote: > On Fri, Nov 5, 2021 at 1:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Sat, Nov 06, 2021 at 12:40:55AM IST, Andrii Nakryiko wrote: > > > 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> > > > > Thanks for the fix, and sorry about that. > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > No worries, we just need to figure out which compiler flags we need to > catch this. I'm surprised BCC build caught this and neither libbpf's > Makefile nor selftest did. Selftests are definitely too permissive > w.r.t. stuff like this. > > If you could take a look and see what we'll need to lock it down a > bit, that would be great. I've also requested help from the original > reporter of this issue (see issue on Github). > I think you want -std=gnu89 (i.e. C89 with GNU extensions). I get the same error as the reporter when building with that. > > > > [...] > > > > -- > > Kartikeya -- Kartikeya
On Fri, Nov 5, 2021 at 3:04 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sat, Nov 06, 2021 at 03:19:38AM IST, Andrii Nakryiko wrote: > > On Fri, Nov 5, 2021 at 1:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > On Sat, Nov 06, 2021 at 12:40:55AM IST, Andrii Nakryiko wrote: > > > > 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> > > > > > > Thanks for the fix, and sorry about that. > > > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > > No worries, we just need to figure out which compiler flags we need to > > catch this. I'm surprised BCC build caught this and neither libbpf's > > Makefile nor selftest did. Selftests are definitely too permissive > > w.r.t. stuff like this. > > > > If you could take a look and see what we'll need to lock it down a > > bit, that would be great. I've also requested help from the original > > reporter of this issue (see issue on Github). > > > > I think you want -std=gnu89 (i.e. C89 with GNU extensions). I get the same error > as the reporter when building with that. Oh, I think I tried -std=c89 and it didn't compile due to use of those extensions. Didn't realize gnu89 exists. Do you mind adding it to bpftool, libbpf, and selftests Makefiles and sending a patch? > > > > > > > [...] > > > > > > -- > > > Kartikeya > > -- > Kartikeya
On Sat, Nov 06, 2021 at 03:50:47AM IST, Andrii Nakryiko wrote: > On Fri, Nov 5, 2021 at 3:04 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Sat, Nov 06, 2021 at 03:19:38AM IST, Andrii Nakryiko wrote: > > > On Fri, Nov 5, 2021 at 1:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > On Sat, Nov 06, 2021 at 12:40:55AM IST, Andrii Nakryiko wrote: > > > > > 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> > > > > > > > > Thanks for the fix, and sorry about that. > > > > > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > > > > > No worries, we just need to figure out which compiler flags we need to > > > catch this. I'm surprised BCC build caught this and neither libbpf's > > > Makefile nor selftest did. Selftests are definitely too permissive > > > w.r.t. stuff like this. > > > > > > If you could take a look and see what we'll need to lock it down a > > > bit, that would be great. I've also requested help from the original > > > reporter of this issue (see issue on Github). > > > > > > > I think you want -std=gnu89 (i.e. C89 with GNU extensions). I get the same error > > as the reporter when building with that. > > Oh, I think I tried -std=c89 and it didn't compile due to use of those > extensions. Didn't realize gnu89 exists. Do you mind adding it to > bpftool, libbpf, and selftests Makefiles and sending a patch? > Sure, will do! -- Kartikeya
On Fri, Nov 5, 2021 at 12:11 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > 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 I'd prefer to fix bcc and/or its derivatives instead. It's year 2021.
On Sat, Nov 6, 2021 at 9:38 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Nov 5, 2021 at 12:11 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > 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 > > I'd prefer to fix bcc and/or its derivatives instead. > It's year 2021. Fixing BCC and derivatives is a separate topic. This is the only instance of non-C89 compliant for() loop in libbpf and I'd prefer to fix it at the very least for style consistency.
On Sat, Nov 6, 2021 at 12:56 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sat, Nov 6, 2021 at 9:38 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Nov 5, 2021 at 12:11 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > 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 > > > > I'd prefer to fix bcc and/or its derivatives instead. > > It's year 2021. > > Fixing BCC and derivatives is a separate topic. This is the only > instance of non-C89 compliant for() loop in libbpf and I'd prefer to > fix it at the very least for style consistency. Fair enough. Applied.
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];
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> --- tools/lib/bpf/gen_loader.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)