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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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@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. 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.

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