On Wed, Aug 29, 2018 at 4:37 PM Christian Ehrhardt <
christian.ehrha...@canonical.com> wrote:
>
>
> On Wed, Aug 29, 2018 at 3:16 PM Adrien Mazarguil <
> adrien.mazarg...@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.mazarg...@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++
>
I also made sure to only mess up exactly the affected case PPC64 without
__APPLE_ALTIVEC__ (no matter what -std or else made it disappear).
The minimal solution that works for now until we have maintainer feedback
would be this:
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..d0c57e28 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
@@ -37,6 +37,15 @@
#include <string.h>
/*To include altivec.h, GCC version must >= 4.8 */
#include <altivec.h>
+/*
+ * Compilation workaround for PPC64 targets when AltiVec is fully
+ * enabled e.g. with std=c11. Otherwise there would be a type conflict
+ * of "bool" between stdbool and altivec.
+ */
+#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
+ #undef bool
+ #define bool _Bool
+#endif
#ifdef __cplusplus
extern "C" {
I'll reply that as a proper patch for inclusion as an interim fix for now.
> 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
>
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
