diff mbox series

[v1,02/13] libbpf: Make __printf define conditional

Message ID 20240310020509.647319-3-irogers@google.com (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series tools header compiler.h update | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async
bpf/vmtest-bpf-PR fail merge-conflict
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-27 fail Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat

Commit Message

Ian Rogers March 10, 2024, 2:04 a.m. UTC 
libbpf depends upon linux/err.h which has a linux/compiler.h
dependency. In the kernel includes, as opposed to the tools version,
linux/compiler.h includes linux/compiler_attributes.h which defines
__printf. As the libbpf.c __printf definition isn't guarded by an
ifndef, this leads to a duplicate definition compilation error when
trying to update the tools/include/linux/compiler.h. Fix this by
adding the missing ifndef.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/bpf/libbpf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko March 11, 2024, 5:49 p.m. UTC | #1 
On Sat, Mar 9, 2024 at 6:05 PM Ian Rogers <irogers@google.com> wrote:
>
> libbpf depends upon linux/err.h which has a linux/compiler.h
> dependency. In the kernel includes, as opposed to the tools version,
> linux/compiler.h includes linux/compiler_attributes.h which defines
> __printf. As the libbpf.c __printf definition isn't guarded by an
> ifndef, this leads to a duplicate definition compilation error when
> trying to update the tools/include/linux/compiler.h. Fix this by
> adding the missing ifndef.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/bpf/libbpf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index afd09571c482..2152360b4b18 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -66,7 +66,9 @@
>   */
>  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
>
> -#define __printf(a, b) __attribute__((format(printf, a, b)))
> +#ifndef __printf
> +# define __printf(a, b)        __attribute__((format(printf, a, b)))

styling nit: don't add spaces between # and define, please

overall LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

Two questions, though.

1. It seems like just dropping #define __printf in libbpf.c compiles
fine (I checked both building libbpf directly, and BPF selftest, and
perf, and bpftool directly, all of them built fine). So we can
probably just drop this. I'll need to add __printf on Github, but
that's fine.

2. Logistics. Which tree should this patch go through? Can I land it
in bpf-next or it's too much inconvenience for you?


> +#endif
>
>  static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
>  static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog);
> --
> 2.44.0.278.ge034bb2e1d-goog
>
Ian Rogers March 11, 2024, 6:54 p.m. UTC | #2 
On Mon, Mar 11, 2024 at 10:49 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Mar 9, 2024 at 6:05 PM Ian Rogers <irogers@google.com> wrote:
> >
> > libbpf depends upon linux/err.h which has a linux/compiler.h
> > dependency. In the kernel includes, as opposed to the tools version,
> > linux/compiler.h includes linux/compiler_attributes.h which defines
> > __printf. As the libbpf.c __printf definition isn't guarded by an
> > ifndef, this leads to a duplicate definition compilation error when
> > trying to update the tools/include/linux/compiler.h. Fix this by
> > adding the missing ifndef.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index afd09571c482..2152360b4b18 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -66,7 +66,9 @@
> >   */
> >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> >
> > -#define __printf(a, b) __attribute__((format(printf, a, b)))
> > +#ifndef __printf
> > +# define __printf(a, b)        __attribute__((format(printf, a, b)))
>
> styling nit: don't add spaces between # and define, please
>
> overall LGTM
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> Two questions, though.
>
> 1. It seems like just dropping #define __printf in libbpf.c compiles
> fine (I checked both building libbpf directly, and BPF selftest, and
> perf, and bpftool directly, all of them built fine). So we can
> probably just drop this. I'll need to add __printf on Github, but
> that's fine.
>
> 2. Logistics. Which tree should this patch go through? Can I land it
> in bpf-next or it's too much inconvenience for you?

Thanks Andrii,

dropping the #define (1) sgtm but the current compiler.h will fail to
build libbpf.c without the later compiler.h update in this series.
This causes another logistic issue for your point 2. Presumably if
this patch goes through bpf-next, the first patch "tools bpf:
Synchronize bpf.h with kernel uapi version" should also go through the
bpf-next.

Thanks,
Ian


> > +#endif
> >
> >  static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
> >  static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog);
> > --
> > 2.44.0.278.ge034bb2e1d-goog
> >
Andrii Nakryiko March 11, 2024, 8:51 p.m. UTC | #3 
On Mon, Mar 11, 2024 at 11:54 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Mar 11, 2024 at 10:49 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Mar 9, 2024 at 6:05 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > libbpf depends upon linux/err.h which has a linux/compiler.h
> > > dependency. In the kernel includes, as opposed to the tools version,
> > > linux/compiler.h includes linux/compiler_attributes.h which defines
> > > __printf. As the libbpf.c __printf definition isn't guarded by an
> > > ifndef, this leads to a duplicate definition compilation error when
> > > trying to update the tools/include/linux/compiler.h. Fix this by
> > > adding the missing ifndef.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index afd09571c482..2152360b4b18 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -66,7 +66,9 @@
> > >   */
> > >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > >
> > > -#define __printf(a, b) __attribute__((format(printf, a, b)))
> > > +#ifndef __printf
> > > +# define __printf(a, b)        __attribute__((format(printf, a, b)))
> >
> > styling nit: don't add spaces between # and define, please
> >
> > overall LGTM
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > Two questions, though.
> >
> > 1. It seems like just dropping #define __printf in libbpf.c compiles
> > fine (I checked both building libbpf directly, and BPF selftest, and
> > perf, and bpftool directly, all of them built fine). So we can
> > probably just drop this. I'll need to add __printf on Github, but
> > that's fine.
> >
> > 2. Logistics. Which tree should this patch go through? Can I land it
> > in bpf-next or it's too much inconvenience for you?
>
> Thanks Andrii,
>
> dropping the #define (1) sgtm but the current compiler.h will fail to
> build libbpf.c without the later compiler.h update in this series.
> This causes another logistic issue for your point 2. Presumably if
> this patch goes through bpf-next, the first patch "tools bpf:
> Synchronize bpf.h with kernel uapi version" should also go through the
> bpf-next.
>

That's what I'm saying, it seems to work without your patches already.
At least on bpf-next/master. But it's ok, let's keep it and just add
#ifndef guard, that will make my life easier when syncing to Github
later one. Then the patch can go through other trees and eventually
make it into bpf-next and then Github. So please keep my ack for
#ifndef version, thanks.

> Thanks,
> Ian
>
>
> > > +#endif
> > >
> > >  static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
> > >  static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog);
> > > --
> > > 2.44.0.278.ge034bb2e1d-goog
> > >
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index afd09571c482..2152360b4b18 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -66,7 +66,9 @@ 
  */
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
 
-#define __printf(a, b)	__attribute__((format(printf, a, b)))
+#ifndef __printf
+# define __printf(a, b)	__attribute__((format(printf, a, b)))
+#endif
 
 static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
 static bool prog_is_subprog(const struct bpf_object *obj, const struct bpf_program *prog);