From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id EC51C5A for ; Wed, 29 Aug 2018 15:16:22 +0200 (CEST) Received: by mail-wm0-f66.google.com with SMTP id c14-v6so5264364wmb.4 for ; Wed, 29 Aug 2018 06:16:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=P7ogwZnp/ZAtSel3ZgX04ohguunlnhRernMFhdXt2ZM=; b=u37nsKHCb9Irbbnxz9knAHR+vYbu7RL/nq38o/KFBEiBNLP/0LQ+SF5zJUkJh6qcif XGlagf232WLDsKJ5kgSMWG0qQoTUK9YMDgWDrC6F7D9yYOe+o3CsCh1JY0sjkSkvb7uz zKqm/di83nNCBfiHBhN8AqIMbQvTZ7mhMTobwvuzeLmNzdhmUAqp8KDEd50SgS/68O6g 7YpXa4zlSNN5hIU86uDu3B40oqPORM9GUOMJvQQTs33RfzQLRNeNBbixvk00f5foWlcG 6VD+2nPm/5qFUKO52ZJrw5+4bMxfEoKDmWRsSguqLIr8Wv/B81n+5Y/G+7I0OyMRD7uf bJiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=P7ogwZnp/ZAtSel3ZgX04ohguunlnhRernMFhdXt2ZM=; b=Y7wPjsAirhRzlBvZxdLjx2vzXYVxt54HmhSboK4K8frhhbEkD+lbaIjS1lRkK6ht3n JkugjhvYcGbJYdbfB+Xtwv3IbTFAUbHYXVZJm8KehZvFpx1+Gvz8mrKhZmRRONgn1J6I K8qTnIw4gxTN+qHrLbDCRx1BXXw/eVufaYOs/OKt3XGspFzk/R0CKfMfNM0u/lF6rzy0 DfTtEPyQ1EFePZKUxGSZda24sEuXnhLVcaJRZ4ldVmxQlsHG00UeBDhhTW7UUJuCLVZG TVGYGirQXb6GDuckTC64fJ+69uPUBGH8ZZ8LeQWZzuWK+mZRIG50BGm5O7riPeMwJZCg L36g== X-Gm-Message-State: APzg51BruAP117a4T4+QHCHlxTQ9c2AmvSxDOgCYo/5HuJ6BkKaLNaH8 FBiL+uevEAuLikIXj7DuZR582g== X-Google-Smtp-Source: ANB0VdYqW0x5ghTIcVzxWZFBIiyTJt6/Vv+nRT4n3BxuiNyhqorifItSU/QMtJ+DLFGfCPY1aQ7ZkA== X-Received: by 2002:a1c:1943:: with SMTP id 64-v6mr314773wmz.89.1535548582393; Wed, 29 Aug 2018 06:16:22 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id e14-v6sm2536643wrv.44.2018.08.29.06.16.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Aug 2018 06:16:20 -0700 (PDT) Date: Wed, 29 Aug 2018 15:16:05 +0200 From: Adrien Mazarguil To: Christian Ehrhardt Cc: dev , Gowrishankar Muthukrishnan , Chao Zhu , Luca Boccassi , Thomas Monjalon Message-ID: <20180829131605.GJ3695@6wind.com> References: <20180827122219.GB3695@6wind.com> <20180828114422.GG3695@6wind.com> <20180828150235.GH3695@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 13:16:23 -0000 On Wed, Aug 29, 2018 at 10:27:03AM +0200, Christian Ehrhardt wrote: > On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil > 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 >= 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 > > > > > > #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 > 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 > > 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 > #include > +/* > + * 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 > > 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 #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 > +#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 += 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 > #include > -/*To include altivec.h, GCC version must >= 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 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 > + > +#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