[v3] ppc64: fix compilation of when AltiVec is enabled
Checks
Commit Message
The definition of almost any newer standard like --stc=c11 will drop
__APPLCE_ALTIVEC__ which otherwise would be defined.
If that is the case then altivec.h will redefine bool to a type
conflicting with those defined by stdbool.h.
This breaks compilation of 18.08 on ppc64 like:
mlx5_nl_flow.c:407:17: error: incompatible types when assigning
to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
from type ‘int’ in_port_id_set = false;
Other alternatives were pursued on [1] but they always ended up being
more complex than what would be appropriate for the issue we face.
[1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
.../common/include/arch/ppc_64/rte_memcpy.h | 11 +++++++++++
1 file changed, 11 insertions(+)
Comments
I think this patch is good enough to solve the confliction issue.
> -----Original Message-----
> From: Christian Ehrhardt [mailto:christian.ehrhardt@canonical.com]
> Sent: 2018年8月30日 20:00
> To: adrien.mazarguil@6wind.com; dev <dev@dpdk.org>; Gowrishankar
> Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>; Chao Zhu
> <chaozhu@linux.vnet.ibm.com>
> Cc: Luca Boccassi <bluca@debian.org>; Thomas Monjalon
> <thomasm@mellanox.com>; Christian Ehrhardt
> <christian.ehrhardt@canonical.com>
> Subject: [PATCH v3] ppc64: fix compilation of when AltiVec is enabled
>
> The definition of almost any newer standard like --stc=c11 will drop
> __APPLCE_ALTIVEC__ which otherwise would be defined.
> If that is the case then altivec.h will redefine bool to a type conflicting with
> those defined by stdbool.h.
>
> This breaks compilation of 18.08 on ppc64 like:
> mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> from type ‘int’ in_port_id_set = false;
>
> Other alternatives were pursued on [1] but they always ended up being more
> complex than what would be appropriate for the issue we face.
>
> [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
>
> Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
> Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
> .../common/include/arch/ppc_64/rte_memcpy.h | 11
> +++++++++++
> 1 file changed, 11 insertions(+)
>
> 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 75f74897b..0b3b89b56 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
> @@ -37,6 +37,17 @@
> #include <string.h>
> /*To include altivec.h, GCC version must >= 4.8 */ #include <altivec.h>
> +/*
> + * Compilation workaround for PPC64 targets when AltiVec is fully
> + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> + * of "bool" between stdbool and altivec.
> + */
> +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__) #undef bool
> + /* redefine as in stdbool.h */
> + #define bool _Bool
> +#endif
> +
>
> #ifdef __cplusplus
> extern "C" {
> --
> 2.17.1
Acked-by: Chao Zhu <chaozhu@linux.vnet.ibm.com>
On Fri, Aug 31, 2018 at 3:48 AM Chao Zhu <chaozhu@linux.vnet.ibm.com> wrote:
> I think this patch is good enough to solve the confliction issue.
>
> > -----Original Message-----
> > From: Christian Ehrhardt [mailto:christian.ehrhardt@canonical.com]
> > Sent: 2018年8月30日 20:00
> > To: adrien.mazarguil@6wind.com; dev <dev@dpdk.org>; Gowrishankar
> > Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>; Chao Zhu
> > <chaozhu@linux.vnet.ibm.com>
> > Cc: Luca Boccassi <bluca@debian.org>; Thomas Monjalon
> > <thomasm@mellanox.com>; Christian Ehrhardt
> > <christian.ehrhardt@canonical.com>
> > Subject: [PATCH v3] ppc64: fix compilation of when AltiVec is enabled
> >
> > The definition of almost any newer standard like --stc=c11 will drop
> > __APPLCE_ALTIVEC__ which otherwise would be defined.
> > If that is the case then altivec.h will redefine bool to a type
> conflicting with
> > those defined by stdbool.h.
> >
> > This breaks compilation of 18.08 on ppc64 like:
> > mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> > to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> > from type ‘int’ in_port_id_set = false;
> >
> > Other alternatives were pursued on [1] but they always ended up being
> more
> > complex than what would be appropriate for the issue we face.
> >
> > [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> >
> > Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
> > Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> > .../common/include/arch/ppc_64/rte_memcpy.h | 11
> > +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > 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 75f74897b..0b3b89b56 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
> > @@ -37,6 +37,17 @@
> > #include <string.h>
> > /*To include altivec.h, GCC version must >= 4.8 */ #include
> <altivec.h>
> > +/*
> > + * Compilation workaround for PPC64 targets when AltiVec is fully
> > + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> > + * of "bool" between stdbool and altivec.
> > + */
> > +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__) #undef bool
> > + /* redefine as in stdbool.h */
> > + #define bool _Bool
> > +#endif
> > +
> >
> > #ifdef __cplusplus
> > extern "C" {
> > --
> > 2.17.1
> Acked-by: Chao Zhu <chaozhu@linux.vnet.ibm.com>
>
Thanks Chao for taking over for now.
Being listed as Maintainer for "IBM Power" atm, can you push this yourself
or do you need someone else to commit it?
We’ll have internal discussion and push it.
Thanks!
From: Christian Ehrhardt [mailto:christian.ehrhardt@canonical.com]
Sent: 2018年8月31日 13:15
To: Chao Zhu <chaozhu@linux.vnet.ibm.com>
Cc: adrien.mazarguil@6wind.com; dev <dev@dpdk.org>; Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>; Luca Boccassi <bluca@debian.org>; Thomas Monjalon <thomasm@mellanox.com>
Subject: Re: [PATCH v3] ppc64: fix compilation of when AltiVec is enabled
On Fri, Aug 31, 2018 at 3:48 AM Chao Zhu <chaozhu@linux.vnet.ibm.com <mailto:chaozhu@linux.vnet.ibm.com> > wrote:
I think this patch is good enough to solve the confliction issue.
> -----Original Message-----
> From: Christian Ehrhardt [mailto:christian.ehrhardt@canonical.com <mailto:christian.ehrhardt@canonical.com> ]
> Sent: 2018年8月30日 20:00
> To: adrien.mazarguil@6wind.com <mailto:adrien.mazarguil@6wind.com> ; dev <dev@dpdk.org <mailto:dev@dpdk.org> >; Gowrishankar
> Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com <mailto:gowrishankar.m@linux.vnet.ibm.com> >; Chao Zhu
> <chaozhu@linux.vnet.ibm.com <mailto:chaozhu@linux.vnet.ibm.com> >
> Cc: Luca Boccassi <bluca@debian.org <mailto:bluca@debian.org> >; Thomas Monjalon
> <thomasm@mellanox.com <mailto:thomasm@mellanox.com> >; Christian Ehrhardt
> <christian.ehrhardt@canonical.com <mailto:christian.ehrhardt@canonical.com> >
> Subject: [PATCH v3] ppc64: fix compilation of when AltiVec is enabled
>
> The definition of almost any newer standard like --stc=c11 will drop
> __APPLCE_ALTIVEC__ which otherwise would be defined.
> If that is the case then altivec.h will redefine bool to a type conflicting with
> those defined by stdbool.h.
>
> This breaks compilation of 18.08 on ppc64 like:
> mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> from type ‘int’ in_port_id_set = false;
>
> Other alternatives were pursued on [1] but they always ended up being more
> complex than what would be appropriate for the issue we face.
>
> [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
>
> Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com <mailto:TYOS@jp.ibm.com> >
> Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com <mailto:adrien.mazarguil@6wind.com> >
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com <mailto:christian.ehrhardt@canonical.com> >
> ---
> .../common/include/arch/ppc_64/rte_memcpy.h | 11
> +++++++++++
> 1 file changed, 11 insertions(+)
>
> 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 75f74897b..0b3b89b56 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
> @@ -37,6 +37,17 @@
> #include <string.h>
> /*To include altivec.h, GCC version must >= 4.8 */ #include <altivec.h>
> +/*
> + * Compilation workaround for PPC64 targets when AltiVec is fully
> + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> + * of "bool" between stdbool and altivec.
> + */
> +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__) #undef bool
> + /* redefine as in stdbool.h */
> + #define bool _Bool
> +#endif
> +
>
> #ifdef __cplusplus
> extern "C" {
> --
> 2.17.1
Acked-by: Chao Zhu <chaozhu@linux.vnet.ibm.com <mailto:chaozhu@linux.vnet.ibm.com> >
Thanks Chao for taking over for now.
Being listed as Maintainer for "IBM Power" atm, can you push this yourself or do you need someone else to commit it?
Hi Christian,
Couldn't follow up on this last week, however I still have some concerns and
comments, please see below.
On Thu, Aug 30, 2018 at 01:59:59PM +0200, Christian Ehrhardt wrote:
> The definition of almost any newer standard like --stc=c11 will drop
> __APPLCE_ALTIVEC__ which otherwise would be defined.
> If that is the case then altivec.h will redefine bool to a type
> conflicting with those defined by stdbool.h.
>
> This breaks compilation of 18.08 on ppc64 like:
> mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> from type ‘int’ in_port_id_set = false;
>
> Other alternatives were pursued on [1] but they always ended up being
> more complex than what would be appropriate for the issue we face.
>
> [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
>
> Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
> Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
> .../common/include/arch/ppc_64/rte_memcpy.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> 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 75f74897b..0b3b89b56 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
> @@ -37,6 +37,17 @@
> #include <string.h>
> /*To include altivec.h, GCC version must >= 4.8 */
> #include <altivec.h>
> +/*
> + * Compilation workaround for PPC64 targets when AltiVec is fully
> + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> + * of "bool" between stdbool and altivec.
> + */
> +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
> + #undef bool
> + /* redefine as in stdbool.h */
> + #define bool _Bool
> +#endif
> +
The above will break existing C++ programs that include rte_memcpy.h.
Problem is that bool is an actual C++ type. C99 has _Bool which doesn't
exist in C++ along with a bool macro that appears only after including
stdbool.h.
To make things worse, nothing prevents C++ programs from importing a C-style
bool macro by including stdbool.h (or cstdbool).
Enclosing it in #ifdef __cplusplus won't help because you never know what
bool is supposed to be in the first place as it depends on how applications
are written. I think something like this prior suggestion [1]
(saving/restoring bool) is the only way to deal with that in a safe-ish
fashion.
Pending something better, the above #undef/#define workaround is only safe
to use inside mlx5 PMD code that triggers the compilation issue. It must not
be found in a public header.
> #ifdef __cplusplus
> extern "C" {
> --
> 2.17.1
>
[1] https://mails.dpdk.org/archives/dev/2018-August/110401.html
03/09/2018 11:29, Adrien Mazarguil:
> Hi Christian,
>
> Couldn't follow up on this last week, however I still have some concerns and
> comments, please see below.
>
> On Thu, Aug 30, 2018 at 01:59:59PM +0200, Christian Ehrhardt wrote:
> > The definition of almost any newer standard like --stc=c11 will drop
> > __APPLCE_ALTIVEC__ which otherwise would be defined.
> > If that is the case then altivec.h will redefine bool to a type
> > conflicting with those defined by stdbool.h.
> >
> > This breaks compilation of 18.08 on ppc64 like:
> > mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> > to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> > from type ‘int’ in_port_id_set = false;
> >
> > Other alternatives were pursued on [1] but they always ended up being
> > more complex than what would be appropriate for the issue we face.
> >
> > [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> >
> > Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
> > Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> > .../common/include/arch/ppc_64/rte_memcpy.h | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > --- 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,17 @@
> > #include <string.h>
> > /*To include altivec.h, GCC version must >= 4.8 */
> > #include <altivec.h>
> > +/*
> > + * Compilation workaround for PPC64 targets when AltiVec is fully
> > + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> > + * of "bool" between stdbool and altivec.
> > + */
> > +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
> > + #undef bool
> > + /* redefine as in stdbool.h */
> > + #define bool _Bool
> > +#endif
> > +
>
> The above will break existing C++ programs that include rte_memcpy.h.
>
> Problem is that bool is an actual C++ type. C99 has _Bool which doesn't
> exist in C++ along with a bool macro that appears only after including
> stdbool.h.
>
> To make things worse, nothing prevents C++ programs from importing a C-style
> bool macro by including stdbool.h (or cstdbool).
>
> Enclosing it in #ifdef __cplusplus won't help because you never know what
> bool is supposed to be in the first place as it depends on how applications
> are written. I think something like this prior suggestion [1]
> (saving/restoring bool) is the only way to deal with that in a safe-ish
> fashion.
>
> Pending something better, the above #undef/#define workaround is only safe
> to use inside mlx5 PMD code that triggers the compilation issue. It must not
> be found in a public header.
>
> > #ifdef __cplusplus
> > extern "C" {
>
> [1] https://mails.dpdk.org/archives/dev/2018-August/110401.html
Thank you for the review Adrien. I think we could accept the patch
if some notes mention it breaks C++ applications.
But...
After 2 months, nobody replied or complained about the issue not fixed.
So I classify this patch as rejected.
Summary of alerts here:
http://mails.dpdk.org/archives/dev/2018-November/118259.html
@@ -37,6 +37,17 @@
#include <string.h>
/*To include altivec.h, GCC version must >= 4.8 */
#include <altivec.h>
+/*
+ * Compilation workaround for PPC64 targets when AltiVec is fully
+ * enabled e.g. with std=c11. Otherwise there would be a type conflict
+ * of "bool" between stdbool and altivec.
+ */
+#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
+ #undef bool
+ /* redefine as in stdbool.h */
+ #define bool _Bool
+#endif
+
#ifdef __cplusplus
extern "C" {