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 43CF32B92 for ; Tue, 28 Aug 2018 14:39:04 +0200 (CEST) Received: from mail-oi0-f69.google.com ([209.85.218.69]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1fudGx-0004X1-QC for dev@dpdk.org; Tue, 28 Aug 2018 12:39:04 +0000 Received: by mail-oi0-f69.google.com with SMTP id 20-v6so1071401ois.21 for ; Tue, 28 Aug 2018 05:39:03 -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=RcB5yhTOvmN+GvWr30/I2QIgbI25azIZ33UDb44qvLw=; b=NC2k2JWbULt4LK/4zIE3ecar8ZhatFSs5vBxPs1glEFoIpE8Du7+Jk7awEl+oDC5xe m6eZy+BiqyPI8g8ib9IJMvtVV049QfVoZZ2cVgyyn3I7O7J5qFx740mHw7ZrV1kyAecF WjshfdISxy3QAz0br2PzpNA61TXCrdceOV2oa6hLwf1tpcxEOqFqLxdi5qOo3t4UXmp2 dDJDEMg6BklNKrGs1yb5Do1hoNDNk2rs40hfgk0tuBplV6Gdxp7UR7plRQ8TUgoJKAW0 lBdU/YRLsfYhLEdR3NtPs/0ifGCDrVX2+rppXsWoqjrPszXsjcY120cPw1t8fp2ngi5g ixGw== X-Gm-Message-State: APzg51Chz7zIrCSqrbQ/i1tWTSFUc3inH3/CDc00ZDk++WEQSDpkxoYj F5OvNeLQ5medOSVms8cNyITlBGiuu6C0WEdRGM5/wR6SAAnkAwuv/1zg/wtBRnvtrGYkWvzLYHl vAfmApWj6nC2q79THSzgJCdYI92aB0ui26HJ3 X-Received: by 2002:aca:dbc2:: with SMTP id s185-v6mr1205838oig.251.1535459942764; Tue, 28 Aug 2018 05:39:02 -0700 (PDT) X-Google-Smtp-Source: ANB0VdbcAofmbHz7ej2aobCxIsnW/F7bicdO42ryXtxDcDHN7ajQSkw0S1W88SzoOXPDYWx5SMsict+KoqDsCBIlQXA= X-Received: by 2002:aca:dbc2:: with SMTP id s185-v6mr1205821oig.251.1535459942465; Tue, 28 Aug 2018 05:39:02 -0700 (PDT) MIME-Version: 1.0 References: <20180827122219.GB3695@6wind.com> <20180828114422.GG3695@6wind.com> In-Reply-To: <20180828114422.GG3695@6wind.com> From: Christian Ehrhardt Date: Tue, 28 Aug 2018 14:38:35 +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: Tue, 28 Aug 2018 12:39:04 -0000 On Tue, Aug 28, 2018 at 1:44 PM Adrien Mazarguil wrote: > On Tue, Aug 28, 2018 at 01:30:12PM +0200, Christian Ehrhardt wrote: > > On Mon, Aug 27, 2018 at 2:22 PM Adrien Mazarguil < > adrien.mazarguil@6wind.com> > > wrote: > > > > > Hi Christian, > > > > > > On Wed, Aug 22, 2018 at 05:11:41PM +0200, Christian Ehrhardt wrote: > > > > Just FYI the simple change hits similar issues later on. > > > > > > > > The (not really) proposed patch would have to be extended to be as > > > > following. > > > > We really need a better solution (or somebody has to convince me > that my > > > > change is better than a band aid). > > > > > > Thanks for reporting. I've made a quick investigation on my own and > believe > > > it's a toolchain issue which may affect more than this PMD; > potentially all > > > users of stdbool.h (C11) on this platform. > > > > > > > Yeah I assumed as much, which is why I was hoping that some of the arch > > experts would jump in and say "yeah this is a common thing and correctl= y > > handled like " > > I'll continue trying to reach out to people that should know better sti= ll > > ... > > > > > > > C11's stdbool.h defines a bool macro as _Bool (big B) along with > > > true/false. On PPC targets, another file (altivec.h) defines bool as > _bool > > > (small b) but not true/false: > > > > > > #if !defined(__APPLE_ALTIVEC__) > > > /* You are allowed to undef these for C++ compatibility. */ > > > #define vector __vector > > > #define pixel __pixel > > > #define bool __bool > > > #endif > > > > > > mlx5_nl.c explicitly includes stdbool.h to get the above definitions > then > > > includes mlx5.h -> rte_ether.h -> ppc_64/rte_memcpy.h -> altivec.h. > > > > > > For some reason the conflicting bool redefinition doesn't seem to > raise any > > > warnings, but results in mismatching bool and true/false definitions; > an > > > integer value cannot be assigned to a bool variable anymore, hence th= e > > > build > > > failure. > > > > > > The inability to assign integer values to bool is, in my opinion, a > > > fundamental issue caused by altivec.h. If there is no way to fix this > on > > > the > > > system, there are a couple of workarounds for DPDK, by order of > preference: > > > > > > 1. Always #undef bool after including altivec.h in > > > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h. I do not > think > > > anyone expects this type to be unusable with true/false or integer > > > values > > > anyway. The version of altivec.h I have doesn't rely on this macro > at > > > all so it's probably not a big loss. > > > > > > > The undef of a definition in header A by hedaer B can lead to most > > interesting, still broken effects. > > If e.g. one does > > #include > > #include "mlx5.h" > > > > or similar then it would undefine that of stdbool as well right? > > In any case, the undefine not only would be suspicious it also fails > right > > away: > > > > In file included from > > /home/ubuntu/deb_dpdk/lib/librte_eal/common/malloc_heap.c:27: > > /home/ubuntu/deb_dpdk/lib/librte_eal/common/eal_memalloc.h:30:15: > > error: unknown > > type name =E2=80=98bool=E2=80=99; did you mean =E2=80=98_Bool=E2=80=99? > > int socket, bool exact); > > ^~~~ > > _Bool > > [...] > > > > > > > > > Ditto for "pixel" and "vector" keywords. Alternatively you could > #define > > > __APPLE_ALTIVEC__ before including altivec.h to prevent them from > > > getting > > > defined in the first place. > > > > > > > Interesting I got plenty of these: > > In file included from > > /home/ubuntu/deb_dpdk/lib/librte_eal/common/eal_common_options.c:25: > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_memcpy.h:39: > > warning: > > "__APPLE_ALTIVEC__" redefined > > #define __APPLE_ALTIVEC__ > > > > With a few of it being even errors, but the position of the original > define > > is interesting. > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_memcpy.h:39: > error: > > "__APPLE_ALTIVEC__" redefined [-Werror] > > #define __APPLE_ALTIVEC__ > > : note: this is the location of the previous definition > > > > So if being a built-in, shouldn't it ALWAYS be defined and never > > over-declare the bool type? > > > > Checking GCC on the platform: > > $ gcc -dM -E - < /dev/null | grep ALTI > > #define __ALTIVEC__ 1 > > #define __APPLE_ALTIVEC__ 1 > > > > > > I added an #error in the header and dropped all dpdk changes. > > if !defined(__APPLE_ALTIVEC__) > > /* You are allowed to undef these for C++ compatibility. */ > > #error WOULD REDECLARE BOOL > > #define vector __vector > > > > And I get: > > gcc -Wp,-MD,./.mlx4.o.d.tmp -Wdate-time -D_FORTIFY_SOURCE=3D2 -m64 -pth= read > > -DRTE_MACHINE_CPUFLAG_PPC64 -DRTE_MACHINE_CPUFLAG_ALTIVEC > > -DRTE_MACHINE_CPUFLAG_VSX -I/home/ubuntu/deb_dpdk/debia > > n/build/static-root/include -include > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_config.h -O3 > > -std=3Dc11 -Wall -Wextra -g -I. -D_BSD_SOURCE -D_DEFAULT_SOURCE > > -D_XOPEN_SOURCE=3D600 > > -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declaration= s > > -Wold-style-definition -Wpointer-arith -Wcast-align -Wnested-externs > > -Wcast-qual -Wformat-nonliteral -Wformat-securi > > ty -Wundef -Wwrite-strings -Wdeprecated -Wimplicit-fallthrough=3D2 > > -Wno-format-truncation -DALLOW_EXPERIMENTAL_API -Wno-error=3Dcast-qual > > -DNDEBUG -UPEDANTIC -g -g -o mlx4.o -c /home/ubuntu/de > > b_dpdk/drivers/net/mlx4/mlx4.c > > In file included from > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_memcpy.h:39, > > from > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_ether.h:21, > > from > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_ethdev.h:158= , > > from > > > /home/ubuntu/deb_dpdk/debian/build/static-root/include/rte_ethdev_driver.= h:18, > > > > from /home/ubuntu/deb_dpdk/drivers/net/mlx4/mlx4.c:35: > > /usr/lib/gcc/powerpc64le-linux-gnu/8/include/altivec.h:44:2: error: > #error > > WOULD REDECLARE BOOL > > #error WOULD REDECLARE BOOL > > > > But a quick sanity test with a hello world including this special altiv= ec > > header did build not reaching the #error. > > So something in DPDK undefines __ALTIVEC__ ?! > > > > After being that close I found which of our usual build does the switch > to > > trigger this. > > It is "-std=3Dc11" > > > > And in fact > > $ gcc -std=3Dc11 -dM -E - < /dev/null | grep ALTI > > #define __ALTIVEC__ 1 > > > > But no __APPLE_ALTIVEC__ > > > > The header says > > /* You are allowed to undef these for C++ compatibility. */ > > > > But I thought "wait a minute, didn't we just undefine it above and it > > failed?" > > Yes we did, and it turns out not all gcc calls have --std=3Dc11 and due= to > > that it failed for those. > > > > > > > > 2. Add Altivec detection to impacted users of stdbool.h, which #undef a= nd > > > redefine bool as _Bool on their own with a short comment about > broken > > > toolchains. > > > > > > > I tested a few versions of this after my findings on #1 above. > > A few extra loops to jump like to make drivers/net/cxgbe/cxgbe_compat.h > > usage of bool happy. > > I eventually came up with the following as a fix that seems to work: > > > > --- 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,19 @@ > > #include > > /*To include altivec.h, GCC version must >=3D 4.8 */ > > #include > > +/* > > + * if built with std=3Dc11 stdbool and vector bool will conflict. > > + * But if std is not c11 (which is true for some of our gcc calls) it > will > > + * have __APPLE_ALTIVEC__ defined which will make it not define the > types > > + * at all. > > + * Furthermore we need to be careful to only redefine as stdbool would > have > > + * done if it was included - otherwise we might conflict with other > > intended > > + * meanings of "bool". > > + */ > > +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >=3D 201112L && > > defined(_STDBOOL_H) > > +#undef bool > > +#define bool _Bool > > +#endif > > > > #ifdef __cplusplus > > extern "C" { > > > > > > In turn I have only checked this modification on ppc64 so far, but > anyway I > > still have the feeling we are only trying to poke at things with a stic= k > > and someone with subject matter experience would just tell us. > > Opinions on the change above as a "v2 RFC"? > > Thanks for the detailed analysis :) > > I'm afraid this suggestion can still break since stdbool.h won't be > necessarily included before altivec.h. How about this instead? > > /* Blurb */ > #ifndef __APPLE_ALTIVEC__ > #define __APPLE_ALTIVEC__ 1 > #endif > #include > I was there before in my experiments - even a bit safer with the following to only do so in C11 mode to avoid cases where one might have "undefined" it in non C11 for a reason so we do not switch it on again unexpectedly. --- 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 conflic= t + * 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: er= ror: 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: err= or: 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 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. $ grep -e 'stdbool.h' -e 'altivec.h' mlx5_nl.E # 1 "/usr/lib/gcc/powerpc64le-linux-gnu/8/include/altivec.h" 1 3 4 # 1 "/usr/lib/gcc/powerpc64le-linux-gnu/8/include/stdbool.h" 1 3 4 Still the build worked with the fix as suggested in my last mail: --- 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,19 @@ #include /*To include altivec.h, GCC version must >=3D 4.8 */ #include +/* + * if built with std=3Dc11 stdbool and vector bool will conflict. + * But if std is not c11 (which is true for some of our gcc calls) it will + * have __APPLE_ALTIVEC__ defined which will make it not define the types + * at all. + * Furthermore we need to be careful to only redefine as stdbool would hav= e + * done if it was included - otherwise we might conflict with other intended + * meanings of "bool". + */ +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >=3D 201112L && defined(_STDBOOL_H) +#undef bool +#define bool _Bool +#endif #ifdef __cplusplus extern "C" { Which is odd, as I'd have expected the stdbool.h inclusion would then trigger a redefinition of the bool type. --=20 > Adrien Mazarguil > 6WIND > --=20 Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd