[dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[pbhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Pmdinfogen is always compiled with host gcc.
If host gcc version is lessthan 7 and target gcc is greaterthan 7
pmdinfogen fails to compile due to unsupported cflags.
This patch removes unsupported host cflags when the above condition is
met.

Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
Cc: stable@dpdk.org

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 buildtools/pmdinfogen/Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
index a97a7648f..86f883e05 100644
--- a/buildtools/pmdinfogen/Makefile
+++ b/buildtools/pmdinfogen/Makefile
@@ -9,6 +9,14 @@ include $(RTE_SDK)/mk/rte.vars.mk
 #
 HOSTAPP = dpdk-pmdinfogen
 
+HOST_GCC_MAJOR = $(shell echo __GNUC__ | $(HOSTCC) -E -x c - | tail -n 1)
+HOST_GCC_MINOR = $(shell echo __GNUC_MINOR__ | $(HOSTCC) -E -x c - | tail -n 1)
+HOST_GCC_VERSION = $(HOST_GCC_MAJOR)$(HOST_GCC_MINOR)
+
+ifeq ($(shell test $(HOST_GCC_VERSION) -gt 70 && echo 1), 1)
+HOST_WERROR_FLAGS = $(filter-out -Wimplicit-fallthrough=2, $(WERROR_FLAGS))
+endif
+
 #
 # all sources are stored in SRCS-y
 #
-- 
2.17.1

Re: [dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[Neil Horman]

On Wed, Jul 31, 2019 at 11:57:05AM +0530, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Pmdinfogen is always compiled with host gcc.
> If host gcc version is lessthan 7 and target gcc is greaterthan 7
> pmdinfogen fails to compile due to unsupported cflags.
> This patch removes unsupported host cflags when the above condition is
> met.
> 
> Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  buildtools/pmdinfogen/Makefile | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
> index a97a7648f..86f883e05 100644
> --- a/buildtools/pmdinfogen/Makefile
> +++ b/buildtools/pmdinfogen/Makefile
> @@ -9,6 +9,14 @@ include $(RTE_SDK)/mk/rte.vars.mk
>  #
>  HOSTAPP = dpdk-pmdinfogen
>  
> +HOST_GCC_MAJOR = $(shell echo __GNUC__ | $(HOSTCC) -E -x c - | tail -n 1)
> +HOST_GCC_MINOR = $(shell echo __GNUC_MINOR__ | $(HOSTCC) -E -x c - | tail -n 1)
> +HOST_GCC_VERSION = $(HOST_GCC_MAJOR)$(HOST_GCC_MINOR)
> +
> +ifeq ($(shell test $(HOST_GCC_VERSION) -gt 70 && echo 1), 1)
> +HOST_WERROR_FLAGS = $(filter-out -Wimplicit-fallthrough=2, $(WERROR_FLAGS))
> +endif
> +

A few things:

1) HOST_GCC_MAJOR and HOST_GCC_MINOR seem to already be computed in
rte.toolchain-compat.mk and so I don't think you need to recompute them here

2) This seems limited in its function.  That is to say, ostensibly there are
simmilar incompatibilities with icc and clang that may need addressing for which
there are different environment variables (CLANG_MAJOR_VERSION, etc)

3) This may also need to be reflected into the meson build environment

4) I'm not sure how this is a problem at all.  In your description, you indicate
that:
	a) pmdinfogen is always compiled with host gcc
	b) this fails when the target gcc uses a compiler version older than the 
	   host gcc version, leading to corresponding flag incompatibilities
If (a) is true, why does (b) matter?  If we are using the host gcc, then the
host gcc flags should work.  If we're mixing and matching host gcc and target
gcc flags, that seems like a bug that should be fixed in the target rte.vars.mk

Neil

Re: [dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[Bruce Richardson]

On Wed, Jul 31, 2019 at 07:35:03AM -0400, Neil Horman wrote:
> On Wed, Jul 31, 2019 at 11:57:05AM +0530, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > 
> > Pmdinfogen is always compiled with host gcc.
> > If host gcc version is lessthan 7 and target gcc is greaterthan 7
> > pmdinfogen fails to compile due to unsupported cflags.
> > This patch removes unsupported host cflags when the above condition is
> > met.
> > 
> > Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> >  buildtools/pmdinfogen/Makefile | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
> > index a97a7648f..86f883e05 100644
> > --- a/buildtools/pmdinfogen/Makefile
> > +++ b/buildtools/pmdinfogen/Makefile
> > @@ -9,6 +9,14 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >  #
> >  HOSTAPP = dpdk-pmdinfogen
> >  
> > +HOST_GCC_MAJOR = $(shell echo __GNUC__ | $(HOSTCC) -E -x c - | tail -n 1)
> > +HOST_GCC_MINOR = $(shell echo __GNUC_MINOR__ | $(HOSTCC) -E -x c - | tail -n 1)
> > +HOST_GCC_VERSION = $(HOST_GCC_MAJOR)$(HOST_GCC_MINOR)
> > +
> > +ifeq ($(shell test $(HOST_GCC_VERSION) -gt 70 && echo 1), 1)
> > +HOST_WERROR_FLAGS = $(filter-out -Wimplicit-fallthrough=2, $(WERROR_FLAGS))
> > +endif
> > +
> 
> A few things:
> 
> 1) HOST_GCC_MAJOR and HOST_GCC_MINOR seem to already be computed in
> rte.toolchain-compat.mk and so I don't think you need to recompute them here
> 
> 2) This seems limited in its function.  That is to say, ostensibly there are
> simmilar incompatibilities with icc and clang that may need addressing for which
> there are different environment variables (CLANG_MAJOR_VERSION, etc)
> 
> 3) This may also need to be reflected into the meson build environment

Initially I though that meson cross-compilation support would make this a
non-issue, but looking into it further it could theoretically be a problem
with meson too. Where the issue would arise is where we use
"add_project_arguments()" in "config/meson.build" for the cflags. When
adding those flags we only check the standard compiler, which would be the
cross-compiler in the cross compilation case. Unfortunately, I don't
believe that meson supports setting project options only for native or
non-native compiles - it assumes that per-project arguments are really
global to that whole project, and expect you to specify them per-target if
not.

To fix this possible issue for cross-compilation, what we really need to do
in to get the native compiler too (using "meson.get_compiler('c', native:
'true')") and check that the cflags are valid for it also. In case of a
flag that is supported by one compiler but not the other, the flag would be
omitted, which means that instead of a compiler error we should instead get
an error with reduced warning flags. (I'm assuming that it's only the
warnings need adjusting here - any compiler that doesn't support
"-D<define>" syntax just won't work anyway, I suspect :-))

I'll see if I can do up a quick patch to fix this for meson.


> 
> 4) I'm not sure how this is a problem at all.  In your description, you indicate
> that:
> 	a) pmdinfogen is always compiled with host gcc
> 	b) this fails when the target gcc uses a compiler version older than the 
> 	   host gcc version, leading to corresponding flag incompatibilities
> If (a) is true, why does (b) matter?  If we are using the host gcc, then the
> host gcc flags should work.  If we're mixing and matching host gcc and target
> gcc flags, that seems like a bug that should be fixed in the target rte.vars.mk
> 
Yes, this is the issue - the mixing of host and target gcc flags.

/Bruce

Re: [dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[Bruce Richardson]

On Wed, Jul 31, 2019 at 03:21:46PM +0100, Bruce Richardson wrote:
> On Wed, Jul 31, 2019 at 07:35:03AM -0400, Neil Horman wrote:
> > On Wed, Jul 31, 2019 at 11:57:05AM +0530, pbhagavatula@marvell.com wrote:
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > 
> > > Pmdinfogen is always compiled with host gcc.
> > > If host gcc version is lessthan 7 and target gcc is greaterthan 7
> > > pmdinfogen fails to compile due to unsupported cflags.
> > > This patch removes unsupported host cflags when the above condition is
> > > met.
> > > 
> > > Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > ---
> > >  buildtools/pmdinfogen/Makefile | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
> > > index a97a7648f..86f883e05 100644
> > > --- a/buildtools/pmdinfogen/Makefile
> > > +++ b/buildtools/pmdinfogen/Makefile
> > > @@ -9,6 +9,14 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > >  #
> > >  HOSTAPP = dpdk-pmdinfogen
> > >  
> > > +HOST_GCC_MAJOR = $(shell echo __GNUC__ | $(HOSTCC) -E -x c - | tail -n 1)
> > > +HOST_GCC_MINOR = $(shell echo __GNUC_MINOR__ | $(HOSTCC) -E -x c - | tail -n 1)
> > > +HOST_GCC_VERSION = $(HOST_GCC_MAJOR)$(HOST_GCC_MINOR)
> > > +
> > > +ifeq ($(shell test $(HOST_GCC_VERSION) -gt 70 && echo 1), 1)
> > > +HOST_WERROR_FLAGS = $(filter-out -Wimplicit-fallthrough=2, $(WERROR_FLAGS))
> > > +endif
> > > +
> > 
> > A few things:
> > 
> > 1) HOST_GCC_MAJOR and HOST_GCC_MINOR seem to already be computed in
> > rte.toolchain-compat.mk and so I don't think you need to recompute them here
> > 
> > 2) This seems limited in its function.  That is to say, ostensibly there are
> > simmilar incompatibilities with icc and clang that may need addressing for which
> > there are different environment variables (CLANG_MAJOR_VERSION, etc)
> > 
> > 3) This may also need to be reflected into the meson build environment
> 
> Initially I though that meson cross-compilation support would make this a
> non-issue, but looking into it further it could theoretically be a problem
> with meson too. Where the issue would arise is where we use
> "add_project_arguments()" in "config/meson.build" for the cflags. When
> adding those flags we only check the standard compiler, which would be the
> cross-compiler in the cross compilation case. Unfortunately, I don't
> believe that meson supports setting project options only for native or
> non-native compiles - it assumes that per-project arguments are really
> global to that whole project, and expect you to specify them per-target if
> not.
> 
> To fix this possible issue for cross-compilation, what we really need to do
> in to get the native compiler too (using "meson.get_compiler('c', native:
> 'true')") and check that the cflags are valid for it also. In case of a
> flag that is supported by one compiler but not the other, the flag would be
> omitted, which means that instead of a compiler error we should instead get
> an error with reduced warning flags. (I'm assuming that it's only the
> warnings need adjusting here - any compiler that doesn't support
> "-D<define>" syntax just won't work anyway, I suspect :-))
> 
> I'll see if I can do up a quick patch to fix this for meson.
> 
Actually, looks like I sent the reply too quickly and I may be incorrect here.

Checking the build.ninja file for a cross-build for arm64-armv8, I already
see different flags for pmdinfogen and other objects, so looks to me like no
issue after all. [This is with meson v0.51].

/Bruce

Re: [dpdk-dev] [PATCH] buildtools: fix pmdinfogen compilation

[Bruce Richardson]

On Wed, Jul 31, 2019 at 11:57:05AM +0530, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Pmdinfogen is always compiled with host gcc.
> If host gcc version is lessthan 7 and target gcc is greaterthan 7
> pmdinfogen fails to compile due to unsupported cflags.
> This patch removes unsupported host cflags when the above condition is
> met.
> 
> Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---

Can you perhaps check with a meson cross-compile and see if you encounter
the same issue? If so, I'll investigate.

Thanks,
/Bruce

Re: [dpdk-dev] [EXT] Re: [PATCH] buildtools: fix pmdinfogen compilation

[Pavan Nikhilesh Bhagavatula]

Hi Bruce, Neil,

We had this patch in our internal tree that was intended to fix  -Wimplicit-fallthrough related error due to Host and Target GCC mismatch.
But I just verified both combination of GCC in Make and Meson seems like it has already been addressed somewhere down the line.
So, I'm dropping the patch.

Regards,
Pavan.

>-----Original Message-----
>From: Bruce Richardson <bruce.richardson@intel.com>
>Sent: Wednesday, July 31, 2019 8:02 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Neil Horman
><nhorman@tuxdriver.com>; dev@dpdk.org; stable@dpdk.org
>Subject: [EXT] Re: [dpdk-dev] [PATCH] buildtools: fix pmdinfogen
>compilation
>
>External Email
>
>----------------------------------------------------------------------
>On Wed, Jul 31, 2019 at 11:57:05AM +0530, pbhagavatula@marvell.com
>wrote:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Pmdinfogen is always compiled with host gcc.
>> If host gcc version is lessthan 7 and target gcc is greaterthan 7
>> pmdinfogen fails to compile due to unsupported cflags.
>> This patch removes unsupported host cflags when the above
>condition is
>> met.
>>
>> Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen
>utility")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> ---
>
>Can you perhaps check with a meson cross-compile and see if you
>encounter
>the same issue? If so, I'll investigate.
>
>Thanks,
>/Bruce