[v3,bpf-next,09/14] samples: bpf: makefile: use own flags but not host when cross compile
diff mbox series

Message ID 20190916105433.11404-10-ivan.khoronzhuk@linaro.org
State Superseded
Headers show
Series
  • samples: bpf: improve/fix cross-compilation
Related show

Commit Message

Ivan Khoronzhuk Sept. 16, 2019, 10:54 a.m. UTC
While compile natively, the hosts cflags and ldflags are equal to ones
used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
have own, used for target arch. While verification, for arm, arm64 and
x86_64 the following flags were used alsways:

-Wall
-O2
-fomit-frame-pointer
-Wmissing-prototypes
-Wstrict-prototypes

So, add them as they were verified and used before adding
Makefile.target, but anyway limit it only for cross compile options as
for host can be some configurations when another options can be used,
So, for host arch samples left all as is, it allows to avoid potential
option mistmatches for existent environments.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 samples/bpf/Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Sergei Shtylyov Sept. 17, 2019, 10:14 a.m. UTC | #1
Hello!

On 16.09.2019 13:54, Ivan Khoronzhuk wrote:

> While compile natively, the hosts cflags and ldflags are equal to ones

   Compiling. Host's.

> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> have own, used for target arch. While verification, for arm, arm64 and
> x86_64 the following flags were used alsways:
> 
> -Wall
> -O2
> -fomit-frame-pointer
> -Wmissing-prototypes
> -Wstrict-prototypes
> 
> So, add them as they were verified and used before adding
> Makefile.target, but anyway limit it only for cross compile options as
> for host can be some configurations when another options can be used,
> So, for host arch samples left all as is, it allows to avoid potential
> option mistmatches for existent environments.

    Mismatches.

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
[...]

MBR, Sergei
Andrii Nakryiko Sept. 17, 2019, 11:42 p.m. UTC | #2
On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> While compile natively, the hosts cflags and ldflags are equal to ones
> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> have own, used for target arch. While verification, for arm, arm64 and
> x86_64 the following flags were used alsways:
>
> -Wall
> -O2
> -fomit-frame-pointer
> -Wmissing-prototypes
> -Wstrict-prototypes
>
> So, add them as they were verified and used before adding
> Makefile.target, but anyway limit it only for cross compile options as
> for host can be some configurations when another options can be used,
> So, for host arch samples left all as is, it allows to avoid potential
> option mistmatches for existent environments.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  samples/bpf/Makefile | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 1579cc16a1c2..b5c87a8b8b51 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
>  TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
>  endif
>
> +ifdef CROSS_COMPILE
> +TPROGS_CFLAGS += -Wall
> +TPROGS_CFLAGS += -O2

Specifying one arg per line seems like overkill, put them in one line?

> +TPROGS_CFLAGS += -fomit-frame-pointer

Why this one?

> +TPROGS_CFLAGS += -Wmissing-prototypes
> +TPROGS_CFLAGS += -Wstrict-prototypes

Are these in some way special that we want them in cross-compile mode only?

All of those flags seem useful regardless of cross-compilation or not,
shouldn't they be common? I'm a bit lost about the intent here...

> +else
>  TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
>  TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> +endif
> +
>  TPROGS_CFLAGS += -I$(objtree)/usr/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> --
> 2.17.1
>
Ivan Khoronzhuk Sept. 18, 2019, 10:35 a.m. UTC | #3
On Tue, Sep 17, 2019 at 04:42:07PM -0700, Andrii Nakryiko wrote:
>On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
><ivan.khoronzhuk@linaro.org> wrote:
>>
>> While compile natively, the hosts cflags and ldflags are equal to ones
>> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
>> have own, used for target arch. While verification, for arm, arm64 and
>> x86_64 the following flags were used alsways:
>>
>> -Wall
>> -O2
>> -fomit-frame-pointer
>> -Wmissing-prototypes
>> -Wstrict-prototypes
>>
>> So, add them as they were verified and used before adding
>> Makefile.target, but anyway limit it only for cross compile options as
>> for host can be some configurations when another options can be used,
>> So, for host arch samples left all as is, it allows to avoid potential
>> option mistmatches for existent environments.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>  samples/bpf/Makefile | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
>> index 1579cc16a1c2..b5c87a8b8b51 100644
>> --- a/samples/bpf/Makefile
>> +++ b/samples/bpf/Makefile
>> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
>>  TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
>>  endif
>>
>> +ifdef CROSS_COMPILE
>> +TPROGS_CFLAGS += -Wall
>> +TPROGS_CFLAGS += -O2
>
>Specifying one arg per line seems like overkill, put them in one line?
Will combine.

>
>> +TPROGS_CFLAGS += -fomit-frame-pointer
>
>Why this one?
I've explained in commit msg. The logic is to have as much as close options
to have smiliar binaries. As those options are used before for hosts and kinda
cross builds - better follow same way.

>
>> +TPROGS_CFLAGS += -Wmissing-prototypes
>> +TPROGS_CFLAGS += -Wstrict-prototypes
>
>Are these in some way special that we want them in cross-compile mode only?
>
>All of those flags seem useful regardless of cross-compilation or not,
>shouldn't they be common? I'm a bit lost about the intent here...
They are common but split is needed to expose it at least. Also host for
different arches can have some own opts already used that shouldn't be present
for cross, better not mix it for safety.

>
>> +else
>>  TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
>>  TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
>> +endif
>> +
>>  TPROGS_CFLAGS += -I$(objtree)/usr/include
>>  TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
>>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
>> --
>> 2.17.1
>>
Andrii Nakryiko Sept. 18, 2019, 9:29 p.m. UTC | #4
On Wed, Sep 18, 2019 at 3:35 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> On Tue, Sep 17, 2019 at 04:42:07PM -0700, Andrii Nakryiko wrote:
> >On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
> ><ivan.khoronzhuk@linaro.org> wrote:
> >>
> >> While compile natively, the hosts cflags and ldflags are equal to ones
> >> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> >> have own, used for target arch. While verification, for arm, arm64 and
> >> x86_64 the following flags were used alsways:
> >>
> >> -Wall
> >> -O2
> >> -fomit-frame-pointer
> >> -Wmissing-prototypes
> >> -Wstrict-prototypes
> >>
> >> So, add them as they were verified and used before adding
> >> Makefile.target, but anyway limit it only for cross compile options as
> >> for host can be some configurations when another options can be used,
> >> So, for host arch samples left all as is, it allows to avoid potential
> >> option mistmatches for existent environments.
> >>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> ---
> >>  samples/bpf/Makefile | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> >> index 1579cc16a1c2..b5c87a8b8b51 100644
> >> --- a/samples/bpf/Makefile
> >> +++ b/samples/bpf/Makefile
> >> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
> >>  TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> >>  endif
> >>
> >> +ifdef CROSS_COMPILE
> >> +TPROGS_CFLAGS += -Wall
> >> +TPROGS_CFLAGS += -O2
> >
> >Specifying one arg per line seems like overkill, put them in one line?
> Will combine.
>
> >
> >> +TPROGS_CFLAGS += -fomit-frame-pointer
> >
> >Why this one?
> I've explained in commit msg. The logic is to have as much as close options
> to have smiliar binaries. As those options are used before for hosts and kinda
> cross builds - better follow same way.

I'm just asking why omit frame pointers and make it harder to do stuff
like profiling? What performance benefits are we seeking for in BPF
samples?

>
> >
> >> +TPROGS_CFLAGS += -Wmissing-prototypes
> >> +TPROGS_CFLAGS += -Wstrict-prototypes
> >
> >Are these in some way special that we want them in cross-compile mode only?
> >
> >All of those flags seem useful regardless of cross-compilation or not,
> >shouldn't they be common? I'm a bit lost about the intent here...
> They are common but split is needed to expose it at least. Also host for
> different arches can have some own opts already used that shouldn't be present
> for cross, better not mix it for safety.

We want -Wmissing-prototypes and -Wstrict-prototypes for cross-compile
and non-cross-compile cases, right? So let's specify them as common
set of options, instead of relying on KBUILD_HOSTCFLAGS or
HOST_EXTRACFLAGS to have them. Otherwise we'll be getting extra
warnings for just cross-compile case, which is not good. If you are
worrying about having duplicate -W flags, seems like it's handled by
GCC already, so shouldn't be a problem.

>
> >
> >> +else
> >>  TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
> >>  TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> >> +endif
> >> +
> >>  TPROGS_CFLAGS += -I$(objtree)/usr/include
> >>  TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
> >>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> >> --
> >> 2.17.1
> >>
>
> --
> Regards,
> Ivan Khoronzhuk
Ivan Khoronzhuk Sept. 19, 2019, 2:18 p.m. UTC | #5
On Wed, Sep 18, 2019 at 02:29:53PM -0700, Andrii Nakryiko wrote:
>On Wed, Sep 18, 2019 at 3:35 AM Ivan Khoronzhuk
><ivan.khoronzhuk@linaro.org> wrote:
>>
>> On Tue, Sep 17, 2019 at 04:42:07PM -0700, Andrii Nakryiko wrote:
>> >On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
>> ><ivan.khoronzhuk@linaro.org> wrote:
>> >>
>> >> While compile natively, the hosts cflags and ldflags are equal to ones
>> >> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
>> >> have own, used for target arch. While verification, for arm, arm64 and
>> >> x86_64 the following flags were used alsways:
>> >>
>> >> -Wall
>> >> -O2
>> >> -fomit-frame-pointer
>> >> -Wmissing-prototypes
>> >> -Wstrict-prototypes
>> >>
>> >> So, add them as they were verified and used before adding
>> >> Makefile.target, but anyway limit it only for cross compile options as
>> >> for host can be some configurations when another options can be used,
>> >> So, for host arch samples left all as is, it allows to avoid potential
>> >> option mistmatches for existent environments.
>> >>
>> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> ---
>> >>  samples/bpf/Makefile | 9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >>
>> >> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
>> >> index 1579cc16a1c2..b5c87a8b8b51 100644
>> >> --- a/samples/bpf/Makefile
>> >> +++ b/samples/bpf/Makefile
>> >> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
>> >>  TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
>> >>  endif
>> >>
>> >> +ifdef CROSS_COMPILE
>> >> +TPROGS_CFLAGS += -Wall
>> >> +TPROGS_CFLAGS += -O2
>> >
>> >Specifying one arg per line seems like overkill, put them in one line?
>> Will combine.
>>
>> >
>> >> +TPROGS_CFLAGS += -fomit-frame-pointer
>> >
>> >Why this one?
>> I've explained in commit msg. The logic is to have as much as close options
>> to have smiliar binaries. As those options are used before for hosts and kinda
>> cross builds - better follow same way.
>
>I'm just asking why omit frame pointers and make it harder to do stuff
>like profiling? What performance benefits are we seeking for in BPF
>samples?
>
>>
>> >
>> >> +TPROGS_CFLAGS += -Wmissing-prototypes
>> >> +TPROGS_CFLAGS += -Wstrict-prototypes
>> >
>> >Are these in some way special that we want them in cross-compile mode only?
>> >
>> >All of those flags seem useful regardless of cross-compilation or not,
>> >shouldn't they be common? I'm a bit lost about the intent here...
>> They are common but split is needed to expose it at least. Also host for
>> different arches can have some own opts already used that shouldn't be present
>> for cross, better not mix it for safety.
>
>We want -Wmissing-prototypes and -Wstrict-prototypes for cross-compile
>and non-cross-compile cases, right? So let's specify them as common
>set of options, instead of relying on KBUILD_HOSTCFLAGS or
>HOST_EXTRACFLAGS to have them. Otherwise we'll be getting extra
>warnings for just cross-compile case, which is not good. If you are
>worrying about having duplicate -W flags, seems like it's handled by
>GCC already, so shouldn't be a problem.

Ok, lets drop omit-frame-pointer.

But then, lets do more radical step and drop
KBUILD_HOSTCFLAGS & HOST_EXTRACFLAG in this patch:

-ifdef CROSS_COMPILE
+TPROGS_CFLAGS += -Wall -O2
+TPROGS_CFLAGS += -Wmissing-prototypes
+TPROGS_CFLAGS += -Wstrict-prototypes
-else
-TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
-TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
-endif

At least it allows to use same options always for both, native and cross.

I verified on native x86_64, arm64 and arm and cross for arm and arm64,
but should work for others, at least it can be tuned explicitly and
no need to depend on KBUILD and use "cross" fork here.
Andrii Nakryiko Sept. 19, 2019, 5:54 p.m. UTC | #6
On Thu, Sep 19, 2019 at 7:18 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> On Wed, Sep 18, 2019 at 02:29:53PM -0700, Andrii Nakryiko wrote:
> >On Wed, Sep 18, 2019 at 3:35 AM Ivan Khoronzhuk
> ><ivan.khoronzhuk@linaro.org> wrote:
> >>
> >> On Tue, Sep 17, 2019 at 04:42:07PM -0700, Andrii Nakryiko wrote:
> >> >On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
> >> ><ivan.khoronzhuk@linaro.org> wrote:
> >> >>
> >> >> While compile natively, the hosts cflags and ldflags are equal to ones
> >> >> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> >> >> have own, used for target arch. While verification, for arm, arm64 and
> >> >> x86_64 the following flags were used alsways:
> >> >>
> >> >> -Wall
> >> >> -O2
> >> >> -fomit-frame-pointer
> >> >> -Wmissing-prototypes
> >> >> -Wstrict-prototypes
> >> >>
> >> >> So, add them as they were verified and used before adding
> >> >> Makefile.target, but anyway limit it only for cross compile options as
> >> >> for host can be some configurations when another options can be used,
> >> >> So, for host arch samples left all as is, it allows to avoid potential
> >> >> option mistmatches for existent environments.
> >> >>
> >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> ---
> >> >>  samples/bpf/Makefile | 9 +++++++++
> >> >>  1 file changed, 9 insertions(+)
> >> >>
> >> >> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> >> >> index 1579cc16a1c2..b5c87a8b8b51 100644
> >> >> --- a/samples/bpf/Makefile
> >> >> +++ b/samples/bpf/Makefile
> >> >> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
> >> >>  TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> >> >>  endif
> >> >>
> >> >> +ifdef CROSS_COMPILE
> >> >> +TPROGS_CFLAGS += -Wall
> >> >> +TPROGS_CFLAGS += -O2
> >> >
> >> >Specifying one arg per line seems like overkill, put them in one line?
> >> Will combine.
> >>
> >> >
> >> >> +TPROGS_CFLAGS += -fomit-frame-pointer
> >> >
> >> >Why this one?
> >> I've explained in commit msg. The logic is to have as much as close options
> >> to have smiliar binaries. As those options are used before for hosts and kinda
> >> cross builds - better follow same way.
> >
> >I'm just asking why omit frame pointers and make it harder to do stuff
> >like profiling? What performance benefits are we seeking for in BPF
> >samples?
> >
> >>
> >> >
> >> >> +TPROGS_CFLAGS += -Wmissing-prototypes
> >> >> +TPROGS_CFLAGS += -Wstrict-prototypes
> >> >
> >> >Are these in some way special that we want them in cross-compile mode only?
> >> >
> >> >All of those flags seem useful regardless of cross-compilation or not,
> >> >shouldn't they be common? I'm a bit lost about the intent here...
> >> They are common but split is needed to expose it at least. Also host for
> >> different arches can have some own opts already used that shouldn't be present
> >> for cross, better not mix it for safety.
> >
> >We want -Wmissing-prototypes and -Wstrict-prototypes for cross-compile
> >and non-cross-compile cases, right? So let's specify them as common
> >set of options, instead of relying on KBUILD_HOSTCFLAGS or
> >HOST_EXTRACFLAGS to have them. Otherwise we'll be getting extra
> >warnings for just cross-compile case, which is not good. If you are
> >worrying about having duplicate -W flags, seems like it's handled by
> >GCC already, so shouldn't be a problem.
>
> Ok, lets drop omit-frame-pointer.
>
> But then, lets do more radical step and drop
> KBUILD_HOSTCFLAGS & HOST_EXTRACFLAG in this patch:

Yeah, let's do this, if you confirmed that everything still works (and
I don't see a reason why it shouldn't). Thanks.

>
> -ifdef CROSS_COMPILE
> +TPROGS_CFLAGS += -Wall -O2
> +TPROGS_CFLAGS += -Wmissing-prototypes
> +TPROGS_CFLAGS += -Wstrict-prototypes
> -else
> -TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
> -TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> -endif
>
> At least it allows to use same options always for both, native and cross.
>
> I verified on native x86_64, arm64 and arm and cross for arm and arm64,
> but should work for others, at least it can be tuned explicitly and
> no need to depend on KBUILD and use "cross" fork here.

Yep, I like it.

>
> --
> Regards,
> Ivan Khoronzhuk

Patch
diff mbox series

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 1579cc16a1c2..b5c87a8b8b51 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -178,8 +178,17 @@  CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
 TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
 endif
 
+ifdef CROSS_COMPILE
+TPROGS_CFLAGS += -Wall
+TPROGS_CFLAGS += -O2
+TPROGS_CFLAGS += -fomit-frame-pointer
+TPROGS_CFLAGS += -Wmissing-prototypes
+TPROGS_CFLAGS += -Wstrict-prototypes
+else
 TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
 TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
+endif
+
 TPROGS_CFLAGS += -I$(objtree)/usr/include
 TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
 TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/