[dpdk-dev] 18.08 build error on ppc64el - bool as vector type

Christian Ehrhardt christian.ehrhardt at canonical.com
Wed Aug 29 16:37:28 CEST 2018
Previous message: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type
Next message: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On Wed, Aug 29, 2018 at 3:16 PM Adrien Mazarguil <adrien.mazarguil at 6wind.com>
wrote:

> On Wed, Aug 29, 2018 at 10:27:03AM +0200, Christian Ehrhardt wrote:
> > On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil <
> adrien.mazarguil at 6wind.com>
> > wrote:
> >
> > > On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
> <trimming thread a bit>
> > > > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > > > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > > > @@ -36,6 +36,14 @@
> > > > #include <stdint.h>
> > > > #include <string.h>
> > > > /*To include altivec.h, GCC version must  >= 4.8 */
> > > > +/*
> > > > + * If built with std=c11 stdbool and altivec bool will conflict.
> > > > + * The altivec bool type is not needed at the moment, to avoid the
> > > conflict
> > > > + * define __APPLE_ALTIVEC__ so that the conflict will not happen.
> > > > + */
> > > > +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L &&
> > > > !defined(__APPLE_ALTIVEC__)
> > > > +#define __APPLE_ALTIVEC__
> > > > +#endif
> > > > #include <altivec.h>
> > > >
> > > > #ifdef __cplusplus
> > > >
> > > > But it turned out we are not allowed to switch of other things as
> vector
> > > > (and probably some more code than the type) is actually used:
> > > > With your suggestion or mine above it will break on:
> > > >
> > > > x5.o -c /home/ubuntu/deb_dpdk/drivers/net/mlx5/mlx5.c
> > > > In file included from
> > > /home/ubuntu/deb_dpdk/drivers/net/mlx5/mlx5_prm.h:21,
> > > >                 from
> > > /home/ubuntu/deb_dpdk/drivers/net/mlx5/mlx5_rxtx.h:37,
> > > >                 from
> /home/ubuntu/deb_dpdk/drivers/net/mlx5/mlx5.h:36,
> > > >                 from
> /home/ubuntu/deb_dpdk/drivers/net/mlx5/mlx5.c:42:
> > > >
> /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_vect.h:43:15:
> > > error:
> > > > expected ‘;’ before ‘signed’
> > > > typedef vector signed int xmm_t;
> > > >               ^~~~~~~
> > > >               ;
> > > >
> /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_vect.h:49:2:
> > > error:
> > > > expected specifier-qualifier-list before ‘xmm_t’
> > > >  xmm_t    x;
> > > >  ^~~~~
> > > >
> > > > I have no much better suggestion for the ordering issue that you
> raised.
> > > > To test what would happen I moved the stdbool include after all other
> > > > includes in drivers/net/mlx5/mlx5_nl.c
> > > > I also moved mlx5.h (which eventually brings in altivec) right at the
> > > top.
> > > > This works to build, but such a check is always subtle as one of the
> > > other
> > > > includes might have pulled in stdbool before altivec still.
> > > > For a bit of confidence I picked said gcc call and ran it with -E.
> > > > The output suggests altivec really was included before stdbool.
> > >
> > > How about making altivec.h users (rte_vect.h and rte_memcpy.h) rely on
> > > "__vector" directly instead of the "vector" macro to make it
> transparent
> > > for
> > > others then?
> > >
> > > I think we can assume they have internal knowledge of this file in
> order to
> > > deal with __APPLE_ALTIVEC__ anyway.
> > >
> >
> > While "pushing the internal knowledge out to users" sounds right at
> first.
> > There are far too many IMHO, the change would be huge unclean and messy.
> >
> > $ grep -Hrn altivec.h
> > drivers/net/i40e/i40e_rxtx_vec_altivec.c:45:#include <altivec.h>
> > examples/l3fwd/l3fwd_lpm.c:165:#include "l3fwd_lpm_altivec.h"
> > examples/l3fwd/l3fwd_lpm_altivec.h:10:#include "l3fwd_altivec.h"
> > MAINTAINERS:239:F: examples/l3fwd/*altivec.h
> > lib/librte_acl/acl_run_altivec.c:34:#include "acl_run_altivec.h"
> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:49:/*To include
> > altivec.h, GCC version must  >= 4.8 */
> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:50:#include <
> > altivec.h>
> > lib/librte_eal/common/include/arch/ppc_64/rte_vect.h:36:#include
> <altivec.h>
> >
> > lib/librte_lpm/meson.build:9:headers += files('rte_lpm_altivec.h',
> > 'rte_lpm_neon.h', 'rte_lpm_sse.h')
> > lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=
> > rte_lpm_altivec.h
> > lib/librte_lpm/rte_lpm.h:461:#include "rte_lpm_altivec.h"
>
> I'd still like to give it a try given only knwon users of AltiVec code may
> rely on these vector/pixel/bool definitions. Scope should be quite small.
>
> The root issue we need to address is that DPDK applications may
> involuntarily pull altivec.h by including something unrelated
> (rte_memcpy.h)
> and get unwanted bool/vector/pixel macros polluting their namespace and
> breaking things.
>
> > > Also I would suggest not to make this workaround C11-only. I suspect
> the
> > > same issue will be encountered with -std=c99 or -std=c90. Keep in mind
> DPDK
> > > applications are free to specify their own CFLAGS.
> > >
> >
> > Yeah Independent to the other part of the discussion I think we can make
> it
> > generally apply and not just C11.
> >
> > The following "would work" in the code right now.
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > @@ -35,6 +35,21 @@
> >
> > #include <stdint.h>
> > #include <string.h>
> > +/*
> > + * if built with newer C standard like -std=c11 stdbool.h bool and
> altivec
> > + * bool types will conflict. We have to force altivec users (rte_vect.h
> and
> > + * rte_memcpy.h) rely on __vector implying internal altivec knowledge to
> > the
> > + * users but keeping things transparent for all others.
> > + * Therefore define __APPLE_ALTIVEC__ which make it not (re-)define the
> > + * non prefixed types lile "bool".
> > + * While we need to disambiguise bool, we want vector/pixel to stay
> as-is
> > + * in those cases so define them as altivec.h would.
> > + */
> > +#ifndef __APPLE_ALTIVEC__
> > +#define __APPLE_ALTIVEC__ 1
> > +#define vector __vector
> > +#define pixel __pixel
> > +#endif
> > /*To include altivec.h, GCC version must  >= 4.8 */
> > #include <altivec.h>
> >
> > But here again, ordering could make altivec.h be included before this
> > statement in rte_memcpy.
>
> Another possible issue: Clang's altivec.h differs from that of GCC.
>
> It doesn't have __APPLE_ALTIVEC__ and doesn't define macros for
> bool/vector/pixel, which is good as they only exist as context-aware
> compiler keywords with -maltivec, however I don't see instances of __pixel
> or __bool beside __vector in that file. This should be carefully tested to
> make sure the __ prefix is supported by both compilers.
>
> > I would not want to see us search and replace all occurrence of "vector"
> > nor doing the same trick all over the place at every include of altivec.h
> > How about the following which should address the follwing:
> > - resolve the issue with stbool conflicting
> > - no issues with vector types as it retains what would be defined
> > - have the workaround in just one place
> > - independent to include order as long as rte_altivec.h is uses instead
> of
> > a direct include
>
> I like rte_altivec.h. It's explicit and easy to make sure it remains the
> only user of altivec.h in DPDK. However due to the remaining issues with
> these macros, I still believe we should address them all at once.
>
> So let's take a slighly different approach. Assuming users of altivec.h
> know
> what they are doing, stdbool.h and altivec.h conflicts and the compiler
> flags they use is their problem. We only need to help those that didn't ask
> for altivec.h and get infected by it through header dependencies.
>
> Normally, -maltivec is all that's needed to get harmless bool/vector/pixel
> context-sensitive keywords (even without including altivec.h) as expected
> by
> applications, however no one ever expects these to be harmful macros as is
> the case when compiling with GCC in ISO C mode.
>
> In short, public headers that include altivec.h need to clean the mess
> after
> themselves transparently. So here's a different suggestion for
> rte_altivec.h:
>
> ///
>
>  #ifndef RTE_ALTIVEC_H_
>  #define RTE_ALTIVEC_H_
>
>  #ifdef bool
>  #define RTE_ALTIVEC_H_ORIG_BOOL bool
>  #undef bool
>  #endif
>
>  #ifdef vector
>  #define RTE_ALTIVEC_H_ORIG_VECTOR vector
>  #undef vector
>  #endif
>
>  #ifdef pixel
>  #define RTE_ALTIVEC_H_ORIG_PIXEL pixel
>  #undef pixel
>  #endif
>
>  #include <altivec.h>
>
>  #undef bool
>  #undef vector
>  #undef pixel
>
>  #ifdef RTE_ALTIVEC_H_ORIG_BOOL
>  #define bool RTE_ALTIVEC_H_ORIG_BOOL
>  #undef RTE_ALTIVEC_H_ORIG_BOOL
>  #endif
>
>  #ifdef RTE_ALTIVEC_H_ORIG_VECTOR
>  #define vector RTE_ALTIVEC_H_ORIG_VECTOR
>  #undef RTE_ALTIVEC_H_ORIG_VECTOR
>  #endif
>
>  #ifdef RTE_ALTIVEC_H_ORIG_PIXEL
>  #define pixel RTE_ALTIVEC_H_ORIG_PIXEL
>  #undef RTE_ALTIVEC_H_ORIG_PIXEL
>
> ///
>
> With public headers such as rte_vect.h and rte_memcpy.h modified to use
> rte_altivec.h and rely on __vector and __bool where relevant. Applications
> can continue to rely on altivec.h and non-prefixed counterparts as usual.
>
> That way, applications that absolutely want to combine ISO C and altivec.h
> yet still get bool/vector/pixel macros only have to make sure it's included
> before any DPDK headers. This shouldn't be a problem for the vast majority.
>
> What's your opinion?
>
> Now given the size of this mess, I'm starting to think that the quick &
> dirty workaround in mlx5 doesn't look that bad after all.


Yes, we do not want to (re-)invent anything here.
And by our engineering habits I guess we already have started more than we
should.
More on generic thoughts below ..


> Files that rely on
> stdbool.h only need something like this after #include directives:
>
>  /* Compilation workaround for PPC targets when AltiVec is enabled. */
>  #undef bool
>  #define bool _Bool
>
> I'm fine with that if it's OK for you.
>

Yeah I'd be fine with something like that as well.
I'll tomorrow try to come up with a minimal version that is proven to build
there based on the suggestion.
Just no time for it today.

I'll add a "heavily-discussed-by:" tag for you - thanks++


On the engineering of messy huge solutions by two people not really
responsible for it :-), something else came to my mind.
Why is no-one of IBM/Power world replying at all?
There not even was a "oh yeah, <whatever the content would be>" mail by
them.
Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
This is something that mid term has to be sorted out - I tried to pull some
strings to get attention "there" but so far I'm waiting for a reply.
I'd say 18.11 should not be released with a clear re-confirmation of ppc64
maintainance ppc64 by "someone".


A few unrelated minor comments about your patch below.
>
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > index f3fc8267..31dc6839 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > @@ -42,7 +42,7 @@
> > #include "i40e_rxtx.h"
> > #include "i40e_rxtx_vec_common.h"
> >
> > -#include <altivec.h>
> > +#include <rte_altivec.h>
> >
> > #pragma GCC diagnostic ignored "-Wcast-qual"
> >
> > diff --git a/lib/librte_eal/common/Makefile
> b/lib/librte_eal/common/Makefile
> > index cca68826..cab13f1d 100644
> > --- a/lib/librte_eal/common/Makefile
> > +++ b/lib/librte_eal/common/Makefile
> > @@ -17,6 +17,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h
> > INC += rte_service.h rte_service_component.h
> > INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
> > INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
> > +INC += rte_altivec.h
> >
> > GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
> > GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
> > diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > index 75f74897..225de7a0 100644
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > @@ -35,8 +35,8 @@
> >
> > #include <stdint.h>
> > #include <string.h>
> > -/*To include altivec.h, GCC version must  >= 4.8 */
> > -#include <altivec.h>
> > +
> > +#include <rte_altivec.h>
> >
> > #ifdef __cplusplus
> > extern "C" {
> > diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > index 99586e58..1ac81d73 100644
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > @@ -33,7 +33,7 @@
> > #ifndef _RTE_VECT_PPC_64_H_
> > #define _RTE_VECT_PPC_64_H_
> >
> > -#include <altivec.h>
> > +#include <rte_altivec.h>
> > #include "generic/rte_vect.h"
> >
> > #ifdef __cplusplus
> > diff --git a/lib/librte_eal/common/include/rte_altivec.h
> > b/lib/librte_eal/common/include/rte_altivec.h
> > new file mode 100644
> > index 00000000..e620e701
> > --- /dev/null
> > +++ b/lib/librte_eal/common/include/rte_altivec.h
> > @@ -0,0 +1,60 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright 2018 Canonical Ltd.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of NXP nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > + */
>
> Make sure to use the SPDX format for the copyright notice, see other
> headers.
>
> > +#ifndef _RTE_EAL_ALTIVEC_H_
> > +#define _RTE_EAL_ALTIVEC_H_
> > +
> > +/*
> > + * if built with newer C standard like -std=c11 stdbool.h bool and
> altivec
> > + * bool types will conflict.
> > + * There is no user of the altivec bool type, so make sure it never is
> > + * defined. Therefore define __APPLE_ALTIVEC__ which make it not
> > (re-)define
> > + * the non prefixed types lile "bool".
> > + * On the other hand other types like vector are used, so while we need
> to
> > + * disambiguise type bool, we want vector/pixel to stay as-is so we
> define
> > + * them as altivec.h would.
> > + * Note: without -std= the compiler has all those as context sensitive
> > + * keywords and the header will not define them at all. Therefore the
> > + * compiler has __APPLE_ALTIVEC__ already set in those cases -
> therefore we
> > + * don't touch things if __APPLE_ALTIVEC__ is already set.
> > + */
> > +#ifndef __APPLE_ALTIVEC__
> > +#define __APPLE_ALTIVEC__ 1
> > +#define vector __vector
> > +#define pixel __pixel
> > +#endif
> > +
> > +/*To include altivec.h, GCC version must  >= 4.8 */
>
> This comment can probably be dropped given Clang also ships altivec.h and
> GCC <= 4.8 is not actively supported anymore (sys_reqs.rst).
>
> > +#include <altivec.h>
> > +
> > +#endif /* _RTE_EAL_ALTIVEC_H_ */
> > diff --git a/lib/librte_eal/common/meson.build
> > b/lib/librte_eal/common/meson.build
> > index 56005bea..616f2084 100644
> > --- a/lib/librte_eal/common/meson.build
> > +++ b/lib/librte_eal/common/meson.build
> > @@ -45,6 +45,7 @@ common_objs += eal_common_arch_objs
> >
> > common_headers = files(
> >        'include/rte_alarm.h',
> > +       'include/rte_altivec.h',
> >        'include/rte_branch_prediction.h',
> >        'include/rte_bus.h',
> >        'include/rte_bitmap.h',
>
> --
> Adrien Mazarguil
> 6WIND
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
Previous message: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type
Next message: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list