diff mbox series

eal/ppc: fix redefine bool type

Message ID 1588060706-27316-1-git-send-email-orika@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series eal/ppc: fix redefine bool type |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-nxp-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Ori Kam April 28, 2020, 7:58 a.m. UTC 
  The AltiVec header file breaks boolean type. [1] [2]

Currently the workaround was located only in mlx5 device.
Adding the trace module caused this issue to appear again, due to
order of includes, it keeps overriding the local fix.

This patch solves this issue by resetting the bool type, immediately
after it is being changed.

[1] https://mails.dpdk.org/archives/dev/2018-August/110281.html

[2]
In file included from
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:18:0,
                 from
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool.h:54,
                 from
dpdk/drivers/common/mlx5/mlx5_common_mr.c:7:
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h: In
function '__rte_trace_point_fp_is_enabled':
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:226:2:
error: incompatible types when returning type 'int' but '__vector __bool
int' was expected
  return false;
  ^
In file included from
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:281:0,
                 from
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:18,
                 from
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool.h:54,
                 from
dpdk/drivers/common/mlx5/mlx5_common_mr.c:7:
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:
In function 'rte_mempool_trace_ops_dequeue_bulk':
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point_provider.h:104:6:
error: wrong type argument to unary exclamation mark
  if (!__rte_trace_point_fp_is_enabled()) \
      ^
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:49:2:
note: in expansion of macro '__rte_trace_point_emit_header_fp'
  __rte_trace_point_emit_header_##_mode(&__##_tp); \
  ^
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:99:2:
note: in expansion of macro '__RTE_TRACE_POINT'
  __RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
  ^
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:20:1:
note: in expansion of macro 'RTE_TRACE_POINT_FP'
 RTE_TRACE_POINT_FP(
 ^
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:
In function 'rte_mempool_trace_ops_dequeue_contig_blocks':
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point_provider.h:104:6:
error: wrong type argument to unary exclamation mark
  if (!__rte_trace_point_fp_is_enabled()) \
      ^
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:49:2:
note: in expansion of macro '__rte_trace_point_emit_header_fp'
  __rte_trace_point_emit_header_##_mode(&__##_tp); \
  ^
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point.h:99:2:
note: in expansion of macro '__RTE_TRACE_POINT'
  __RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
  ^
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:29:1:
note: in expansion of macro 'RTE_TRACE_POINT_FP'
 RTE_TRACE_POINT_FP(
 ^
dpdk/ppc_64-power8-linux-gcc/include/rte_mempool_trace_fp.h:
In function 'rte_mempool_trace_ops_enqueue_bulk':
dpdk/ppc_64-power8-linux-gcc/include/rte_trace_point_provider.h:104:6:
error: wrong type argument to unary exclamation mark
  if (!__rte_trace_point_fp_is_enabled()) \

Fixes: 725f5dd0bfb5 ("net/mlx5: fix build on PPC64")

Signed-off-by: Ori Kam <orika@mellanox.com>
---
 drivers/common/mlx5/mlx5_common.h        | 10 ----------
 drivers/net/mlx5/mlx5_utils.h            | 10 ----------
 lib/librte_eal/ppc/include/meson.build   |  1 +
 lib/librte_eal/ppc/include/rte_altivec.h | 22 ++++++++++++++++++++++
 lib/librte_eal/ppc/include/rte_memcpy.h  |  3 +--
 5 files changed, 24 insertions(+), 22 deletions(-)
 create mode 100644 lib/librte_eal/ppc/include/rte_altivec.h

Comments

David Christensen April 28, 2020, 6:20 p.m. UTC | #1 
> Fixes: 725f5dd0bfb5 ("net/mlx5: fix build on PPC64")
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---

There are a couple of other uses that should be covered in the patch:

diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c 
b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
index 5fa92bf92..72bd410fc 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
@@ -13,7 +13,7 @@
  #include "i40e_rxtx.h"
  #include "i40e_rxtx_vec_common.h"

-#include <altivec.h>
+#include "rte_altivec.h"

  #pragma GCC diagnostic ignored "-Wcast-qual"


diff --git a/drivers/net/virtio/virtio_rxtx_simple_altivec.c 
b/drivers/net/virtio/virtio_rxtx_simple_altivec.c
index 47225f412..b2bdd05c8 100644
--- a/drivers/net/virtio/virtio_rxtx_simple_altivec.c
+++ b/drivers/net/virtio/virtio_rxtx_simple_altivec.c
@@ -9,7 +9,7 @@
  #include <string.h>
  #include <errno.h>

-#include <altivec.h>
+#include "rte_altivec.h"

  #include <rte_byteorder.h>
  #include <rte_branch_prediction.h>

Dave
David Marchand April 29, 2020, 8:17 a.m. UTC | #2 
On Tue, Apr 28, 2020 at 9:59 AM Ori Kam <orika@mellanox.com> wrote:
>
> The AltiVec header file breaks boolean type. [1] [2]
>
> Currently the workaround was located only in mlx5 device.
> Adding the trace module caused this issue to appear again, due to
> order of includes, it keeps overriding the local fix.
>
> This patch solves this issue by resetting the bool type, immediately
> after it is being changed.


With this patch applied, there are still a few remaining spots as
mentioned by David C.
I see rte_vect.h too.

$ git grep -w altivec.h
MAINTAINERS:F: examples/l3fwd/*altivec.h
drivers/net/i40e/i40e_rxtx_vec_altivec.c:#include <altivec.h>
drivers/net/mlx5/mlx5_rxtx_vec_altivec.h:#include <altivec.h>
drivers/net/virtio/virtio_rxtx_simple_altivec.c:#include <altivec.h>
lib/librte_eal/ppc/include/rte_altivec.h:/* To include altivec.h, GCC
version must be >= 4.8 */
lib/librte_eal/ppc/include/rte_altivec.h:#include <altivec.h>
lib/librte_eal/ppc/include/rte_vect.h:#include <altivec.h>


> diff --git a/lib/librte_eal/ppc/include/rte_memcpy.h b/lib/librte_eal/ppc/include/rte_memcpy.h
> index 25311ba..d234e21 100644
> --- a/lib/librte_eal/ppc/include/rte_memcpy.h
> +++ b/lib/librte_eal/ppc/include/rte_memcpy.h
> @@ -8,13 +8,12 @@
>
>  #include <stdint.h>
>  #include <string.h>
> -/*To include altivec.h, GCC version must  >= 4.8 */
> -#include <altivec.h>

Why move the inclusion under the __cplusplus check?


>
>  #ifdef __cplusplus
>  extern "C" {
>  #endif
>
> +#include "rte_altivec.h"
>  #include "generic/rte_memcpy.h"
>
>  static inline void
> --
> 1.8.3.1
>

Thanks.
Ori Kam April 30, 2020, 8:53 a.m. UTC | #3 
Hi  David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, April 29, 2020 11:17 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>; David Christensen
> <drc@linux.vnet.ibm.com>; dev <dev@dpdk.org>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eal/ppc: fix redefine bool type
> 
> On Tue, Apr 28, 2020 at 9:59 AM Ori Kam <orika@mellanox.com> wrote:
> >
> > The AltiVec header file breaks boolean type. [1] [2]
> >
> > Currently the workaround was located only in mlx5 device.
> > Adding the trace module caused this issue to appear again, due to
> > order of includes, it keeps overriding the local fix.
> >
> > This patch solves this issue by resetting the bool type, immediately
> > after it is being changed.
> 
> 
> With this patch applied, there are still a few remaining spots as
> mentioned by David C.
> I see rte_vect.h too.
> 

I will add the missing code.

> $ git grep -w altivec.h
> MAINTAINERS:F: examples/l3fwd/*altivec.h
> drivers/net/i40e/i40e_rxtx_vec_altivec.c:#include <altivec.h>
> drivers/net/mlx5/mlx5_rxtx_vec_altivec.h:#include <altivec.h>
> drivers/net/virtio/virtio_rxtx_simple_altivec.c:#include <altivec.h>
> lib/librte_eal/ppc/include/rte_altivec.h:/* To include altivec.h, GCC
> version must be >= 4.8 */
> lib/librte_eal/ppc/include/rte_altivec.h:#include <altivec.h>
> lib/librte_eal/ppc/include/rte_vect.h:#include <altivec.h>
> 
> 
> > diff --git a/lib/librte_eal/ppc/include/rte_memcpy.h
> b/lib/librte_eal/ppc/include/rte_memcpy.h
> > index 25311ba..d234e21 100644
> > --- a/lib/librte_eal/ppc/include/rte_memcpy.h
> > +++ b/lib/librte_eal/ppc/include/rte_memcpy.h
> > @@ -8,13 +8,12 @@
> >
> >  #include <stdint.h>
> >  #include <string.h>
> > -/*To include altivec.h, GCC version must  >= 4.8 */
> > -#include <altivec.h>
> 
> Why move the inclusion under the __cplusplus check?
> 
Just to make it in the same part as other rte includes.
> 
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> >
> > +#include "rte_altivec.h"
> >  #include "generic/rte_memcpy.h"
> >
> >  static inline void
> > --
> > 1.8.3.1
> >
> 
> Thanks.
> 
> --
> David Marchand
Ori Kam April 30, 2020, 8:54 a.m. UTC | #4 
Hi David,

I will add your code. 

Thanks,
Ori

> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Tuesday, April 28, 2020 9:20 PM
> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Matan Azrad <matan@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com
> Subject: Re: [PATCH] eal/ppc: fix redefine bool type
> 
> > Fixes: 725f5dd0bfb5 ("net/mlx5: fix build on PPC64")
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> 
> There are a couple of other uses that should be covered in the patch:
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> index 5fa92bf92..72bd410fc 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> @@ -13,7 +13,7 @@
>   #include "i40e_rxtx.h"
>   #include "i40e_rxtx_vec_common.h"
> 
> -#include <altivec.h>
> +#include "rte_altivec.h"
> 
>   #pragma GCC diagnostic ignored "-Wcast-qual"
> 
> 
> diff --git a/drivers/net/virtio/virtio_rxtx_simple_altivec.c
> b/drivers/net/virtio/virtio_rxtx_simple_altivec.c
> index 47225f412..b2bdd05c8 100644
> --- a/drivers/net/virtio/virtio_rxtx_simple_altivec.c
> +++ b/drivers/net/virtio/virtio_rxtx_simple_altivec.c
> @@ -9,7 +9,7 @@
>   #include <string.h>
>   #include <errno.h>
> 
> -#include <altivec.h>
> +#include "rte_altivec.h"
> 
>   #include <rte_byteorder.h>
>   #include <rte_branch_prediction.h>
> 
> Dave
David Marchand April 30, 2020, 9:04 a.m. UTC | #5 
On Thu, Apr 30, 2020 at 10:53 AM Ori Kam <orika@mellanox.com> wrote:
> > > diff --git a/lib/librte_eal/ppc/include/rte_memcpy.h
> > b/lib/librte_eal/ppc/include/rte_memcpy.h
> > > index 25311ba..d234e21 100644
> > > --- a/lib/librte_eal/ppc/include/rte_memcpy.h
> > > +++ b/lib/librte_eal/ppc/include/rte_memcpy.h
> > > @@ -8,13 +8,12 @@
> > >
> > >  #include <stdint.h>
> > >  #include <string.h>
> > > -/*To include altivec.h, GCC version must  >= 4.8 */
> > > -#include <altivec.h>
> >
> > Why move the inclusion under the __cplusplus check?
> >
> Just to make it in the same part as other rte includes.

"Normal" rte includes are usually standalone and "#ifdef __cplusplus" safe.
The rte_altivec.h header you added does not need any "#ifdef
__cplusplus" protection, but it might later).

But otoh, "generic/" headers are special/internal headers and this is
why generic/rte_memcpy.h is under this check.

So if there is no reason on your side, please leave rte_altivec.h
inclusion at the same place as the previous altivec.h.


Thanks.
Ori Kam April 30, 2020, 9:06 a.m. UTC | #6 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, April 30, 2020 12:04 PM
> To: Ori Kam <orika@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>; David Christensen
> <drc@linux.vnet.ibm.com>; dev <dev@dpdk.org>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eal/ppc: fix redefine bool type
> 
> On Thu, Apr 30, 2020 at 10:53 AM Ori Kam <orika@mellanox.com> wrote:
> > > > diff --git a/lib/librte_eal/ppc/include/rte_memcpy.h
> > > b/lib/librte_eal/ppc/include/rte_memcpy.h
> > > > index 25311ba..d234e21 100644
> > > > --- a/lib/librte_eal/ppc/include/rte_memcpy.h
> > > > +++ b/lib/librte_eal/ppc/include/rte_memcpy.h
> > > > @@ -8,13 +8,12 @@
> > > >
> > > >  #include <stdint.h>
> > > >  #include <string.h>
> > > > -/*To include altivec.h, GCC version must  >= 4.8 */
> > > > -#include <altivec.h>
> > >
> > > Why move the inclusion under the __cplusplus check?
> > >
> > Just to make it in the same part as other rte includes.
> 
> "Normal" rte includes are usually standalone and "#ifdef __cplusplus" safe.
> The rte_altivec.h header you added does not need any "#ifdef
> __cplusplus" protection, but it might later).
> 
> But otoh, "generic/" headers are special/internal headers and this is
> why generic/rte_memcpy.h is under this check.
> 
> So if there is no reason on your side, please leave rte_altivec.h
> inclusion at the same place as the previous altivec.h.
> 

Sure, I will leave at the original place.

Best,
Ori
> 
> Thanks.
> 
> --
> David Marchand
diff mbox series

Patch

diff --git a/drivers/common/mlx5/mlx5_common.h b/drivers/common/mlx5/mlx5_common.h
index 16de1b3..c2d688a 100644
--- a/drivers/common/mlx5/mlx5_common.h
+++ b/drivers/common/mlx5/mlx5_common.h
@@ -17,16 +17,6 @@ 
 #include "mlx5_prm.h"
 
 
-/*
- * Compilation workaround for PPC64 when AltiVec is fully enabled, e.g. std=c11.
- * Otherwise there would be a type conflict between stdbool and altivec.
- */
-#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
-#undef bool
-/* redefine as in stdbool.h */
-#define bool _Bool
-#endif
-
 /* Bit-field manipulation. */
 #define BITFIELD_DECLARE(bf, type, size) \
 	type bf[(((size_t)(size) / (sizeof(type) * CHAR_BIT)) + \
diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
index d81ace3..0e016f8 100644
--- a/drivers/net/mlx5/mlx5_utils.h
+++ b/drivers/net/mlx5/mlx5_utils.h
@@ -21,16 +21,6 @@ 
 #include "mlx5_defs.h"
 
 
-/*
- * Compilation workaround for PPC64 when AltiVec is fully enabled, e.g. std=c11.
- * Otherwise there would be a type conflict between stdbool and altivec.
- */
-#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
-#undef bool
-/* redefine as in stdbool.h */
-#define bool _Bool
-#endif
-
 /* Convert a bit number to the corresponding 64-bit mask */
 #define MLX5_BITSHIFT(v) (UINT64_C(1) << (v))
 
diff --git a/lib/librte_eal/ppc/include/meson.build b/lib/librte_eal/ppc/include/meson.build
index 3a91c98..3ee38f6 100644
--- a/lib/librte_eal/ppc/include/meson.build
+++ b/lib/librte_eal/ppc/include/meson.build
@@ -4,6 +4,7 @@ 
 includes += include_directories('.')
 
 arch_headers = files(
+	'rte_altivec.h',
 	'rte_atomic.h',
 	'rte_byteorder.h',
 	'rte_cpuflags.h',
diff --git a/lib/librte_eal/ppc/include/rte_altivec.h b/lib/librte_eal/ppc/include/rte_altivec.h
new file mode 100644
index 0000000..1551a94
--- /dev/null
+++ b/lib/librte_eal/ppc/include/rte_altivec.h
@@ -0,0 +1,22 @@ 
+/*
+ * SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (C) Mellanox 2020.
+ */
+
+#ifndef _RTE_ALTIVEC_H_
+#define _RTE_ALTIVEC_H_
+
+/* To include altivec.h, GCC version must be >= 4.8 */
+#include <altivec.h>
+
+/*
+ * Compilation workaround for PPC64 when AltiVec is fully enabled, e.g. std=c11.
+ * Otherwise there would be a type conflict between stdbool and altivec.
+ */
+#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
+#undef bool
+/* redefine as in stdbool.h */
+#define bool _Bool
+#endif
+
+#endif /* _RTE_ALTIVEC_H_ */
diff --git a/lib/librte_eal/ppc/include/rte_memcpy.h b/lib/librte_eal/ppc/include/rte_memcpy.h
index 25311ba..d234e21 100644
--- a/lib/librte_eal/ppc/include/rte_memcpy.h
+++ b/lib/librte_eal/ppc/include/rte_memcpy.h
@@ -8,13 +8,12 @@ 
 
 #include <stdint.h>
 #include <string.h>
-/*To include altivec.h, GCC version must  >= 4.8 */
-#include <altivec.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+#include "rte_altivec.h"
 #include "generic/rte_memcpy.h"
 
 static inline void