Message ID | 20200114164250.922192-1-toke@redhat.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] libbpf: Fix include of bpf_helpers.h when libbpf is installed on system | expand |
On Tue, Jan 14, 2020 at 8:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > The change to use angled includes for bpf_helper_defs.h breaks compilation > against libbpf when it is installed in the include path, since the file is > installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the > bpf/ prefix to the #include directive. > > Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > Not actually sure this fix works for all the cases you originally tried to This does break selftests/bpf. Have you tried building selftests, does it work for you? We need to fix selftests simultaneously with this change. > fix with the referred commit; please check. Also, could we please stop breaking > libbpf builds? :) Which libbpf build is failing right now? Both github and in-kernel libbpf builds are fine. You must be referring to something else. What exactly? > > tools/lib/bpf/bpf_helpers.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 050bb7bf5be6..fa43d649e7a2 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -2,7 +2,7 @@ > #ifndef __BPF_HELPERS__ > #define __BPF_HELPERS__ > > -#include <bpf_helper_defs.h> > +#include <bpf/bpf_helper_defs.h> > > #define __uint(name, val) int (*name)[val] > #define __type(name, val) typeof(val) *name > -- > 2.24.1 >
On Tue, Jan 14, 2020 at 11:07 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Jan 14, 2020 at 8:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > > > The change to use angled includes for bpf_helper_defs.h breaks compilation > > against libbpf when it is installed in the include path, since the file is > > installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the > > bpf/ prefix to the #include directive. > > > > Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > > --- > > Not actually sure this fix works for all the cases you originally tried to > > This does break selftests/bpf. Have you tried building selftests, does > it work for you? We need to fix selftests simultaneously with this > change. > > > fix with the referred commit; please check. Also, could we please stop breaking > > libbpf builds? :) > > Which libbpf build is failing right now? Both github and in-kernel > libbpf builds are fine. You must be referring to something else. What > exactly? I think it's better to just ensure that when compiling BPF programs, they have -I/usr/include/bpf specified, so that all BPF-side headers can be simply included as #include <bpf_helpers.h>, #include <bpf_tracing.h>, etc > > > > > tools/lib/bpf/bpf_helpers.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index 050bb7bf5be6..fa43d649e7a2 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -2,7 +2,7 @@ > > #ifndef __BPF_HELPERS__ > > #define __BPF_HELPERS__ > > > > -#include <bpf_helper_defs.h> > > +#include <bpf/bpf_helper_defs.h> > > > > #define __uint(name, val) int (*name)[val] > > #define __type(name, val) typeof(val) *name > > -- > > 2.24.1 > >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Jan 14, 2020 at 11:07 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Tue, Jan 14, 2020 at 8:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> > >> > The change to use angled includes for bpf_helper_defs.h breaks compilation >> > against libbpf when it is installed in the include path, since the file is >> > installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the >> > bpf/ prefix to the #include directive. >> > >> > Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> > --- >> > Not actually sure this fix works for all the cases you originally tried to >> >> This does break selftests/bpf. Have you tried building selftests, does >> it work for you? We need to fix selftests simultaneously with this >> change. >> >> > fix with the referred commit; please check. Also, could we please stop breaking >> > libbpf builds? :) >> >> Which libbpf build is failing right now? Both github and in-kernel >> libbpf builds are fine. You must be referring to something else. What >> exactly? > > I think it's better to just ensure that when compiling BPF programs, > they have -I/usr/include/bpf specified, so that all BPF-side headers > can be simply included as #include <bpf_helpers.h>, #include > <bpf_tracing.h>, etc And break all programs that don't have that already? Just to make the kernel build env slightly more convenient? Hardly friendly to the library users, is it? :) As far as selftests are concerned, I finally managed to get an LLVM version that will build them all; so I'll test that tomorrow and send a v2 that doesn't break them... -Toke
On Tue, Jan 14, 2020 at 10:26:57PM +0100, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Jan 14, 2020 at 11:07 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Tue, Jan 14, 2020 at 8:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > > >> > The change to use angled includes for bpf_helper_defs.h breaks compilation > >> > against libbpf when it is installed in the include path, since the file is > >> > installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the > >> > bpf/ prefix to the #include directive. > >> > > >> > Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") > >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> > --- > >> > Not actually sure this fix works for all the cases you originally tried to > >> > >> This does break selftests/bpf. Have you tried building selftests, does > >> it work for you? We need to fix selftests simultaneously with this > >> change. > >> > >> > fix with the referred commit; please check. Also, could we please stop breaking > >> > libbpf builds? :) > >> > >> Which libbpf build is failing right now? Both github and in-kernel > >> libbpf builds are fine. You must be referring to something else. What > >> exactly? > > > > I think it's better to just ensure that when compiling BPF programs, > > they have -I/usr/include/bpf specified, so that all BPF-side headers > > can be simply included as #include <bpf_helpers.h>, #include > > <bpf_tracing.h>, etc > > And break all programs that don't have that already? Just to make the > kernel build env slightly more convenient? Hardly friendly to the > library users, is it? :) Could you explain the breakage ? bpf_helpers.h and bpf_helper_defs.h are installed in the same location. If prefix==/usr during make install of libbpf they both will go into /usr/include/bpf Are you saying the bpf progs had: #include <bpf/bpf_helpers.h> to pick that header from /usr/include and commit 6910d7d3867a that did: +++ b/tools/lib/bpf/bpf_helpers.h @@ -2,7 +2,7 @@ #ifndef __BPF_HELPERS__ #define __BPF_HELPERS__ -#include "bpf_helper_defs.h" +#include <bpf_helper_defs.h> broke it? If so this bit needs to be reverted. And we need a selfttest for such include order.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Tue, Jan 14, 2020 at 10:26:57PM +0100, Toke Høiland-Jørgensen wrote: >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Tue, Jan 14, 2020 at 11:07 AM Andrii Nakryiko >> > <andrii.nakryiko@gmail.com> wrote: >> >> >> >> On Tue, Jan 14, 2020 at 8:43 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> > >> >> > The change to use angled includes for bpf_helper_defs.h breaks compilation >> >> > against libbpf when it is installed in the include path, since the file is >> >> > installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the >> >> > bpf/ prefix to the #include directive. >> >> > >> >> > Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") >> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> >> > --- >> >> > Not actually sure this fix works for all the cases you originally tried to >> >> >> >> This does break selftests/bpf. Have you tried building selftests, does >> >> it work for you? We need to fix selftests simultaneously with this >> >> change. >> >> >> >> > fix with the referred commit; please check. Also, could we please stop breaking >> >> > libbpf builds? :) >> >> >> >> Which libbpf build is failing right now? Both github and in-kernel >> >> libbpf builds are fine. You must be referring to something else. What >> >> exactly? >> > >> > I think it's better to just ensure that when compiling BPF programs, >> > they have -I/usr/include/bpf specified, so that all BPF-side headers >> > can be simply included as #include <bpf_helpers.h>, #include >> > <bpf_tracing.h>, etc >> >> And break all programs that don't have that already? Just to make the >> kernel build env slightly more convenient? Hardly friendly to the >> library users, is it? :) > > Could you explain the breakage ? > bpf_helpers.h and bpf_helper_defs.h are installed in the same location. > If prefix==/usr during make install of libbpf they both will go into > /usr/include/bpf > > Are you saying the bpf progs had: > #include <bpf/bpf_helpers.h> > to pick that header from /usr/include and commit 6910d7d3867a that did: > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -2,7 +2,7 @@ > #ifndef __BPF_HELPERS__ > #define __BPF_HELPERS__ > > -#include "bpf_helper_defs.h" > +#include <bpf_helper_defs.h> > broke it? Yes. I get this error in xdp-tools[0] when updating lib/libbpf to the latest upstream from github: In file included from xdpfilt_blk_udp.c:10: In file included from ./xdpfilt_prog.h:12: ../lib/libbpf/src/root/usr/include/bpf/bpf_helpers.h:5:10: error: 'bpf_helper_defs.h' file not found with <angled> include; use "quotes" instead #include <bpf_helper_defs.h> ^~~~~~~~~~~~~~~~~~~ "bpf_helper_defs.h" 1 error generated. > If so this bit needs to be reverted. > And we need a selfttest for such include order. Yes, please! :) Trying to convince the selftests Makefile to do the right thing; will send a v2 of this patch if I succeed... -Toke [0] https://github.com/xdp-project/xdp-tools
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 050bb7bf5be6..fa43d649e7a2 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -2,7 +2,7 @@ #ifndef __BPF_HELPERS__ #define __BPF_HELPERS__ -#include <bpf_helper_defs.h> +#include <bpf/bpf_helper_defs.h> #define __uint(name, val) int (*name)[val] #define __type(name, val) typeof(val) *name
The change to use angled includes for bpf_helper_defs.h breaks compilation against libbpf when it is installed in the include path, since the file is installed in the bpf/ subdirectory of $INCLUDE_PATH. Fix this by adding the bpf/ prefix to the #include directive. Fixes: 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are taken from selftests dir") Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- Not actually sure this fix works for all the cases you originally tried to fix with the referred commit; please check. Also, could we please stop breaking libbpf builds? :) tools/lib/bpf/bpf_helpers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)