Re: [PATCH v3 bpf-next 09/14] samples: bpf: makefile: use own flags but not host when cross compile

[Andrii Nakryiko]

On Thu, Sep 19, 2019 at 7:18 AM Ivan Khoronzhuk
<ivan.khoronzhuk@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> >> ---
> >> >>  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