From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id D83D498 for ; Tue, 28 Aug 2018 17:02:52 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id b19-v6so2277703wme.3 for ; Tue, 28 Aug 2018 08:02:52 -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=9xoAI7R6V2vH8Y0Ij+V0XvZGbyUeqw4X+3WpD5lawsA=; b=1o6izwRG7evFK+T0wDmWzYCAd7zPHgbbJNqKJcHdMUVnQmSRNIISeBKSACOnMIBuY3 AruBun43+FcYBqHk19TXXRVcizKWehvWiqTh7kYcqCC/5mXFdc/HrP6J+DsFv6X2tRU2 s4oTbkVlzIOxOYlthF0x5ocwBwyJx2mvltIfLcLIL/2tdoPiBRpEvydx32l4BMll5ZGQ d65141bPEAAfm3QMBNGdxLkmJj3ZXZJ+ZnpAkQgAclymuR7wFlIP6FoqQuoN7dPD4R3/ aHvOCMqEZJ3bQrpwsvgBlvrqG0tXz2wSKe4ox74JeGLMplNypSoiHpscyCWsMrbXM1wC ng8A== 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=9xoAI7R6V2vH8Y0Ij+V0XvZGbyUeqw4X+3WpD5lawsA=; b=ZyhL+W5JBYv8XLZXR61bGcl7xpNZJF5XFgkQ2U26eUbJIEXQoJWyxZLE9/+pKVVbvZ lsmm5iJEeKgZZ4FO3w9K73Pxh8ZizDVIpEy9OkAR7maxD3ylCivdXZu2wafClp9MjFyO sApc18AIuKJVNCrzqvcCWn05Z+b6/MTGAKHgTJD7cs8tPQO0KP5OLIQj2yRFBk2hJEVl lbW8/EC2zbYAkhsJHS5ZEjPH0asmiScCBR9Bh29IEPFPPf/Vw9TyEq3iM1UHoFlsvBmK tNHjmNokiQLCLnRSUlPE/ni13c8PhphpYvjpAqzX6eYMIoDjhjmwAyJUhiQHGZM1sjHM GdJA== X-Gm-Message-State: APzg51CiNLC8phrW+4XFNFh0ZvLTt6GjU3BkHRMT7yVomiQM/+lGFy9i 2I+IKLKQWKOwpKStzIN8sgO8Xg== X-Google-Smtp-Source: ANB0Vdafs1Un6TFoHUwABkTtfJU19Lc8AyTVPhZyZNtL3pPvG4UNjVteqeBajKwTHtHzaPs1+D4ldw== X-Received: by 2002:a1c:ac1:: with SMTP id 184-v6mr1583079wmk.119.1535468572447; Tue, 28 Aug 2018 08:02:52 -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 t9-v6sm2594955wra.91.2018.08.28.08.02.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Aug 2018 08:02:51 -0700 (PDT) Date: Tue, 28 Aug 2018 17:02:35 +0200 From: Adrien Mazarguil To: Christian Ehrhardt Cc: dev , Gowrishankar Muthukrishnan , Chao Zhu , Luca Boccassi , Thomas Monjalon Message-ID: <20180828150235.GH3695@6wind.com> References: <20180827122219.GB3695@6wind.com> <20180828114422.GG3695@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: Tue, 28 Aug 2018 15:02:53 -0000 On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote: > 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 correctly > > > handled like " > > > I'll continue trying to reach out to people that should know better still > > > ... > > > > > > > > > > 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 the > > > > 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 ‘bool’; did you mean ‘_Bool’? > > > 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=2 -m64 -pthread > > > -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=c11 -Wall -Wextra -g -I. -D_BSD_SOURCE -D_DEFAULT_SOURCE > > > -D_XOPEN_SOURCE=600 > > > -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations > > > -Wold-style-definition -Wpointer-arith -Wcast-align -Wnested-externs > > > -Wcast-qual -Wformat-nonliteral -Wformat-securi > > > ty -Wundef -Wwrite-strings -Wdeprecated -Wimplicit-fallthrough=2 > > > -Wno-format-truncation -DALLOW_EXPERIMENTAL_API -Wno-error=cast-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 altivec > > > 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=c11" > > > > > > And in fact > > > $ gcc -std=c11 -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=c11 and due to > > > that it failed for those. > > > > > > > > > > > > 2. Add Altivec detection to impacted users of stdbool.h, which #undef and > > > > 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 >= 4.8 */ > > > #include > > > +/* > > > + * if built with std=c11 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__ >= 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 stick > > > 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 >= 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. 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. > $ 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 >= 4.8 */ > #include > +/* > + * if built with std=c11 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__ >= 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. Yeah, seems like internal GCC headers in /usr/lib are exempt from warnings on conflicting definitions or some such. -- Adrien Mazarguil 6WIND