From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by dpdk.org (Postfix) with ESMTP id 1C4E02B92 for ; Wed, 29 Aug 2018 16:37:57 +0200 (CEST) Received: from mail-oi0-f72.google.com ([209.85.218.72]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1fv1bY-0004jc-J7 for dev@dpdk.org; Wed, 29 Aug 2018 14:37:56 +0000 Received: by mail-oi0-f72.google.com with SMTP id q11-v6so4449726oih.15 for ; Wed, 29 Aug 2018 07:37:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=myYLYqm1XuGU+nvKKrY8FUSf9vb0P39XgyPOJoXyp40=; b=HJq+i9BkbReDjs5LtGJ511W43QOOklNOFtDyaA6k1ft/SyyzQi4Y7Cif7n6Mo47vDm 0QcdMN/xUwTM5qxE6BAk9M7CHnoS4ROLFzHi+K1axfnXJSRRWMUqgRxEZcA/LCDZHF6u i/7PbDFBAOdTXM5YMP5TnPI3qq5ETrTfJLQTia+n+rx35xw/wuAt1EaAoWLFQaN1QsIc QH+Lv3pKgmUWTC6KO0HDPajwE0K3zYyStn1jOXv/9YzoNo88OaMflQyTcLb9esO0/q5T xCXZnlRPRiG5Iy/eKPWu9a9Y1ist+o7dYceQ40LNqXHQpc6vtyJL/rwCvq4ck4SbhIcD qjlA== X-Gm-Message-State: APzg51CFz+cuDAWagUpK7dRLp6xDx1VjuNY8rZtHrOd2FD3b8zCR6QiO gNW/nuUXaE963CFi9AP+Oyr6uYVLEBB3srTYFGUaYIAhBMdLPlx45o+0YrhnHjTIZcPmHcRRitv YcWwibig1af3DJhXemu+iMBlL7wYS7O4eM7/S X-Received: by 2002:aca:ed57:: with SMTP id l84-v6mr2796298oih.62.1535553475504; Wed, 29 Aug 2018 07:37:55 -0700 (PDT) X-Google-Smtp-Source: ANB0VdaH6rqrODRg7LKgRKRMktznftRKPQ98AgYuDDibsgTHHgS0EDMye5D4+r9eUEJqfYqWAX0VVBcGX0/X2jqRFSI= X-Received: by 2002:aca:ed57:: with SMTP id l84-v6mr2796268oih.62.1535553475162; Wed, 29 Aug 2018 07:37:55 -0700 (PDT) MIME-Version: 1.0 References: <20180827122219.GB3695@6wind.com> <20180828114422.GG3695@6wind.com> <20180828150235.GH3695@6wind.com> <20180829131605.GJ3695@6wind.com> In-Reply-To: <20180829131605.GJ3695@6wind.com> From: Christian Ehrhardt Date: Wed, 29 Aug 2018 16:37:28 +0200 Message-ID: To: adrien.mazarguil@6wind.com Cc: dev , Gowrishankar Muthukrishnan , Chao Zhu , Luca Boccassi , Thomas Monjalon Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Aug 2018 14:37:57 -0000 On Wed, Aug 29, 2018 at 3:16 PM Adrien Mazarguil 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@6wind.com> > > wrote: > > > > > On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote: > > > > > --- 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 > > > > #include > > > > /*To include altivec.h, GCC version must >=3D 4.8 */ > > > > +/* > > > > + * If built with std=3Dc11 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__ >=3D 201112L && > > > > !defined(__APPLE_ALTIVEC__) > > > > +#define __APPLE_ALTIVEC__ > > > > +#endif > > > > #include > > > > > > > > #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 =E2=80=98;=E2=80=99 before =E2=80=98signed=E2=80=99 > > > > 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 =E2=80=98xmm_t=E2=80=99 > > > > 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 oth= er > > > > includes in drivers/net/mlx5/mlx5_nl.c > > > > I also moved mlx5.h (which eventually brings in altivec) right at t= he > > > top. > > > > This works to build, but such a check is always subtle as one of th= e > > > 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 o= n > > > "__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 > > 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 >=3D 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 > > > > > lib/librte_lpm/meson.build:9:headers +=3D files('rte_lpm_altivec.h', > > 'rte_lpm_neon.h', 'rte_lpm_sse.h') > > lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include += =3D > > 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 ma= y > 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=3Dc99 or -std=3Dc90. 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 mak= e > 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 > > #include > > +/* > > + * if built with newer C standard like -std=3Dc11 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 th= e > > + * 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 >=3D 4.8 */ > > #include > > > > 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 __pixe= l > or __bool beside __vector in that file. This should be carefully tested t= o > 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 a= sk > for altivec.h and get infected by it through header dependencies. > > Normally, -maltivec is all that's needed to get harmless bool/vector/pixe= l > context-sensitive keywords (even without including altivec.h) as expected > by > applications, however no one ever expects these to be harmful macros as i= s > 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 > > #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. Application= s > 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 includ= ed > before any DPDK headers. This shouldn't be a problem for the vast majorit= y. > > 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, " 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 > > +#include > > > > #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 +=3D rte_malloc.h rte_keepalive.h rte_time.h > > INC +=3D rte_service.h rte_service_component.h > > INC +=3D rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h > > INC +=3D rte_reciprocal.h rte_fbarray.h rte_uuid.h > > +INC +=3D rte_altivec.h > > > > GENERIC_INC :=3D rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch= .h > > GENERIC_INC +=3D 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 > > #include > > -/*To include altivec.h, GCC version must >=3D 4.8 */ > > -#include > > + > > +#include > > > > #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 > > +#include > > #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 withou= t > > + * modification, are permitted provided that the following condition= s > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyrigh= t > > + * 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 deriv= ed > > + * from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTO= RS > > + * "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 TO= RT > > + * (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=3Dc11 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 nee= d > to > > + * disambiguise type bool, we want vector/pixel to stay as-is so we > define > > + * them as altivec.h would. > > + * Note: without -std=3D the compiler has all those as context sensiti= ve > > + * 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 >=3D 4.8 */ > > This comment can probably be dropped given Clang also ships altivec.h and > GCC <=3D 4.8 is not actively supported anymore (sys_reqs.rst). > > > +#include > > + > > +#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 +=3D eal_common_arch_objs > > > > common_headers =3D 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 > --=20 Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd