From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <christian.ehrhardt@canonical.com>
Received: from youngberry.canonical.com (youngberry.canonical.com
[91.189.89.112]) by dpdk.org (Postfix) with ESMTP id 43CF32B92
for <dev@dpdk.org>; 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 <christian.ehrhardt@canonical.com>)
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 <dev@dpdk.org>; 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: <CAATJJ0Ld_3o7FzZaoRO_eK2qWDpBuYg6QtKDCVjEUh22V_7k=w@mail.gmail.com>
<CAATJJ0LP=u_QufpZJv-Q96+vWm_NA_Nneh7hqOorZQ_=O=2+sg@mail.gmail.com>
<20180827122219.GB3695@6wind.com>
<CAATJJ0Ljz9uUKhfWfts6MvfTDN0K1YmXtKEC5zuFZKpka5PqOw@mail.gmail.com>
<20180828114422.GG3695@6wind.com>
In-Reply-To: <20180828114422.GG3695@6wind.com>
From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Date: Tue, 28 Aug 2018 14:38:35 +0200
Message-ID: <CAATJJ0KFLySTL=9mL0eN-T+-OttzxMvJRDc0TO0iM4fgsrJGqA@mail.gmail.com>
To: adrien.mazarguil@6wind.com
Cc: dev <dev@dpdk.org>,
Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>,
Chao Zhu <chaozhu@linux.vnet.ibm.com>,
Luca Boccassi <bluca@debian.org>, Thomas Monjalon <thomasm@mellanox.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 28 Aug 2018 12:39:04 -0000
On Tue, Aug 28, 2018 at 1:44 PM Adrien Mazarguil <adrien.mazarguil@6wind.co=
m>
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 <FOO>"
> > 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 <stdbool.h>
> > #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__
> > <built-in>: 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 <string.h>
> > /*To include altivec.h, GCC version must >=3D 4.8 */
> > #include <altivec.h>
> > +/*
> > + * 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 <altivec.h>
>
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 <stdint.h>
#include <string.h>
/*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 <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: 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 <string.h>
/*To include altivec.h, GCC version must >=3D 4.8 */
#include <altivec.h>
+/*
+ * 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