[dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

Hi,
Debian and Ubuntu face a build error with 18.08 on ppc64el.
It looks like that:

Full log:
https://buildd.debian.org/status/fetch.php?pkg=dpdk&arch=ppc64el&ver=18.08-1&stamp=1534520196&raw=0

/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c: In function
'mlx5_nl_switch_info_cb':
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:837:23: error: incompatible
types when initializing type '__vector __bool int' {aka '__vector(4) __bool
int'} using type 'int'
  bool port_name_set = false;
                       ^~~~~
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:838:23: error: incompatible
types when initializing type '__vector __bool int' {aka '__vector(4) __bool
int'} using type 'int'
  bool switch_id_set = false;
                       ^~~~~
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:857:18: error: incompatible
types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
int'} from type 'int'
    port_name_set = true;
                  ^
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:865:18: error: incompatible
types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
int'} from type 'int'
    switch_id_set = true;
                  ^
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:16: error: used vector type
where scalar is required
  info.master = switch_id_set && !port_name_set;
                ^~~~~~~~~~~~~
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:33: error: wrong type
argument to unary exclamation mark
  info.master = switch_id_set && !port_name_set;
                                 ^
/<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:871:21: error: used vector type
where scalar is required
  info.representor = switch_id_set && port_name_set;


Now I checked and the reason seems to be some combination of altivec and
MLX headers and the use of bool - probably stdbool vs altivec bool.

If built with gcc -E I see it the bool variables become:
   __attribute__((altivec(bool__))) unsigned port_name_set =

I have found a strawmans approach to it, but I'm sure people with
experience on the matter will come up with something better.

My current change looks like that and would work:
$ git diff
diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
index d61826aea..2cc8f49c5 100644
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void *arg)
               .switch_id = 0,
       };
       size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-       bool port_name_set = false;
-       bool switch_id_set = false;
+       int port_name_set = 0;
+       int switch_id_set = 0;

       if (nh->nlmsg_type != RTM_NEWLINK)
               goto error;
@@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void *arg)
                       if (errno ||
                           (size_t)(end - (char *)payload) !=
strlen(payload))
                               goto error;
-                       port_name_set = true;
+                       port_name_set = 1;
                       break;
               case IFLA_PHYS_SWITCH_ID:
                       info.switch_id = 0;
@@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void *arg)
                               info.switch_id <<= 8;
                               info.switch_id |= ((uint8_t *)payload)[i];
                       }
-                       switch_id_set = true;
+                       switch_id_set = 1;
                       break;
               }
               off += RTA_ALIGN(ra->rta_len);




-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

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).


Description: Fix ppc64le build error between altivec and bool

We really hope there will eventually be a better fix for this, but
currently
we have to unbreak building this code so until something better is
available
let's use this modification.

Forwarded: yes
Forward-info: http://mails.dpdk.org/archives/dev/2018-August/109926.html
Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Last-Update: 2018-08-22
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
               .switch_id = 0,
       };
       size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-       bool port_name_set = false;
-       bool switch_id_set = false;
+       int port_name_set = 0;
+       int switch_id_set = 0;

       if (nh->nlmsg_type != RTM_NEWLINK)
               goto error;
@@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
                       if (errno ||
                           (size_t)(end - (char *)payload) !=
strlen(payload))
                               goto error;
-                       port_name_set = true;
+                       port_name_set = 1;
                       break;
               case IFLA_PHYS_SWITCH_ID:
                       info.switch_id = 0;
@@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
                               info.switch_id <<= 8;
                               info.switch_id |= ((uint8_t *)payload)[i];
                       }
-                       switch_id_set = true;
+                       switch_id_set = 1;
                       break;
               }
               off += RTA_ALIGN(ra->rta_len);
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1335,8 +1335,8 @@ mlx5_sysfs_switch_info(unsigned int ifin
       char ifname[IF_NAMESIZE];
       FILE *file;
       struct mlx5_switch_info data = { .master = 0, };
-       bool port_name_set = false;
-       bool port_switch_id_set = false;
+       int port_name_set = 0;
+       int port_switch_id_set = 0;
       char c;

       if (!if_indextoname(ifindex, ifname)) {
--- a/drivers/net/mlx5/mlx5_nl_flow.c
+++ b/drivers/net/mlx5/mlx5_nl_flow.c
@@ -385,11 +385,11 @@ mlx5_nl_flow_transpose(void *buf,
       const struct rte_flow_action *action;
       unsigned int n;
       uint32_t act_index_cur;
-       bool in_port_id_set;
-       bool eth_type_set;
-       bool vlan_present;
-       bool vlan_eth_type_set;
-       bool ip_proto_set;
+       int in_port_id_set;
+       int eth_type_set;
+       int vlan_present;
+       int vlan_eth_type_set;
+       int ip_proto_set;
       struct nlattr *na_flower;
       struct nlattr *na_flower_act;
       struct nlattr *na_vlan_id;
@@ -404,11 +404,11 @@ init:
       action = actions;
       n = 0;
       act_index_cur = 0;
-       in_port_id_set = false;
-       eth_type_set = false;
-       vlan_present = false;
-       vlan_eth_type_set = false;
-       ip_proto_set = false;
+       in_port_id_set = 0;
+       eth_type_set = 0;
+       vlan_present = 0;
+       vlan_eth_type_set = 0;
+       ip_proto_set = 0;
       na_flower = NULL;
       na_flower_act = NULL;
       na_vlan_id = NULL;


On Tue, Aug 21, 2018 at 4:19 PM Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:

> Hi,
> Debian and Ubuntu face a build error with 18.08 on ppc64el.
> It looks like that:
>
> Full log:
>
> https://buildd.debian.org/status/fetch.php?pkg=dpdk&arch=ppc64el&ver=18.08-1&stamp=1534520196&raw=0
>
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c: In function
> 'mlx5_nl_switch_info_cb':
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:837:23: error: incompatible
> types when initializing type '__vector __bool int' {aka '__vector(4) __bool
> int'} using type 'int'
>   bool port_name_set = false;
>                        ^~~~~
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:838:23: error: incompatible
> types when initializing type '__vector __bool int' {aka '__vector(4) __bool
> int'} using type 'int'
>   bool switch_id_set = false;
>                        ^~~~~
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:857:18: error: incompatible
> types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
> int'} from type 'int'
>     port_name_set = true;
>                   ^
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:865:18: error: incompatible
> types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
> int'} from type 'int'
>     switch_id_set = true;
>                   ^
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:16: error: used vector
> type where scalar is required
>   info.master = switch_id_set && !port_name_set;
>                 ^~~~~~~~~~~~~
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:33: error: wrong type
> argument to unary exclamation mark
>   info.master = switch_id_set && !port_name_set;
>                                  ^
> /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:871:21: error: used vector
> type where scalar is required
>   info.representor = switch_id_set && port_name_set;
>
>
> Now I checked and the reason seems to be some combination of altivec and
> MLX headers and the use of bool - probably stdbool vs altivec bool.
>
> If built with gcc -E I see it the bool variables become:
>    __attribute__((altivec(bool__))) unsigned port_name_set =
>
> I have found a strawmans approach to it, but I'm sure people with
> experience on the matter will come up with something better.
>
> My current change looks like that and would work:
> $ git diff
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
> index d61826aea..2cc8f49c5 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> *arg)
>                .switch_id = 0,
>        };
>        size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -       bool port_name_set = false;
> -       bool switch_id_set = false;
> +       int port_name_set = 0;
> +       int switch_id_set = 0;
>
>        if (nh->nlmsg_type != RTM_NEWLINK)
>                goto error;
> @@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> *arg)
>                        if (errno ||
>                            (size_t)(end - (char *)payload) !=
> strlen(payload))
>                                goto error;
> -                       port_name_set = true;
> +                       port_name_set = 1;
>                        break;
>                case IFLA_PHYS_SWITCH_ID:
>                        info.switch_id = 0;
> @@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> *arg)
>                                info.switch_id <<= 8;
>                                info.switch_id |= ((uint8_t *)payload)[i];
>                        }
> -                       switch_id_set = true;
> +                       switch_id_set = 1;
>                        break;
>                }
>                off += RTA_ALIGN(ra->rta_len);
>
>
>
>
> --
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Adrien Mazarguil]

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.

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.

   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.

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.

3. Replace bool with _Bool to impacted users of stdbool.h. Basically what
   you did below with "int" but slightly more correct since true/false can
   still be used with _Bool. A comment explaining why is necessary after the
   inclusion of stdbool.h.

Can you validate these suggestions? I don't have the right setup for that.

Thanks.

> Description: Fix ppc64le build error between altivec and bool
> 
> We really hope there will eventually be a better fix for this, but
> currently
> we have to unbreak building this code so until something better is
> available
> let's use this modification.
> 
> Forwarded: yes
> Forward-info: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Last-Update: 2018-08-22
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
>                .switch_id = 0,
>        };
>        size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -       bool port_name_set = false;
> -       bool switch_id_set = false;
> +       int port_name_set = 0;
> +       int switch_id_set = 0;
> 
>        if (nh->nlmsg_type != RTM_NEWLINK)
>                goto error;
> @@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
>                        if (errno ||
>                            (size_t)(end - (char *)payload) !=
> strlen(payload))
>                                goto error;
> -                       port_name_set = true;
> +                       port_name_set = 1;
>                        break;
>                case IFLA_PHYS_SWITCH_ID:
>                        info.switch_id = 0;
> @@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
>                                info.switch_id <<= 8;
>                                info.switch_id |= ((uint8_t *)payload)[i];
>                        }
> -                       switch_id_set = true;
> +                       switch_id_set = 1;
>                        break;
>                }
>                off += RTA_ALIGN(ra->rta_len);
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1335,8 +1335,8 @@ mlx5_sysfs_switch_info(unsigned int ifin
>        char ifname[IF_NAMESIZE];
>        FILE *file;
>        struct mlx5_switch_info data = { .master = 0, };
> -       bool port_name_set = false;
> -       bool port_switch_id_set = false;
> +       int port_name_set = 0;
> +       int port_switch_id_set = 0;
>        char c;
> 
>        if (!if_indextoname(ifindex, ifname)) {
> --- a/drivers/net/mlx5/mlx5_nl_flow.c
> +++ b/drivers/net/mlx5/mlx5_nl_flow.c
> @@ -385,11 +385,11 @@ mlx5_nl_flow_transpose(void *buf,
>        const struct rte_flow_action *action;
>        unsigned int n;
>        uint32_t act_index_cur;
> -       bool in_port_id_set;
> -       bool eth_type_set;
> -       bool vlan_present;
> -       bool vlan_eth_type_set;
> -       bool ip_proto_set;
> +       int in_port_id_set;
> +       int eth_type_set;
> +       int vlan_present;
> +       int vlan_eth_type_set;
> +       int ip_proto_set;
>        struct nlattr *na_flower;
>        struct nlattr *na_flower_act;
>        struct nlattr *na_vlan_id;
> @@ -404,11 +404,11 @@ init:
>        action = actions;
>        n = 0;
>        act_index_cur = 0;
> -       in_port_id_set = false;
> -       eth_type_set = false;
> -       vlan_present = false;
> -       vlan_eth_type_set = false;
> -       ip_proto_set = false;
> +       in_port_id_set = 0;
> +       eth_type_set = 0;
> +       vlan_present = 0;
> +       vlan_eth_type_set = 0;
> +       ip_proto_set = 0;
>        na_flower = NULL;
>        na_flower_act = NULL;
>        na_vlan_id = NULL;
> 
> 
> On Tue, Aug 21, 2018 at 4:19 PM Christian Ehrhardt <
> christian.ehrhardt@canonical.com> wrote:
> 
> > Hi,
> > Debian and Ubuntu face a build error with 18.08 on ppc64el.
> > It looks like that:
> >
> > Full log:
> >
> > https://buildd.debian.org/status/fetch.php?pkg=dpdk&arch=ppc64el&ver=18.08-1&stamp=1534520196&raw=0
> >
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c: In function
> > 'mlx5_nl_switch_info_cb':
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:837:23: error: incompatible
> > types when initializing type '__vector __bool int' {aka '__vector(4) __bool
> > int'} using type 'int'
> >   bool port_name_set = false;
> >                        ^~~~~
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:838:23: error: incompatible
> > types when initializing type '__vector __bool int' {aka '__vector(4) __bool
> > int'} using type 'int'
> >   bool switch_id_set = false;
> >                        ^~~~~
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:857:18: error: incompatible
> > types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
> > int'} from type 'int'
> >     port_name_set = true;
> >                   ^
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:865:18: error: incompatible
> > types when assigning to type '__vector __bool int' {aka '__vector(4) __bool
> > int'} from type 'int'
> >     switch_id_set = true;
> >                   ^
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:16: error: used vector
> > type where scalar is required
> >   info.master = switch_id_set && !port_name_set;
> >                 ^~~~~~~~~~~~~
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:33: error: wrong type
> > argument to unary exclamation mark
> >   info.master = switch_id_set && !port_name_set;
> >                                  ^
> > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:871:21: error: used vector
> > type where scalar is required
> >   info.representor = switch_id_set && port_name_set;
> >
> >
> > Now I checked and the reason seems to be some combination of altivec and
> > MLX headers and the use of bool - probably stdbool vs altivec bool.
> >
> > If built with gcc -E I see it the bool variables become:
> >    __attribute__((altivec(bool__))) unsigned port_name_set =
> >
> > I have found a strawmans approach to it, but I'm sure people with
> > experience on the matter will come up with something better.
> >
> > My current change looks like that and would work:
> > $ git diff
> > diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
> > index d61826aea..2cc8f49c5 100644
> > --- a/drivers/net/mlx5/mlx5_nl.c
> > +++ b/drivers/net/mlx5/mlx5_nl.c
> > @@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > *arg)
> >                .switch_id = 0,
> >        };
> >        size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > -       bool port_name_set = false;
> > -       bool switch_id_set = false;
> > +       int port_name_set = 0;
> > +       int switch_id_set = 0;
> >
> >        if (nh->nlmsg_type != RTM_NEWLINK)
> >                goto error;
> > @@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > *arg)
> >                        if (errno ||
> >                            (size_t)(end - (char *)payload) !=
> > strlen(payload))
> >                                goto error;
> > -                       port_name_set = true;
> > +                       port_name_set = 1;
> >                        break;
> >                case IFLA_PHYS_SWITCH_ID:
> >                        info.switch_id = 0;
> > @@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > *arg)
> >                                info.switch_id <<= 8;
> >                                info.switch_id |= ((uint8_t *)payload)[i];
> >                        }
> > -                       switch_id_set = true;
> > +                       switch_id_set = 1;
> >                        break;
> >                }
> >                off += RTA_ALIGN(ra->rta_len);

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

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 <FOO>"
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 <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 ‘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__
<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=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 <string.h>
/*To include altivec.h, GCC version must  >= 4.8 */
#include <altivec.h>
+/*
+ * 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"?



> 3. Replace bool with _Bool to impacted users of stdbool.h. Basically what
>    you did below with "int" but slightly more correct since true/false can
>    still be used with _Bool. A comment explaining why is necessary after
> the
>    inclusion of stdbool.h.
>

Could be an option, but I think this just means others converting code will
stumble over it again and again.
Especially if one tests x86 only and then ppc64 breaks later (which IMHO is
what happened here).

Can you validate these suggestions? I don't have the right setup for that.
>

I only can grab a ppc box if one is free, but currently it seems I'm lucky
:-)


Thanks.
>
> > Description: Fix ppc64le build error between altivec and bool
> >
> > We really hope there will eventually be a better fix for this, but
> > currently
> > we have to unbreak building this code so until something better is
> > available
> > let's use this modification.
> >
> > Forwarded: yes
> > Forward-info: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> > Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > Last-Update: 2018-08-22
> > --- a/drivers/net/mlx5/mlx5_nl.c
> > +++ b/drivers/net/mlx5/mlx5_nl.c
> > @@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
> >                .switch_id = 0,
> >        };
> >        size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > -       bool port_name_set = false;
> > -       bool switch_id_set = false;
> > +       int port_name_set = 0;
> > +       int switch_id_set = 0;
> >
> >        if (nh->nlmsg_type != RTM_NEWLINK)
> >                goto error;
> > @@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
> >                        if (errno ||
> >                            (size_t)(end - (char *)payload) !=
> > strlen(payload))
> >                                goto error;
> > -                       port_name_set = true;
> > +                       port_name_set = 1;
> >                        break;
> >                case IFLA_PHYS_SWITCH_ID:
> >                        info.switch_id = 0;
> > @@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *
> >                                info.switch_id <<= 8;
> >                                info.switch_id |= ((uint8_t *)payload)[i];
> >                        }
> > -                       switch_id_set = true;
> > +                       switch_id_set = 1;
> >                        break;
> >                }
> >                off += RTA_ALIGN(ra->rta_len);
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1335,8 +1335,8 @@ mlx5_sysfs_switch_info(unsigned int ifin
> >        char ifname[IF_NAMESIZE];
> >        FILE *file;
> >        struct mlx5_switch_info data = { .master = 0, };
> > -       bool port_name_set = false;
> > -       bool port_switch_id_set = false;
> > +       int port_name_set = 0;
> > +       int port_switch_id_set = 0;
> >        char c;
> >
> >        if (!if_indextoname(ifindex, ifname)) {
> > --- a/drivers/net/mlx5/mlx5_nl_flow.c
> > +++ b/drivers/net/mlx5/mlx5_nl_flow.c
> > @@ -385,11 +385,11 @@ mlx5_nl_flow_transpose(void *buf,
> >        const struct rte_flow_action *action;
> >        unsigned int n;
> >        uint32_t act_index_cur;
> > -       bool in_port_id_set;
> > -       bool eth_type_set;
> > -       bool vlan_present;
> > -       bool vlan_eth_type_set;
> > -       bool ip_proto_set;
> > +       int in_port_id_set;
> > +       int eth_type_set;
> > +       int vlan_present;
> > +       int vlan_eth_type_set;
> > +       int ip_proto_set;
> >        struct nlattr *na_flower;
> >        struct nlattr *na_flower_act;
> >        struct nlattr *na_vlan_id;
> > @@ -404,11 +404,11 @@ init:
> >        action = actions;
> >        n = 0;
> >        act_index_cur = 0;
> > -       in_port_id_set = false;
> > -       eth_type_set = false;
> > -       vlan_present = false;
> > -       vlan_eth_type_set = false;
> > -       ip_proto_set = false;
> > +       in_port_id_set = 0;
> > +       eth_type_set = 0;
> > +       vlan_present = 0;
> > +       vlan_eth_type_set = 0;
> > +       ip_proto_set = 0;
> >        na_flower = NULL;
> >        na_flower_act = NULL;
> >        na_vlan_id = NULL;
> >
> >
> > On Tue, Aug 21, 2018 at 4:19 PM Christian Ehrhardt <
> > christian.ehrhardt@canonical.com> wrote:
> >
> > > Hi,
> > > Debian and Ubuntu face a build error with 18.08 on ppc64el.
> > > It looks like that:
> > >
> > > Full log:
> > >
> > >
> https://buildd.debian.org/status/fetch.php?pkg=dpdk&arch=ppc64el&ver=18.08-1&stamp=1534520196&raw=0
> > >
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c: In function
> > > 'mlx5_nl_switch_info_cb':
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:837:23: error: incompatible
> > > types when initializing type '__vector __bool int' {aka '__vector(4)
> __bool
> > > int'} using type 'int'
> > >   bool port_name_set = false;
> > >                        ^~~~~
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:838:23: error: incompatible
> > > types when initializing type '__vector __bool int' {aka '__vector(4)
> __bool
> > > int'} using type 'int'
> > >   bool switch_id_set = false;
> > >                        ^~~~~
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:857:18: error: incompatible
> > > types when assigning to type '__vector __bool int' {aka '__vector(4)
> __bool
> > > int'} from type 'int'
> > >     port_name_set = true;
> > >                   ^
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:865:18: error: incompatible
> > > types when assigning to type '__vector __bool int' {aka '__vector(4)
> __bool
> > > int'} from type 'int'
> > >     switch_id_set = true;
> > >                   ^
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:16: error: used vector
> > > type where scalar is required
> > >   info.master = switch_id_set && !port_name_set;
> > >                 ^~~~~~~~~~~~~
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:870:33: error: wrong type
> > > argument to unary exclamation mark
> > >   info.master = switch_id_set && !port_name_set;
> > >                                  ^
> > > /<<PKGBUILDDIR>>/drivers/net/mlx5/mlx5_nl.c:871:21: error: used vector
> > > type where scalar is required
> > >   info.representor = switch_id_set && port_name_set;
> > >
> > >
> > > Now I checked and the reason seems to be some combination of altivec
> and
> > > MLX headers and the use of bool - probably stdbool vs altivec bool.
> > >
> > > If built with gcc -E I see it the bool variables become:
> > >    __attribute__((altivec(bool__))) unsigned port_name_set =
> > >
> > > I have found a strawmans approach to it, but I'm sure people with
> > > experience on the matter will come up with something better.
> > >
> > > My current change looks like that and would work:
> > > $ git diff
> > > diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
> > > index d61826aea..2cc8f49c5 100644
> > > --- a/drivers/net/mlx5/mlx5_nl.c
> > > +++ b/drivers/net/mlx5/mlx5_nl.c
> > > @@ -834,8 +834,8 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > > *arg)
> > >                .switch_id = 0,
> > >        };
> > >        size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > > -       bool port_name_set = false;
> > > -       bool switch_id_set = false;
> > > +       int port_name_set = 0;
> > > +       int switch_id_set = 0;
> > >
> > >        if (nh->nlmsg_type != RTM_NEWLINK)
> > >                goto error;
> > > @@ -854,7 +854,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > > *arg)
> > >                        if (errno ||
> > >                            (size_t)(end - (char *)payload) !=
> > > strlen(payload))
> > >                                goto error;
> > > -                       port_name_set = true;
> > > +                       port_name_set = 1;
> > >                        break;
> > >                case IFLA_PHYS_SWITCH_ID:
> > >                        info.switch_id = 0;
> > > @@ -862,7 +862,7 @@ mlx5_nl_switch_info_cb(struct nlmsghdr *nh, void
> > > *arg)
> > >                                info.switch_id <<= 8;
> > >                                info.switch_id |= ((uint8_t
> *)payload)[i];
> > >                        }
> > > -                       switch_id_set = true;
> > > +                       switch_id_set = 1;
> > >                        break;
> > >                }
> > >                off += RTA_ALIGN(ra->rta_len);
>
> --
> Adrien Mazarguil
> 6WIND
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Adrien Mazarguil]

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 <FOO>"
> 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 <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 ‘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__
> <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=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 <string.h>
> /*To include altivec.h, GCC version must  >= 4.8 */
> #include <altivec.h>
> +/*
> + * 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 <altivec.h>

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

On Tue, Aug 28, 2018 at 1:44 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
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 <FOO>"
> > 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 <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 ‘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__
> > <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=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 <string.h>
> > /*To include altivec.h, GCC version must  >= 4.8 */
> > #include <altivec.h>
> > +/*
> > + * 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 <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  >= 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 <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: 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.

$ 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  >= 4.8 */
#include <altivec.h>
+/*
+ * 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.


-- 
> Adrien Mazarguil
> 6WIND
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Adrien Mazarguil]

On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
> On Tue, Aug 28, 2018 at 1:44 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 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 <FOO>"
> > > 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 <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 ‘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__
> > > <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=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 <string.h>
> > > /*To include altivec.h, GCC version must  >= 4.8 */
> > > #include <altivec.h>
> > > +/*
> > > + * 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 <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  >= 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 <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: 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 <string.h>
> /*To include altivec.h, GCC version must  >= 4.8 */
> #include <altivec.h>
> +/*
> + * 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

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
wrote:

> On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
> > On Tue, Aug 28, 2018 at 1:44 PM Adrien Mazarguil <
> adrien.mazarguil@6wind.com>
> > 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 <FOO>"
> > > > 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 <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 ‘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__
> > > > <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=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 <string.h>
> > > > /*To include altivec.h, GCC version must  >= 4.8 */
> > > > #include <altivec.h>
> > > > +/*
> > > > + * 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 <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  >= 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 <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:
> 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.
>

While "pushing the internal knowledge out to users" sounds right at first.
There are far too many IMHO, the change would be huge unclean and messy.

$ grep -Hrn altivec.h
drivers/net/i40e/i40e_rxtx_vec_altivec.c:45:#include <altivec.h>
examples/l3fwd/l3fwd_lpm.c:165:#include "l3fwd_lpm_altivec.h"
examples/l3fwd/l3fwd_lpm_altivec.h:10:#include "l3fwd_altivec.h"
MAINTAINERS:239:F: examples/l3fwd/*altivec.h
lib/librte_acl/acl_run_altivec.c:34:#include "acl_run_altivec.h"
lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:49:/*To include
altivec.h, GCC version must  >= 4.8 */
lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:50:#include <
altivec.h>
lib/librte_eal/common/include/arch/ppc_64/rte_vect.h:36:#include <altivec.h>

lib/librte_lpm/meson.build:9:headers += files('rte_lpm_altivec.h',
'rte_lpm_neon.h', 'rte_lpm_sse.h')
lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=
rte_lpm_altivec.h
lib/librte_lpm/rte_lpm.h:461:#include "rte_lpm_altivec.h"


> 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.
>

Yeah Independent to the other part of the discussion I think we can make it
generally apply and not just C11.

The following "would work" in the code right now.
--- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
@@ -35,6 +35,21 @@

#include <stdint.h>
#include <string.h>
+/*
+ * if built with newer C standard like -std=c11 stdbool.h bool and altivec
+ * bool types will conflict. We have to force altivec users (rte_vect.h and
+ * rte_memcpy.h) rely on __vector implying internal altivec knowledge to
the
+ * users but keeping things transparent for all others.
+ * Therefore define __APPLE_ALTIVEC__ which make it not (re-)define the
+ * non prefixed types lile "bool".
+ * While we need to disambiguise bool, we want vector/pixel to stay as-is
+ * in those cases so define them as altivec.h would.
+ */
+#ifndef __APPLE_ALTIVEC__
+#define __APPLE_ALTIVEC__ 1
+#define vector __vector
+#define pixel __pixel
+#endif
/*To include altivec.h, GCC version must  >= 4.8 */
#include <altivec.h>

But here again, ordering could make altivec.h be included before this
statement in rte_memcpy.

I would not want to see us search and replace all occurrence of "vector"
nor doing the same trick all over the place at every include of altivec.h
How about the following which should address the follwing:
- resolve the issue with stbool conflicting
- no issues with vector types as it retains what would be defined
- have the workaround in just one place
- independent to include order as long as rte_altivec.h is uses instead of
a direct include

diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
index f3fc8267..31dc6839 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
@@ -42,7 +42,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/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index cca68826..cab13f1d 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -17,6 +17,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h
INC += rte_service.h rte_service_component.h
INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
+INC += rte_altivec.h

GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
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 75f74897..225de7a0 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
@@ -35,8 +35,8 @@

#include <stdint.h>
#include <string.h>
-/*To include altivec.h, GCC version must  >= 4.8 */
-#include <altivec.h>
+
+#include <rte_altivec.h>

#ifdef __cplusplus
extern "C" {
diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
index 99586e58..1ac81d73 100644
--- a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
+++ b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
@@ -33,7 +33,7 @@
#ifndef _RTE_VECT_PPC_64_H_
#define _RTE_VECT_PPC_64_H_

-#include <altivec.h>
+#include <rte_altivec.h>
#include "generic/rte_vect.h"

#ifdef __cplusplus
diff --git a/lib/librte_eal/common/include/rte_altivec.h
b/lib/librte_eal/common/include/rte_altivec.h
new file mode 100644
index 00000000..e620e701
--- /dev/null
+++ b/lib/librte_eal/common/include/rte_altivec.h
@@ -0,0 +1,60 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright 2018 Canonical Ltd.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of NXP nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+
+#ifndef _RTE_EAL_ALTIVEC_H_
+#define _RTE_EAL_ALTIVEC_H_
+
+/*
+ * if built with newer C standard like -std=c11 stdbool.h bool and altivec
+ * bool types will conflict.
+ * There is no user of the altivec bool type, so make sure it never is
+ * defined. Therefore define __APPLE_ALTIVEC__ which make it not
(re-)define
+ * the non prefixed types lile "bool".
+ * On the other hand other types like vector are used, so while we need to
+ * disambiguise type bool, we want vector/pixel to stay as-is so we define
+ * them as altivec.h would.
+ * Note: without -std= the compiler has all those as context sensitive
+ * keywords and the header will not define them at all. Therefore the
+ * compiler has __APPLE_ALTIVEC__ already set in those cases - therefore we
+ * don't touch things if __APPLE_ALTIVEC__ is already set.
+ */
+#ifndef __APPLE_ALTIVEC__
+#define __APPLE_ALTIVEC__ 1
+#define vector __vector
+#define pixel __pixel
+#endif
+
+/*To include altivec.h, GCC version must  >= 4.8 */
+#include <altivec.h>
+
+#endif /* _RTE_EAL_ALTIVEC_H_ */
diff --git a/lib/librte_eal/common/meson.build
b/lib/librte_eal/common/meson.build
index 56005bea..616f2084 100644
--- a/lib/librte_eal/common/meson.build
+++ b/lib/librte_eal/common/meson.build
@@ -45,6 +45,7 @@ common_objs += eal_common_arch_objs

common_headers = files(
       'include/rte_alarm.h',
+       'include/rte_altivec.h',
       'include/rte_branch_prediction.h',
       'include/rte_bus.h',
       'include/rte_bitmap.h',


> $ 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  >= 4.8 */
> > #include <altivec.h>
> > +/*
> > + * 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
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Adrien Mazarguil]

On Wed, Aug 29, 2018 at 10:27:03AM +0200, Christian Ehrhardt wrote:
> On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
> wrote:
> 
> > On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
<trimming thread a bit>
> > > --- 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  >= 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 <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:
> > 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.
> >
> 
> While "pushing the internal knowledge out to users" sounds right at first.
> There are far too many IMHO, the change would be huge unclean and messy.
> 
> $ grep -Hrn altivec.h
> drivers/net/i40e/i40e_rxtx_vec_altivec.c:45:#include <altivec.h>
> examples/l3fwd/l3fwd_lpm.c:165:#include "l3fwd_lpm_altivec.h"
> examples/l3fwd/l3fwd_lpm_altivec.h:10:#include "l3fwd_altivec.h"
> MAINTAINERS:239:F: examples/l3fwd/*altivec.h
> lib/librte_acl/acl_run_altivec.c:34:#include "acl_run_altivec.h"
> lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:49:/*To include
> altivec.h, GCC version must  >= 4.8 */
> lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:50:#include <
> altivec.h>
> lib/librte_eal/common/include/arch/ppc_64/rte_vect.h:36:#include <altivec.h>
> 
> lib/librte_lpm/meson.build:9:headers += files('rte_lpm_altivec.h',
> 'rte_lpm_neon.h', 'rte_lpm_sse.h')
> lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=
> rte_lpm_altivec.h
> lib/librte_lpm/rte_lpm.h:461:#include "rte_lpm_altivec.h"

I'd still like to give it a try given only knwon users of AltiVec code may
rely on these vector/pixel/bool definitions. Scope should be quite small.

The root issue we need to address is that DPDK applications may
involuntarily pull altivec.h by including something unrelated (rte_memcpy.h)
and get unwanted bool/vector/pixel macros polluting their namespace and
breaking things.

> > 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.
> >
> 
> Yeah Independent to the other part of the discussion I think we can make it
> generally apply and not just C11.
> 
> The following "would work" in the code right now.
> --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> @@ -35,6 +35,21 @@
> 
> #include <stdint.h>
> #include <string.h>
> +/*
> + * if built with newer C standard like -std=c11 stdbool.h bool and altivec
> + * bool types will conflict. We have to force altivec users (rte_vect.h and
> + * rte_memcpy.h) rely on __vector implying internal altivec knowledge to
> the
> + * users but keeping things transparent for all others.
> + * Therefore define __APPLE_ALTIVEC__ which make it not (re-)define the
> + * non prefixed types lile "bool".
> + * While we need to disambiguise bool, we want vector/pixel to stay as-is
> + * in those cases so define them as altivec.h would.
> + */
> +#ifndef __APPLE_ALTIVEC__
> +#define __APPLE_ALTIVEC__ 1
> +#define vector __vector
> +#define pixel __pixel
> +#endif
> /*To include altivec.h, GCC version must  >= 4.8 */
> #include <altivec.h>
> 
> But here again, ordering could make altivec.h be included before this
> statement in rte_memcpy.

Another possible issue: Clang's altivec.h differs from that of GCC.

It doesn't have __APPLE_ALTIVEC__ and doesn't define macros for
bool/vector/pixel, which is good as they only exist as context-aware
compiler keywords with -maltivec, however I don't see instances of __pixel
or __bool beside __vector in that file. This should be carefully tested to
make sure the __ prefix is supported by both compilers.

> I would not want to see us search and replace all occurrence of "vector"
> nor doing the same trick all over the place at every include of altivec.h
> How about the following which should address the follwing:
> - resolve the issue with stbool conflicting
> - no issues with vector types as it retains what would be defined
> - have the workaround in just one place
> - independent to include order as long as rte_altivec.h is uses instead of
> a direct include

I like rte_altivec.h. It's explicit and easy to make sure it remains the
only user of altivec.h in DPDK. However due to the remaining issues with
these macros, I still believe we should address them all at once.

So let's take a slighly different approach. Assuming users of altivec.h know
what they are doing, stdbool.h and altivec.h conflicts and the compiler
flags they use is their problem. We only need to help those that didn't ask
for altivec.h and get infected by it through header dependencies.

Normally, -maltivec is all that's needed to get harmless bool/vector/pixel
context-sensitive keywords (even without including altivec.h) as expected by
applications, however no one ever expects these to be harmful macros as is
the case when compiling with GCC in ISO C mode.

In short, public headers that include altivec.h need to clean the mess after
themselves transparently. So here's a different suggestion for
rte_altivec.h:

///

 #ifndef RTE_ALTIVEC_H_
 #define RTE_ALTIVEC_H_

 #ifdef bool
 #define RTE_ALTIVEC_H_ORIG_BOOL bool
 #undef bool
 #endif

 #ifdef vector
 #define RTE_ALTIVEC_H_ORIG_VECTOR vector
 #undef vector
 #endif

 #ifdef pixel
 #define RTE_ALTIVEC_H_ORIG_PIXEL pixel
 #undef pixel
 #endif

 #include <altivec.h>

 #undef bool
 #undef vector
 #undef pixel

 #ifdef RTE_ALTIVEC_H_ORIG_BOOL
 #define bool RTE_ALTIVEC_H_ORIG_BOOL
 #undef RTE_ALTIVEC_H_ORIG_BOOL
 #endif

 #ifdef RTE_ALTIVEC_H_ORIG_VECTOR
 #define vector RTE_ALTIVEC_H_ORIG_VECTOR
 #undef RTE_ALTIVEC_H_ORIG_VECTOR
 #endif

 #ifdef RTE_ALTIVEC_H_ORIG_PIXEL
 #define pixel RTE_ALTIVEC_H_ORIG_PIXEL
 #undef RTE_ALTIVEC_H_ORIG_PIXEL

///

With public headers such as rte_vect.h and rte_memcpy.h modified to use
rte_altivec.h and rely on __vector and __bool where relevant. Applications
can continue to rely on altivec.h and non-prefixed counterparts as usual.

That way, applications that absolutely want to combine ISO C and altivec.h
yet still get bool/vector/pixel macros only have to make sure it's included
before any DPDK headers. This shouldn't be a problem for the vast majority.

What's your opinion?

Now given the size of this mess, I'm starting to think that the quick &
dirty workaround in mlx5 doesn't look that bad after all. Files that rely on
stdbool.h only need something like this after #include directives:

 /* Compilation workaround for PPC targets when AltiVec is enabled. */
 #undef bool
 #define bool _Bool

I'm fine with that if it's OK for you.

A few unrelated minor comments about your patch below.

> diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> index f3fc8267..31dc6839 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> @@ -42,7 +42,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/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index cca68826..cab13f1d 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -17,6 +17,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h
> INC += rte_service.h rte_service_component.h
> INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
> INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
> +INC += rte_altivec.h
> 
> GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
> GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
> 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 75f74897..225de7a0 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
> @@ -35,8 +35,8 @@
> 
> #include <stdint.h>
> #include <string.h>
> -/*To include altivec.h, GCC version must  >= 4.8 */
> -#include <altivec.h>
> +
> +#include <rte_altivec.h>
> 
> #ifdef __cplusplus
> extern "C" {
> diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> index 99586e58..1ac81d73 100644
> --- a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> @@ -33,7 +33,7 @@
> #ifndef _RTE_VECT_PPC_64_H_
> #define _RTE_VECT_PPC_64_H_
> 
> -#include <altivec.h>
> +#include <rte_altivec.h>
> #include "generic/rte_vect.h"
> 
> #ifdef __cplusplus
> diff --git a/lib/librte_eal/common/include/rte_altivec.h
> b/lib/librte_eal/common/include/rte_altivec.h
> new file mode 100644
> index 00000000..e620e701
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_altivec.h
> @@ -0,0 +1,60 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright 2018 Canonical Ltd.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

Make sure to use the SPDX format for the copyright notice, see other
headers.

> +#ifndef _RTE_EAL_ALTIVEC_H_
> +#define _RTE_EAL_ALTIVEC_H_
> +
> +/*
> + * if built with newer C standard like -std=c11 stdbool.h bool and altivec
> + * bool types will conflict.
> + * There is no user of the altivec bool type, so make sure it never is
> + * defined. Therefore define __APPLE_ALTIVEC__ which make it not
> (re-)define
> + * the non prefixed types lile "bool".
> + * On the other hand other types like vector are used, so while we need to
> + * disambiguise type bool, we want vector/pixel to stay as-is so we define
> + * them as altivec.h would.
> + * Note: without -std= the compiler has all those as context sensitive
> + * keywords and the header will not define them at all. Therefore the
> + * compiler has __APPLE_ALTIVEC__ already set in those cases - therefore we
> + * don't touch things if __APPLE_ALTIVEC__ is already set.
> + */
> +#ifndef __APPLE_ALTIVEC__
> +#define __APPLE_ALTIVEC__ 1
> +#define vector __vector
> +#define pixel __pixel
> +#endif
> +
> +/*To include altivec.h, GCC version must  >= 4.8 */

This comment can probably be dropped given Clang also ships altivec.h and
GCC <= 4.8 is not actively supported anymore (sys_reqs.rst).

> +#include <altivec.h>
> +
> +#endif /* _RTE_EAL_ALTIVEC_H_ */
> diff --git a/lib/librte_eal/common/meson.build
> b/lib/librte_eal/common/meson.build
> index 56005bea..616f2084 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -45,6 +45,7 @@ common_objs += eal_common_arch_objs
> 
> common_headers = files(
>        'include/rte_alarm.h',
> +       'include/rte_altivec.h',
>        'include/rte_branch_prediction.h',
>        'include/rte_bus.h',
>        'include/rte_bitmap.h',

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

On Wed, Aug 29, 2018 at 3:16 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
wrote:

> On Wed, Aug 29, 2018 at 10:27:03AM +0200, Christian Ehrhardt wrote:
> > On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil <
> adrien.mazarguil@6wind.com>
> > wrote:
> >
> > > On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
> <trimming thread a bit>
> > > > --- 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  >= 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 <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:
> > > 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.
> > >
> >
> > While "pushing the internal knowledge out to users" sounds right at
> first.
> > There are far too many IMHO, the change would be huge unclean and messy.
> >
> > $ grep -Hrn altivec.h
> > drivers/net/i40e/i40e_rxtx_vec_altivec.c:45:#include <altivec.h>
> > examples/l3fwd/l3fwd_lpm.c:165:#include "l3fwd_lpm_altivec.h"
> > examples/l3fwd/l3fwd_lpm_altivec.h:10:#include "l3fwd_altivec.h"
> > MAINTAINERS:239:F: examples/l3fwd/*altivec.h
> > lib/librte_acl/acl_run_altivec.c:34:#include "acl_run_altivec.h"
> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:49:/*To include
> > altivec.h, GCC version must  >= 4.8 */
> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:50:#include <
> > altivec.h>
> > lib/librte_eal/common/include/arch/ppc_64/rte_vect.h:36:#include
> <altivec.h>
> >
> > lib/librte_lpm/meson.build:9:headers += files('rte_lpm_altivec.h',
> > 'rte_lpm_neon.h', 'rte_lpm_sse.h')
> > lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=
> > rte_lpm_altivec.h
> > lib/librte_lpm/rte_lpm.h:461:#include "rte_lpm_altivec.h"
>
> I'd still like to give it a try given only knwon users of AltiVec code may
> rely on these vector/pixel/bool definitions. Scope should be quite small.
>
> The root issue we need to address is that DPDK applications may
> involuntarily pull altivec.h by including something unrelated
> (rte_memcpy.h)
> and get unwanted bool/vector/pixel macros polluting their namespace and
> breaking things.
>
> > > 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.
> > >
> >
> > Yeah Independent to the other part of the discussion I think we can make
> it
> > generally apply and not just C11.
> >
> > The following "would work" in the code right now.
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > @@ -35,6 +35,21 @@
> >
> > #include <stdint.h>
> > #include <string.h>
> > +/*
> > + * if built with newer C standard like -std=c11 stdbool.h bool and
> altivec
> > + * bool types will conflict. We have to force altivec users (rte_vect.h
> and
> > + * rte_memcpy.h) rely on __vector implying internal altivec knowledge to
> > the
> > + * users but keeping things transparent for all others.
> > + * Therefore define __APPLE_ALTIVEC__ which make it not (re-)define the
> > + * non prefixed types lile "bool".
> > + * While we need to disambiguise bool, we want vector/pixel to stay
> as-is
> > + * in those cases so define them as altivec.h would.
> > + */
> > +#ifndef __APPLE_ALTIVEC__
> > +#define __APPLE_ALTIVEC__ 1
> > +#define vector __vector
> > +#define pixel __pixel
> > +#endif
> > /*To include altivec.h, GCC version must  >= 4.8 */
> > #include <altivec.h>
> >
> > But here again, ordering could make altivec.h be included before this
> > statement in rte_memcpy.
>
> Another possible issue: Clang's altivec.h differs from that of GCC.
>
> It doesn't have __APPLE_ALTIVEC__ and doesn't define macros for
> bool/vector/pixel, which is good as they only exist as context-aware
> compiler keywords with -maltivec, however I don't see instances of __pixel
> or __bool beside __vector in that file. This should be carefully tested to
> make sure the __ prefix is supported by both compilers.
>
> > I would not want to see us search and replace all occurrence of "vector"
> > nor doing the same trick all over the place at every include of altivec.h
> > How about the following which should address the follwing:
> > - resolve the issue with stbool conflicting
> > - no issues with vector types as it retains what would be defined
> > - have the workaround in just one place
> > - independent to include order as long as rte_altivec.h is uses instead
> of
> > a direct include
>
> I like rte_altivec.h. It's explicit and easy to make sure it remains the
> only user of altivec.h in DPDK. However due to the remaining issues with
> these macros, I still believe we should address them all at once.
>
> So let's take a slighly different approach. Assuming users of altivec.h
> know
> what they are doing, stdbool.h and altivec.h conflicts and the compiler
> flags they use is their problem. We only need to help those that didn't ask
> for altivec.h and get infected by it through header dependencies.
>
> Normally, -maltivec is all that's needed to get harmless bool/vector/pixel
> context-sensitive keywords (even without including altivec.h) as expected
> by
> applications, however no one ever expects these to be harmful macros as is
> the case when compiling with GCC in ISO C mode.
>
> In short, public headers that include altivec.h need to clean the mess
> after
> themselves transparently. So here's a different suggestion for
> rte_altivec.h:
>
> ///
>
>  #ifndef RTE_ALTIVEC_H_
>  #define RTE_ALTIVEC_H_
>
>  #ifdef bool
>  #define RTE_ALTIVEC_H_ORIG_BOOL bool
>  #undef bool
>  #endif
>
>  #ifdef vector
>  #define RTE_ALTIVEC_H_ORIG_VECTOR vector
>  #undef vector
>  #endif
>
>  #ifdef pixel
>  #define RTE_ALTIVEC_H_ORIG_PIXEL pixel
>  #undef pixel
>  #endif
>
>  #include <altivec.h>
>
>  #undef bool
>  #undef vector
>  #undef pixel
>
>  #ifdef RTE_ALTIVEC_H_ORIG_BOOL
>  #define bool RTE_ALTIVEC_H_ORIG_BOOL
>  #undef RTE_ALTIVEC_H_ORIG_BOOL
>  #endif
>
>  #ifdef RTE_ALTIVEC_H_ORIG_VECTOR
>  #define vector RTE_ALTIVEC_H_ORIG_VECTOR
>  #undef RTE_ALTIVEC_H_ORIG_VECTOR
>  #endif
>
>  #ifdef RTE_ALTIVEC_H_ORIG_PIXEL
>  #define pixel RTE_ALTIVEC_H_ORIG_PIXEL
>  #undef RTE_ALTIVEC_H_ORIG_PIXEL
>
> ///
>
> With public headers such as rte_vect.h and rte_memcpy.h modified to use
> rte_altivec.h and rely on __vector and __bool where relevant. Applications
> can continue to rely on altivec.h and non-prefixed counterparts as usual.
>
> That way, applications that absolutely want to combine ISO C and altivec.h
> yet still get bool/vector/pixel macros only have to make sure it's included
> before any DPDK headers. This shouldn't be a problem for the vast majority.
>
> What's your opinion?
>
> Now given the size of this mess, I'm starting to think that the quick &
> dirty workaround in mlx5 doesn't look that bad after all.


Yes, we do not want to (re-)invent anything here.
And by our engineering habits I guess we already have started more than we
should.
More on generic thoughts below ..


> Files that rely on
> stdbool.h only need something like this after #include directives:
>
>  /* Compilation workaround for PPC targets when AltiVec is enabled. */
>  #undef bool
>  #define bool _Bool
>
> I'm fine with that if it's OK for you.
>

Yeah I'd be fine with something like that as well.
I'll tomorrow try to come up with a minimal version that is proven to build
there based on the suggestion.
Just no time for it today.

I'll add a "heavily-discussed-by:" tag for you - thanks++


On the engineering of messy huge solutions by two people not really
responsible for it :-), something else came to my mind.
Why is no-one of IBM/Power world replying at all?
There not even was a "oh yeah, <whatever the content would be>" mail by
them.
Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
This is something that mid term has to be sorted out - I tried to pull some
strings to get attention "there" but so far I'm waiting for a reply.
I'd say 18.11 should not be released with a clear re-confirmation of ppc64
maintainance ppc64 by "someone".


A few unrelated minor comments about your patch below.
>
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > index f3fc8267..31dc6839 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
> > @@ -42,7 +42,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/lib/librte_eal/common/Makefile
> b/lib/librte_eal/common/Makefile
> > index cca68826..cab13f1d 100644
> > --- a/lib/librte_eal/common/Makefile
> > +++ b/lib/librte_eal/common/Makefile
> > @@ -17,6 +17,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h
> > INC += rte_service.h rte_service_component.h
> > INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
> > INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
> > +INC += rte_altivec.h
> >
> > GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
> > GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
> > 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 75f74897..225de7a0 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
> > @@ -35,8 +35,8 @@
> >
> > #include <stdint.h>
> > #include <string.h>
> > -/*To include altivec.h, GCC version must  >= 4.8 */
> > -#include <altivec.h>
> > +
> > +#include <rte_altivec.h>
> >
> > #ifdef __cplusplus
> > extern "C" {
> > diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > index 99586e58..1ac81d73 100644
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
> > @@ -33,7 +33,7 @@
> > #ifndef _RTE_VECT_PPC_64_H_
> > #define _RTE_VECT_PPC_64_H_
> >
> > -#include <altivec.h>
> > +#include <rte_altivec.h>
> > #include "generic/rte_vect.h"
> >
> > #ifdef __cplusplus
> > diff --git a/lib/librte_eal/common/include/rte_altivec.h
> > b/lib/librte_eal/common/include/rte_altivec.h
> > new file mode 100644
> > index 00000000..e620e701
> > --- /dev/null
> > +++ b/lib/librte_eal/common/include/rte_altivec.h
> > @@ -0,0 +1,60 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright 2018 Canonical Ltd.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of NXP nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > + */
>
> Make sure to use the SPDX format for the copyright notice, see other
> headers.
>
> > +#ifndef _RTE_EAL_ALTIVEC_H_
> > +#define _RTE_EAL_ALTIVEC_H_
> > +
> > +/*
> > + * if built with newer C standard like -std=c11 stdbool.h bool and
> altivec
> > + * bool types will conflict.
> > + * There is no user of the altivec bool type, so make sure it never is
> > + * defined. Therefore define __APPLE_ALTIVEC__ which make it not
> > (re-)define
> > + * the non prefixed types lile "bool".
> > + * On the other hand other types like vector are used, so while we need
> to
> > + * disambiguise type bool, we want vector/pixel to stay as-is so we
> define
> > + * them as altivec.h would.
> > + * Note: without -std= the compiler has all those as context sensitive
> > + * keywords and the header will not define them at all. Therefore the
> > + * compiler has __APPLE_ALTIVEC__ already set in those cases -
> therefore we
> > + * don't touch things if __APPLE_ALTIVEC__ is already set.
> > + */
> > +#ifndef __APPLE_ALTIVEC__
> > +#define __APPLE_ALTIVEC__ 1
> > +#define vector __vector
> > +#define pixel __pixel
> > +#endif
> > +
> > +/*To include altivec.h, GCC version must  >= 4.8 */
>
> This comment can probably be dropped given Clang also ships altivec.h and
> GCC <= 4.8 is not actively supported anymore (sys_reqs.rst).
>
> > +#include <altivec.h>
> > +
> > +#endif /* _RTE_EAL_ALTIVEC_H_ */
> > diff --git a/lib/librte_eal/common/meson.build
> > b/lib/librte_eal/common/meson.build
> > index 56005bea..616f2084 100644
> > --- a/lib/librte_eal/common/meson.build
> > +++ b/lib/librte_eal/common/meson.build
> > @@ -45,6 +45,7 @@ common_objs += eal_common_arch_objs
> >
> > common_headers = files(
> >        'include/rte_alarm.h',
> > +       'include/rte_altivec.h',
> >        'include/rte_branch_prediction.h',
> >        'include/rte_bus.h',
> >        'include/rte_bitmap.h',
>
> --
> Adrien Mazarguil
> 6WIND
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Thomas Monjalon]

29/08/2018 16:37, Christian Ehrhardt:
> Why is no-one of IBM/Power world replying at all?
> There not even was a "oh yeah, <whatever the content would be>" mail by
> them.
> Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
> This is something that mid term has to be sorted out - I tried to pull some
> strings to get attention "there" but so far I'm waiting for a reply.
> I'd say 18.11 should not be released with a clear re-confirmation of ppc64
> maintainance ppc64 by "someone".

I tend to agree.

Let's try again:

Chao, are you still available for DPDK PPC maintenance?

SHOULD WE DECLARE DPDK NOT SUPPORTED ON PPC64?

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Alfredo Mendoza]

I think Pradeep would be the correct person to answer about support for 
ppc64le

Thanks,

Freddie Mendoza
IBM Systems and Technology Group 
STG ISV Enablement  - Telco/Mobile
 (720) 395-4767





From:   Thomas Monjalon <thomas@monjalon.net>
To:     Christian Ehrhardt <christian.ehrhardt@canonical.com>, 
Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>, Chao Zhu 
<chaozhu@linux.vnet.ibm.com>, pradeep@us.ibm.com, Alfredo Mendoza 
<mendoza1@us.ibm.com>, Gowrishankar Muthukrishnan 
<gowrishankar.m@in.ibm.com>, Gowrishankar Muthukrishnan 
<gowrishankar.m@linux.vnet.ibm.com>, Kevin Reilly <kjreilly@us.ibm.com>
Cc:     dev@dpdk.org, adrien.mazarguil@6wind.com, Luca Boccassi 
<bluca@debian.org>
Date:   08/30/2018 03:36
Subject:        Re: [dpdk-dev] 18.08 build error on ppc64el - bool as 
vector type



29/08/2018 16:37, Christian Ehrhardt:
> Why is no-one of IBM/Power world replying at all?
> There not even was a "oh yeah, <whatever the content would be>" mail by
> them.
> Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
> This is something that mid term has to be sorted out - I tried to pull 
some
> strings to get attention "there" but so far I'm waiting for a reply.
> I'd say 18.11 should not be released with a clear re-confirmation of 
ppc64
> maintainance ppc64 by "someone".

I tend to agree.

Let's try again:

Chao, are you still available for DPDK PPC maintenance?

SHOULD WE DECLARE DPDK NOT SUPPORTED ON PPC64?

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Chao Zhu]

Thomas,

Sorry for the late reply. DPDK will definitely be supported by IBM. I think
Pradeep and the team is working on this. Personally, I don't have enough
bandwidth to do this, we'll have a discussion internally to decide who will
take over the maintenance.

Thank you!

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: 2018年8月30日 16:37
> To: Christian Ehrhardt <christian.ehrhardt@canonical.com>; Gowrishankar
> Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>; Chao Zhu
> <chaozhu@linux.vnet.ibm.com>; pradeep@us.ibm.com; Alfredo Mendoza
> <mendoza1@us.ibm.com>; Gowrishankar Muthukrishnan
> <gowrishankar.m@in.ibm.com>; Gowrishankar Muthukrishnan
> <gowrishankar.m@linux.vnet.ibm.com>; Kevin Reilly <kjreilly@us.ibm.com>
> Cc: dev@dpdk.org; adrien.mazarguil@6wind.com; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type
> 
> 29/08/2018 16:37, Christian Ehrhardt:
> > Why is no-one of IBM/Power world replying at all?
> > There not even was a "oh yeah, <whatever the content would be>" mail
> > by them.
> > Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
> > This is something that mid term has to be sorted out - I tried to pull
> > some strings to get attention "there" but so far I'm waiting for a
reply.
> > I'd say 18.11 should not be released with a clear re-confirmation of
> > ppc64 maintainance ppc64 by "someone".
> 
> I tend to agree.
> 
> Let's try again:
> 
> Chao, are you still available for DPDK PPC maintenance?
> 
> SHOULD WE DECLARE DPDK NOT SUPPORTED ON PPC64?
> 

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

On Fri, Aug 31, 2018 at 5:44 AM Chao Zhu <chaozhu@linux.vnet.ibm.com> wrote:

> Thomas,
>
> Sorry for the late reply. DPDK will definitely be supported by IBM. I think
> Pradeep and the team is working on this. Personally, I don't have enough
> bandwidth to do this, we'll have a discussion internally to decide who will
> take over the maintenance.
>

We have seen you acking packages recently, but that alone .
But we have a new LTS 18.11 ahead and I'd hate to release it without PPC
maintenance.
Did the discussions resolve, if so did you get more time to do it or was
somebody else elected?


Thank you!
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: 2018年8月30日 16:37
> > To: Christian Ehrhardt <christian.ehrhardt@canonical.com>; Gowrishankar
> > Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>; Chao Zhu
> > <chaozhu@linux.vnet.ibm.com>; pradeep@us.ibm.com; Alfredo Mendoza
> > <mendoza1@us.ibm.com>; Gowrishankar Muthukrishnan
> > <gowrishankar.m@in.ibm.com>; Gowrishankar Muthukrishnan
> > <gowrishankar.m@linux.vnet.ibm.com>; Kevin Reilly <kjreilly@us.ibm.com>
> > Cc: dev@dpdk.org; adrien.mazarguil@6wind.com; Luca Boccassi
> > <bluca@debian.org>
> > Subject: Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector
> type
> >
> > 29/08/2018 16:37, Christian Ehrhardt:
> > > Why is no-one of IBM/Power world replying at all?
> > > There not even was a "oh yeah, <whatever the content would be>" mail
> > > by them.
> > > Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
> > > This is something that mid term has to be sorted out - I tried to pull
> > > some strings to get attention "there" but so far I'm waiting for a
> reply.
> > > I'd say 18.11 should not be released with a clear re-confirmation of
> > > ppc64 maintainance ppc64 by "someone".
> >
> > I tend to agree.
> >
> > Let's try again:
> >
> > Chao, are you still available for DPDK PPC maintenance?
> >
> > SHOULD WE DECLARE DPDK NOT SUPPORTED ON PPC64?
> >
>
>
>

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] 18.08 build error on ppc64el - bool as vector type

[Christian Ehrhardt]

On Wed, Aug 29, 2018 at 4:37 PM Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:

>
>
> On Wed, Aug 29, 2018 at 3:16 PM Adrien Mazarguil <
> adrien.mazarguil@6wind.com> wrote:
>
>> On Wed, Aug 29, 2018 at 10:27:03AM +0200, Christian Ehrhardt wrote:
>> > On Tue, Aug 28, 2018 at 5:02 PM Adrien Mazarguil <
>> adrien.mazarguil@6wind.com>
>> > wrote:
>> >
>> > > On Tue, Aug 28, 2018 at 02:38:35PM +0200, Christian Ehrhardt wrote:
>> <trimming thread a bit>
>> > > > --- 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  >= 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 <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:
>> > > 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.
>> > >
>> >
>> > While "pushing the internal knowledge out to users" sounds right at
>> first.
>> > There are far too many IMHO, the change would be huge unclean and messy.
>> >
>> > $ grep -Hrn altivec.h
>> > drivers/net/i40e/i40e_rxtx_vec_altivec.c:45:#include <altivec.h>
>> > examples/l3fwd/l3fwd_lpm.c:165:#include "l3fwd_lpm_altivec.h"
>> > examples/l3fwd/l3fwd_lpm_altivec.h:10:#include "l3fwd_altivec.h"
>> > MAINTAINERS:239:F: examples/l3fwd/*altivec.h
>> > lib/librte_acl/acl_run_altivec.c:34:#include "acl_run_altivec.h"
>> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:49:/*To include
>> > altivec.h, GCC version must  >= 4.8 */
>> > lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h:50:#include <
>> > altivec.h>
>> > lib/librte_eal/common/include/arch/ppc_64/rte_vect.h:36:#include
>> <altivec.h>
>> >
>> > lib/librte_lpm/meson.build:9:headers += files('rte_lpm_altivec.h',
>> > 'rte_lpm_neon.h', 'rte_lpm_sse.h')
>> > lib/librte_lpm/Makefile:28:SYMLINK-$(CONFIG_RTE_LIBRTE_LPM)-include +=
>> > rte_lpm_altivec.h
>> > lib/librte_lpm/rte_lpm.h:461:#include "rte_lpm_altivec.h"
>>
>> I'd still like to give it a try given only knwon users of AltiVec code may
>> rely on these vector/pixel/bool definitions. Scope should be quite small.
>>
>> The root issue we need to address is that DPDK applications may
>> involuntarily pull altivec.h by including something unrelated
>> (rte_memcpy.h)
>> and get unwanted bool/vector/pixel macros polluting their namespace and
>> breaking things.
>>
>> > > 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.
>> > >
>> >
>> > Yeah Independent to the other part of the discussion I think we can
>> make it
>> > generally apply and not just C11.
>> >
>> > The following "would work" in the code right now.
>> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
>> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
>> > @@ -35,6 +35,21 @@
>> >
>> > #include <stdint.h>
>> > #include <string.h>
>> > +/*
>> > + * if built with newer C standard like -std=c11 stdbool.h bool and
>> altivec
>> > + * bool types will conflict. We have to force altivec users
>> (rte_vect.h and
>> > + * rte_memcpy.h) rely on __vector implying internal altivec knowledge
>> to
>> > the
>> > + * users but keeping things transparent for all others.
>> > + * Therefore define __APPLE_ALTIVEC__ which make it not (re-)define the
>> > + * non prefixed types lile "bool".
>> > + * While we need to disambiguise bool, we want vector/pixel to stay
>> as-is
>> > + * in those cases so define them as altivec.h would.
>> > + */
>> > +#ifndef __APPLE_ALTIVEC__
>> > +#define __APPLE_ALTIVEC__ 1
>> > +#define vector __vector
>> > +#define pixel __pixel
>> > +#endif
>> > /*To include altivec.h, GCC version must  >= 4.8 */
>> > #include <altivec.h>
>> >
>> > But here again, ordering could make altivec.h be included before this
>> > statement in rte_memcpy.
>>
>> Another possible issue: Clang's altivec.h differs from that of GCC.
>>
>> It doesn't have __APPLE_ALTIVEC__ and doesn't define macros for
>> bool/vector/pixel, which is good as they only exist as context-aware
>> compiler keywords with -maltivec, however I don't see instances of __pixel
>> or __bool beside __vector in that file. This should be carefully tested to
>> make sure the __ prefix is supported by both compilers.
>>
>> > I would not want to see us search and replace all occurrence of "vector"
>> > nor doing the same trick all over the place at every include of
>> altivec.h
>> > How about the following which should address the follwing:
>> > - resolve the issue with stbool conflicting
>> > - no issues with vector types as it retains what would be defined
>> > - have the workaround in just one place
>> > - independent to include order as long as rte_altivec.h is uses instead
>> of
>> > a direct include
>>
>> I like rte_altivec.h. It's explicit and easy to make sure it remains the
>> only user of altivec.h in DPDK. However due to the remaining issues with
>> these macros, I still believe we should address them all at once.
>>
>> So let's take a slighly different approach. Assuming users of altivec.h
>> know
>> what they are doing, stdbool.h and altivec.h conflicts and the compiler
>> flags they use is their problem. We only need to help those that didn't
>> ask
>> for altivec.h and get infected by it through header dependencies.
>>
>> Normally, -maltivec is all that's needed to get harmless bool/vector/pixel
>> context-sensitive keywords (even without including altivec.h) as expected
>> by
>> applications, however no one ever expects these to be harmful macros as is
>> the case when compiling with GCC in ISO C mode.
>>
>> In short, public headers that include altivec.h need to clean the mess
>> after
>> themselves transparently. So here's a different suggestion for
>> rte_altivec.h:
>>
>> ///
>>
>>  #ifndef RTE_ALTIVEC_H_
>>  #define RTE_ALTIVEC_H_
>>
>>  #ifdef bool
>>  #define RTE_ALTIVEC_H_ORIG_BOOL bool
>>  #undef bool
>>  #endif
>>
>>  #ifdef vector
>>  #define RTE_ALTIVEC_H_ORIG_VECTOR vector
>>  #undef vector
>>  #endif
>>
>>  #ifdef pixel
>>  #define RTE_ALTIVEC_H_ORIG_PIXEL pixel
>>  #undef pixel
>>  #endif
>>
>>  #include <altivec.h>
>>
>>  #undef bool
>>  #undef vector
>>  #undef pixel
>>
>>  #ifdef RTE_ALTIVEC_H_ORIG_BOOL
>>  #define bool RTE_ALTIVEC_H_ORIG_BOOL
>>  #undef RTE_ALTIVEC_H_ORIG_BOOL
>>  #endif
>>
>>  #ifdef RTE_ALTIVEC_H_ORIG_VECTOR
>>  #define vector RTE_ALTIVEC_H_ORIG_VECTOR
>>  #undef RTE_ALTIVEC_H_ORIG_VECTOR
>>  #endif
>>
>>  #ifdef RTE_ALTIVEC_H_ORIG_PIXEL
>>  #define pixel RTE_ALTIVEC_H_ORIG_PIXEL
>>  #undef RTE_ALTIVEC_H_ORIG_PIXEL
>>
>> ///
>>
>> With public headers such as rte_vect.h and rte_memcpy.h modified to use
>> rte_altivec.h and rely on __vector and __bool where relevant. Applications
>> can continue to rely on altivec.h and non-prefixed counterparts as usual.
>>
>> That way, applications that absolutely want to combine ISO C and altivec.h
>> yet still get bool/vector/pixel macros only have to make sure it's
>> included
>> before any DPDK headers. This shouldn't be a problem for the vast
>> majority.
>>
>> What's your opinion?
>>
>> Now given the size of this mess, I'm starting to think that the quick &
>> dirty workaround in mlx5 doesn't look that bad after all.
>
>
> Yes, we do not want to (re-)invent anything here.
> And by our engineering habits I guess we already have started more than we
> should.
> More on generic thoughts below ..
>
>
>> Files that rely on
>> stdbool.h only need something like this after #include directives:
>>
>>  /* Compilation workaround for PPC targets when AltiVec is enabled. */
>>  #undef bool
>>  #define bool _Bool
>>
>> I'm fine with that if it's OK for you.
>>
>
> Yeah I'd be fine with something like that as well.
> I'll tomorrow try to come up with a minimal version that is proven to
> build there based on the suggestion.
> Just no time for it today.
>
> I'll add a "heavily-discussed-by:" tag for you - thanks++
>

I also made sure to only mess up exactly the affected case PPC64 without
__APPLE_ALTIVEC__ (no matter what -std or else made it disappear).
The minimal solution that works for now until we have maintainer feedback
would be this:

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 75f74897..d0c57e28 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,15 @@
#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
+ #define bool _Bool
+#endif

#ifdef __cplusplus
extern "C" {


I'll reply that as a proper patch for inclusion as an interim fix for now.



> On the engineering of messy huge solutions by two people not really
> responsible for it :-), something else came to my mind.
> Why is no-one of IBM/Power world replying at all?
> There not even was a "oh yeah, <whatever the content would be>" mail by
> them.
> Is in fact ppc64 support in DPDK dead or at least "unmaintained"?
> This is something that mid term has to be sorted out - I tried to pull
> some strings to get attention "there" but so far I'm waiting for a reply.
> I'd say 18.11 should not be released with a clear re-confirmation of ppc64
> maintainance ppc64 by "someone".
>
>
> A few unrelated minor comments about your patch below.
>>
>> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
>> > b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
>> > index f3fc8267..31dc6839 100644
>> > --- a/drivers/net/i40e/i40e_rxtx_vec_altivec.c
>> > +++ b/drivers/net/i40e/i40e_rxtx_vec_altivec.c
>> > @@ -42,7 +42,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/lib/librte_eal/common/Makefile
>> b/lib/librte_eal/common/Makefile
>> > index cca68826..cab13f1d 100644
>> > --- a/lib/librte_eal/common/Makefile
>> > +++ b/lib/librte_eal/common/Makefile
>> > @@ -17,6 +17,7 @@ INC += rte_malloc.h rte_keepalive.h rte_time.h
>> > INC += rte_service.h rte_service_component.h
>> > INC += rte_bitmap.h rte_vfio.h rte_hypervisor.h rte_test.h
>> > INC += rte_reciprocal.h rte_fbarray.h rte_uuid.h
>> > +INC += rte_altivec.h
>> >
>> > GENERIC_INC := rte_atomic.h rte_byteorder.h rte_cycles.h rte_prefetch.h
>> > GENERIC_INC += rte_spinlock.h rte_memcpy.h rte_cpuflags.h rte_rwlock.h
>> > 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 75f74897..225de7a0 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
>> > @@ -35,8 +35,8 @@
>> >
>> > #include <stdint.h>
>> > #include <string.h>
>> > -/*To include altivec.h, GCC version must  >= 4.8 */
>> > -#include <altivec.h>
>> > +
>> > +#include <rte_altivec.h>
>> >
>> > #ifdef __cplusplus
>> > extern "C" {
>> > diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
>> > b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
>> > index 99586e58..1ac81d73 100644
>> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
>> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_vect.h
>> > @@ -33,7 +33,7 @@
>> > #ifndef _RTE_VECT_PPC_64_H_
>> > #define _RTE_VECT_PPC_64_H_
>> >
>> > -#include <altivec.h>
>> > +#include <rte_altivec.h>
>> > #include "generic/rte_vect.h"
>> >
>> > #ifdef __cplusplus
>> > diff --git a/lib/librte_eal/common/include/rte_altivec.h
>> > b/lib/librte_eal/common/include/rte_altivec.h
>> > new file mode 100644
>> > index 00000000..e620e701
>> > --- /dev/null
>> > +++ b/lib/librte_eal/common/include/rte_altivec.h
>> > @@ -0,0 +1,60 @@
>> > +/*-
>> > + *   BSD LICENSE
>> > + *
>> > + *   Copyright 2018 Canonical Ltd.
>> > + *
>> > + *   Redistribution and use in source and binary forms, with or without
>> > + *   modification, are permitted provided that the following conditions
>> > + *   are met:
>> > + *
>> > + *     * Redistributions of source code must retain the above copyright
>> > + *       notice, this list of conditions and the following disclaimer.
>> > + *     * Redistributions in binary form must reproduce the above
>> copyright
>> > + *       notice, this list of conditions and the following disclaimer
>> in
>> > + *       the documentation and/or other materials provided with the
>> > + *       distribution.
>> > + *     * Neither the name of NXP nor the names of its
>> > + *       contributors may be used to endorse or promote products
>> derived
>> > + *       from this software without specific prior written permission.
>> > + *
>> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> CONTRIBUTORS
>> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
>> FOR
>> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
>> COPYRIGHT
>> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
>> INCIDENTAL,
>> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
>> USE,
>> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
>> ANY
>> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
>> TORT
>> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
>> USE
>> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
>> DAMAGE.
>> > + */
>>
>> Make sure to use the SPDX format for the copyright notice, see other
>> headers.
>>
>> > +#ifndef _RTE_EAL_ALTIVEC_H_
>> > +#define _RTE_EAL_ALTIVEC_H_
>> > +
>> > +/*
>> > + * if built with newer C standard like -std=c11 stdbool.h bool and
>> altivec
>> > + * bool types will conflict.
>> > + * There is no user of the altivec bool type, so make sure it never is
>> > + * defined. Therefore define __APPLE_ALTIVEC__ which make it not
>> > (re-)define
>> > + * the non prefixed types lile "bool".
>> > + * On the other hand other types like vector are used, so while we
>> need to
>> > + * disambiguise type bool, we want vector/pixel to stay as-is so we
>> define
>> > + * them as altivec.h would.
>> > + * Note: without -std= the compiler has all those as context sensitive
>> > + * keywords and the header will not define them at all. Therefore the
>> > + * compiler has __APPLE_ALTIVEC__ already set in those cases -
>> therefore we
>> > + * don't touch things if __APPLE_ALTIVEC__ is already set.
>> > + */
>> > +#ifndef __APPLE_ALTIVEC__
>> > +#define __APPLE_ALTIVEC__ 1
>> > +#define vector __vector
>> > +#define pixel __pixel
>> > +#endif
>> > +
>> > +/*To include altivec.h, GCC version must  >= 4.8 */
>>
>> This comment can probably be dropped given Clang also ships altivec.h and
>> GCC <= 4.8 is not actively supported anymore (sys_reqs.rst).
>>
>> > +#include <altivec.h>
>> > +
>> > +#endif /* _RTE_EAL_ALTIVEC_H_ */
>> > diff --git a/lib/librte_eal/common/meson.build
>> > b/lib/librte_eal/common/meson.build
>> > index 56005bea..616f2084 100644
>> > --- a/lib/librte_eal/common/meson.build
>> > +++ b/lib/librte_eal/common/meson.build
>> > @@ -45,6 +45,7 @@ common_objs += eal_common_arch_objs
>> >
>> > common_headers = files(
>> >        'include/rte_alarm.h',
>> > +       'include/rte_altivec.h',
>> >        'include/rte_branch_prediction.h',
>> >        'include/rte_bus.h',
>> >        'include/rte_bitmap.h',
>>
>> --
>> Adrien Mazarguil
>> 6WIND
>>
>
>
> --
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

[dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Christian Ehrhardt]

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

Change-Id: I1ed56da954e4951b9d120ca2ac0c0c218b4a0140
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

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Takeshi T Yoshimura]

>宛先: adrien.mazarguil@6wind.com, dev <dev@dpdk.org>, Gowrishankar
>Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>, Chao Zhu
><chaozhu@linux.vnet.ibm.com>
>送信元: Christian Ehrhardt 
>送信者: "dev" 
>日付: 2018/08/30 07:00PM
>Cc: Luca Boccassi <bluca@debian.org>, Thomas Monjalon
><thomasm@mellanox.com>, Christian Ehrhardt
><christian.ehrhardt@canonical.com>
>件名: [dpdk-dev] [PATCH] 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]:
>INVALID URI REMOVED
>chives_dev_2018-2DAugust_109926.html&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZO
>g&r=EZR6Jx10q0q3dTopeH3WIQ&m=bbU1KVc1ZvNW9Rz7B0MLHfS0f0oZv35d2mpRpHO0
>ByY&s=RvMIFfk-cAAGTrYM76-iSSqIYV_X2EptYZzYIweIHRk&e=
>
>Change-Id: I1ed56da954e4951b9d120ca2ac0c0c218b4a0140
>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
>
>
>

Hi,
I could reproduce the issue you reported in 18.08 with my ppc64le box with RedHat 7.5 and GCC4.8.
The patch resolved the issue in my environment. Thanks!

I am a bit newbie in dpdk-dev, but I will try contacting Chao and other IBM guys... Sorry for our slow reply.

Regards,
Takeshi

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Christian Ehrhardt]

On Thu, Aug 30, 2018 at 12:52 PM Takeshi T Yoshimura <TYOS@jp.ibm.com>
wrote:

> >宛先: adrien.mazarguil@6wind.com, dev <dev@dpdk.org>, Gowrishankar
> >Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>, Chao Zhu
> ><chaozhu@linux.vnet.ibm.com>
> >送信元: Christian Ehrhardt
> >送信者: "dev"
> >日付: 2018/08/30 07:00PM
> >Cc: Luca Boccassi <bluca@debian.org>, Thomas Monjalon
> ><thomasm@mellanox.com>, Christian Ehrhardt
> ><christian.ehrhardt@canonical.com>
> >件名: [dpdk-dev] [PATCH] 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]:
> >INVALID URI REMOVED
> >chives_dev_2018-2DAugust_109926.html&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZO
> >g&r=EZR6Jx10q0q3dTopeH3WIQ&m=bbU1KVc1ZvNW9Rz7B0MLHfS0f0oZv35d2mpRpHO0
> >ByY&s=RvMIFfk-cAAGTrYM76-iSSqIYV_X2EptYZzYIweIHRk&e=
> >
> >Change-Id: I1ed56da954e4951b9d120ca2ac0c0c218b4a0140
> >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
> >
> >
> >
>
> Hi,
> I could reproduce the issue you reported in 18.08 with my ppc64le box with
> RedHat 7.5 and GCC4.8.
> The patch resolved the issue in my environment. Thanks!
>

I added your test (tanks) and Adrien's extensive review/discussion as tags
and also addressed a few checkpatch findings.
V2 is up on the list now ...


> I am a bit newbie in dpdk-dev, but I will try contacting Chao and other
> IBM guys... Sorry for our slow reply.
>

Thanks for your participation Takeshi,
we at least now have had a few replies after Thomas used the superpowers of
"CPT. CAPSLOCK" \o/.

I also have a call later today to make sure this is brought up inside IBM
to make sure someone is maintaining it for real.


> Regards,
> Takeshi
>
>

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Thomas Monjalon]

Hi,

30/08/2018 13:58, Christian Ehrhardt:
> On Thu, Aug 30, 2018 at 12:52 PM Takeshi T Yoshimura <TYOS@jp.ibm.com>
> wrote:
> > Hi,
> > I could reproduce the issue you reported in 18.08 with my ppc64le box with
> > RedHat 7.5 and GCC4.8.
> > The patch resolved the issue in my environment. Thanks!
> 
> I added your test (tanks) and Adrien's extensive review/discussion as tags
> and also addressed a few checkpatch findings.
> V2 is up on the list now ...
> 
> > I am a bit newbie in dpdk-dev, but I will try contacting Chao and other
> > IBM guys... Sorry for our slow reply.
> 
> Thanks for your participation Takeshi,
> we at least now have had a few replies after Thomas used the superpowers of
> "CPT. CAPSLOCK" \o/.
> 
> I also have a call later today to make sure this is brought up inside IBM
> to make sure someone is maintaining it for real.

Summary of the situation:
	- I used caps lock on August 30th
	- We got replies on the ML in the next 2 days (Alfredo, Chao, Takeshi)
	- On September 3rd, Adrien raised a major issue for C++ with the fix v3
		http://mails.dpdk.org/archives/dev/2018-September/110733.html
	- Another email about a possible GCC fix on September 5th (David Wilder)
	- There was a private reply on September 27th, confirming an IBM support
	- and nothing else

Nobody at IBM requests to get a compilation fix for ppc64.
And there was no issue raised after 18.11-rc1 release.
I guess it means DPDK is not used on ppc64.
In this case, we should mark the ppc port as unmaintained for 18.11.

OR SHOULD I USE MY CAPS LOCK AGAIN?

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Pradeep Satyanarayana]

Thomas Monjalon <thomas@monjalon.net> wrote on 11/05/2018 06:15:53 AM:

> From: Thomas Monjalon <thomas@monjalon.net>
> To: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Cc: dev@dpdk.org, TYOS@jp.ibm.com, adrien.mazarguil@6wind.com,
> Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>, Chao
> Zhu <chaozhu@linux.vnet.ibm.com>, Luca Boccassi <bluca@debian.org>,
> Pradeep Satyanarayana <pradeep@us.ibm.com>, dwilder@us.ibm.com
> Date: 11/05/2018 06:16 AM
> Subject: Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when
> AltiVec is enabled
>
> Hi,
>
> 30/08/2018 13:58, Christian Ehrhardt:
> > On Thu, Aug 30, 2018 at 12:52 PM Takeshi T Yoshimura <TYOS@jp.ibm.com>
> > wrote:
> > > Hi,
> > > I could reproduce the issue you reported in 18.08 with my ppc64le box
with
> > > RedHat 7.5 and GCC4.8.
> > > The patch resolved the issue in my environment. Thanks!
> >
> > I added your test (tanks) and Adrien's extensive review/discussion as
tags
> > and also addressed a few checkpatch findings.
> > V2 is up on the list now ...
> >
> > > I am a bit newbie in dpdk-dev, but I will try contacting Chao and
other
> > > IBM guys... Sorry for our slow reply.
> >
> > Thanks for your participation Takeshi,
> > we at least now have had a few replies after Thomas used the
superpowers of
> > "CPT. CAPSLOCK" \o/.
> >
> > I also have a call later today to make sure this is brought up inside
IBM
> > to make sure someone is maintaining it for real.
>
> Summary of the situation:
>    - I used caps lock on August 30th
>    - We got replies on the ML in the next 2 days (Alfredo, Chao, Takeshi)
>    - On September 3rd, Adrien raised a major issue for C++ with the fix
v3
>       INVALID URI REMOVED
>
u=http-3A__mails.dpdk.org_archives_dev_2018-2DSeptember_110733.html&d=DwICAg&c=jf_iaSHvJObTbx-

> siA1ZOg&r=co4lCofxrQP11yIVMply-
>
QYvsUyeKJkYY_jL1QVgeGA&m=dMrH4GLoXWGcc5xt87goaWszBzO4TeTVx7O9pZo160o&s=_hNc10YMFL2mf2TkG9tbjm5_2fyPER3MswvK-

> NKs9RY&e=
>    - Another email about a possible GCC fix on September 5th (David
Wilder)

As Dave mentioned that is only expected in GCC 9.

>    - There was a private reply on September 27th, confirming an IBM
support
>    - and nothing else
>
> Nobody at IBM requests to get a compilation fix for ppc64.

Yes, we do need fixes for ppc64.

(1) http://mails.dpdk.org/archives/dev/2018-August/110499.html
(2) http://mails.dpdk.org/archives/dev/2018-September/110961.html

Based on the above 2 URLs (tested both by Takeshi and David Wiler), we
assumed that it would get
picked up in 18.11. We have been more focussed on 17.11 (and likely dropped
the ball on 18.11)
since 17.11 is an LTS release and we have had lots of problems on ppc64.
Should be submitting
patch to fix those issues shortly.

We have built 18.11-rc1 with the fix above (1), and it does work on
ppc64le.

An updated version of:

(3) http://mails.dpdk.org/archives/dev/2018-August/109926.html

also builds on ppc64.  The latter has the advantage of possibly not
breaking existing applications.


> And there was no issue raised after 18.11-rc1 release.
> I guess it means DPDK is not used on ppc64.
> In this case, we should mark the ppc port as unmaintained for 18.11.
>
> OR SHOULD I USE MY CAPS LOCK AGAIN?

Thanks for your patience while we iron out the issues. Hopefully, we don't
need the CAPS LOCK again.

Thanks
Pradeep
pradeep@us.ibm.com

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Thomas Monjalon]

05/11/2018 22:20, Pradeep Satyanarayana:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 30/08/2018 13:58, Christian Ehrhardt:
> > > On Thu, Aug 30, 2018 Takeshi T Yoshimura <TYOS@jp.ibm.com> wrote:
> > > > Hi,
> > > > I could reproduce the issue you reported in 18.08 with my ppc64le
> > > > box with RedHat 7.5 and GCC4.8.
> > > > The patch resolved the issue in my environment. Thanks!
> > >
> > > I added your test (tanks) and Adrien's extensive review/discussion as
> > > tags and also addressed a few checkpatch findings.
> > > V2 is up on the list now ...
> > >
> > > > I am a bit newbie in dpdk-dev, but I will try contacting Chao
> > > > and other IBM guys... Sorry for our slow reply.
> > >
> > > Thanks for your participation Takeshi,
> > > we at least now have had a few replies after Thomas used the
> > > superpowers of "CPT. CAPSLOCK" \o/.
> > >
> > > I also have a call later today to make sure this is brought up
> > > inside IBM to make sure someone is maintaining it for real.
> >
> > Summary of the situation:
> >    - I used caps lock on August 30th
> >    - We got replies on the ML in the next 2 days (Alfredo, Chao, Takeshi)
> >    - On September 3rd, Adrien raised a major issue for C++ with the fix v3
> > 		http://mails.dpdk.org/archives/dev/2018-September/110733.html
> >    - Another email about a possible GCC fix on September 5th (David Wilder)
> 
> As Dave mentioned that is only expected in GCC 9.
> 
> >    - There was a private reply on September 27th, confirming an IBM support
> >    - and nothing else
> >
> > Nobody at IBM requests to get a compilation fix for ppc64.
> 
> Yes, we do need fixes for ppc64.
> 
> (1) http://mails.dpdk.org/archives/dev/2018-August/110499.html
> (2) http://mails.dpdk.org/archives/dev/2018-September/110961.html
> 
> Based on the above 2 URLs (tested both by Takeshi and David Wiler), we
> assumed that it would get picked up in 18.11.
> We have been more focussed on 17.11 (and likely dropped
> the ball on 18.11)
> since 17.11 is an LTS release and we have had lots of problems on ppc64.

Note that 18.11 is also LTS.

> Should be submitting patch to fix those issues shortly.

Sorry, I have some doubts for two reasons:
	- track records
	- technical reality: there is no perfect solution outside of GCC

> We have built 18.11-rc1 with the fix above (1), and it does work on
> ppc64le.

But it would break C++ applications.

> An updated version of:
> 
> (3) http://mails.dpdk.org/archives/dev/2018-August/109926.html
> 
> also builds on ppc64.  The latter has the advantage of possibly not
> breaking existing applications.

But it fixes only mlx5.
stdbool is used in many other places.
Which PMDs are you compiling?
Are you compiling examples?

> > And there was no issue raised after 18.11-rc1 release.
> > I guess it means DPDK is not used on ppc64.
> > In this case, we should mark the ppc port as unmaintained for 18.11.
> >
> > OR SHOULD I USE MY CAPS LOCK AGAIN?
> 
> Thanks for your patience while we iron out the issues.
> Hopefully, we don't need the CAPS LOCK again.

We have to mention the ppc64 incompatibility in 18.11 release notes.
Either it stays as is and we declare DPDK 18.11 not supported on
IBM platforms, or we fix it and document the limitations.

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[dwilder]

On 2018-11-07 02:03, Thomas Monjalon wrote:
> 05/11/2018 22:20, Pradeep Satyanarayana:
>> From: Thomas Monjalon <thomas@monjalon.net>
>> > 30/08/2018 13:58, Christian Ehrhardt:
>> > > On Thu, Aug 30, 2018 Takeshi T Yoshimura <TYOS@jp.ibm.com> wrote:
>> > > > Hi,
>> > > > I could reproduce the issue you reported in 18.08 with my ppc64le
>> > > > box with RedHat 7.5 and GCC4.8.
>> > > > The patch resolved the issue in my environment. Thanks!
>> > >
>> > > I added your test (tanks) and Adrien's extensive review/discussion as
>> > > tags and also addressed a few checkpatch findings.
>> > > V2 is up on the list now ...
>> > >
>> > > > I am a bit newbie in dpdk-dev, but I will try contacting Chao
>> > > > and other IBM guys... Sorry for our slow reply.
>> > >
>> > > Thanks for your participation Takeshi,
>> > > we at least now have had a few replies after Thomas used the
>> > > superpowers of "CPT. CAPSLOCK" \o/.
>> > >
>> > > I also have a call later today to make sure this is brought up
>> > > inside IBM to make sure someone is maintaining it for real.
>> >
>> > Summary of the situation:
>> >    - I used caps lock on August 30th
>> >    - We got replies on the ML in the next 2 days (Alfredo, Chao, Takeshi)
>> >    - On September 3rd, Adrien raised a major issue for C++ with the fix v3
>> > 		http://mails.dpdk.org/archives/dev/2018-September/110733.html
>> >    - Another email about a possible GCC fix on September 5th (David Wilder)
>> 
>> As Dave mentioned that is only expected in GCC 9.
>> 
>> >    - There was a private reply on September 27th, confirming an IBM support
>> >    - and nothing else
>> >
>> > Nobody at IBM requests to get a compilation fix for ppc64.
>> 
>> Yes, we do need fixes for ppc64.
>> 
>> (1) 
>> http://mails.dpdk.org/archives/dev/2018-August/110499.html
>> (2) 
>> http://mails.dpdk.org/archives/dev/2018-September/110961.html
>> 
>> Based on the above 2 URLs (tested both by Takeshi and David Wiler), we
>> assumed that it would get picked up in 18.11.
>> We have been more focussed on 17.11 (and likely dropped
>> the ball on 18.11)
>> since 17.11 is an LTS release and we have had lots of problems on 
>> ppc64.
> 
> Note that 18.11 is also LTS.
> 
>> Should be submitting patch to fix those issues shortly.
> 
> Sorry, I have some doubts for two reasons:
> 	- track records
> 	- technical reality: there is no perfect solution outside of GCC
> 
>> We have built 18.11-rc1 with the fix above (1), and it does work on
>> ppc64le.
> 
> But it would break C++ applications.
> 
>> An updated version of:
>> 
>> (3) 
>> http://mails.dpdk.org/archives/dev/2018-August/109926.html
>> 
>> also builds on ppc64.  The latter has the advantage of possibly not
>> breaking existing applications.
> 

I am not seeing any build breaks on upstream code with the 
net-mlx5-fix-build-on-PPC64.patch applied.

> But it fixes only mlx5.
> stdbool is used in many other places.
> Which PMDs are you compiling?

CONFIG_RTE_LIBRTE_ARK_PMD=y
CONFIG_RTE_LIBRTE_AXGBE_PMD=y
CONFIG_RTE_LIBRTE_BNXT_PMD=y
CONFIG_RTE_LIBRTE_CXGBE_PMD=y
CONFIG_RTE_LIBRTE_DPAA_PMD=y
CONFIG_RTE_LIBRTE_DPAA2_PMD=y
CONFIG_RTE_LIBRTE_ENETC_PMD=y
CONFIG_RTE_LIBRTE_ENA_PMD=y
CONFIG_RTE_LIBRTE_EM_PMD=y
CONFIG_RTE_LIBRTE_IGB_PMD=y
CONFIG_RTE_LIBRTE_I40E_PMD=y
CONFIG_RTE_LIBRTE_AVF_PMD=y
CONFIG_RTE_LIBRTE_MLX5_PMD=y
CONFIG_RTE_LIBRTE_NFP_PMD=y
CONFIG_RTE_LIBRTE_QEDE_PMD=y
CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD=y
CONFIG_RTE_LIBRTE_LIO_PMD=y
CONFIG_RTE_LIBRTE_OCTEONTX_PMD=y
CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
CONFIG_RTE_LIBRTE_NETVSC_PMD=y
CONFIG_RTE_LIBRTE_VDEV_NETVSC_PMD=y
CONFIG_RTE_LIBRTE_IFC_PMD=y
CONFIG_RTE_TEST_PMD=y

> Are you compiling examples?

Yes, no build issues seen.

> 
>> > And there was no issue raised after 18.11-rc1 release.
>> > I guess it means DPDK is not used on ppc64.
>> > In this case, we should mark the ppc port as unmaintained for 18.11.
>> >
>> > OR SHOULD I USE MY CAPS LOCK AGAIN?
>> 
>> Thanks for your patience while we iron out the issues.
>> Hopefully, we don't need the CAPS LOCK again.
> 
> We have to mention the ppc64 incompatibility in 18.11 release notes.
> Either it stays as is and we declare DPDK 18.11 not supported on
> IBM platforms, or we fix it and document the limitations.

If net-mlx5-fix-build-on-PPC64.patch is accepted I feel power can be 
listed as supported for 18.11.

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Thomas Monjalon]

07/11/2018 19:58, dwilder:
> On 2018-11-07 02:03, Thomas Monjalon wrote:
> > 05/11/2018 22:20, Pradeep Satyanarayana:
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> > 30/08/2018 13:58, Christian Ehrhardt:
> >> > > On Thu, Aug 30, 2018 Takeshi T Yoshimura <TYOS@jp.ibm.com> wrote:
> >> > > > Hi,
> >> > > > I could reproduce the issue you reported in 18.08 with my ppc64le
> >> > > > box with RedHat 7.5 and GCC4.8.
> >> > > > The patch resolved the issue in my environment. Thanks!
> >> > >
> >> > > I added your test (tanks) and Adrien's extensive review/discussion as
> >> > > tags and also addressed a few checkpatch findings.
> >> > > V2 is up on the list now ...
> >> > >
> >> > > > I am a bit newbie in dpdk-dev, but I will try contacting Chao
> >> > > > and other IBM guys... Sorry for our slow reply.
> >> > >
> >> > > Thanks for your participation Takeshi,
> >> > > we at least now have had a few replies after Thomas used the
> >> > > superpowers of "CPT. CAPSLOCK" \o/.
> >> > >
> >> > > I also have a call later today to make sure this is brought up
> >> > > inside IBM to make sure someone is maintaining it for real.
> >> >
> >> > Summary of the situation:
> >> >    - I used caps lock on August 30th
> >> >    - We got replies on the ML in the next 2 days (Alfredo, Chao, Takeshi)
> >> >    - On September 3rd, Adrien raised a major issue for C++ with the fix v3
> >> > 		http://mails.dpdk.org/archives/dev/2018-September/110733.html
> >> >    - Another email about a possible GCC fix on September 5th (David Wilder)
> >> 
> >> As Dave mentioned that is only expected in GCC 9.
> >> 
> >> >    - There was a private reply on September 27th, confirming an IBM support
> >> >    - and nothing else
> >> >
> >> > Nobody at IBM requests to get a compilation fix for ppc64.
> >> 
> >> Yes, we do need fixes for ppc64.
> >> 
> >> (1) 
> >> http://mails.dpdk.org/archives/dev/2018-August/110499.html
> >> (2) 
> >> http://mails.dpdk.org/archives/dev/2018-September/110961.html
> >> 
> >> Based on the above 2 URLs (tested both by Takeshi and David Wiler), we
> >> assumed that it would get picked up in 18.11.
> >> We have been more focussed on 17.11 (and likely dropped
> >> the ball on 18.11)
> >> since 17.11 is an LTS release and we have had lots of problems on 
> >> ppc64.
> > 
> > Note that 18.11 is also LTS.
> > 
> >> Should be submitting patch to fix those issues shortly.
> > 
> > Sorry, I have some doubts for two reasons:
> > 	- track records
> > 	- technical reality: there is no perfect solution outside of GCC
> > 
> >> We have built 18.11-rc1 with the fix above (1), and it does work on
> >> ppc64le.
> > 
> > But it would break C++ applications.
> > 
> >> An updated version of:
> >> 
> >> (3) 
> >> http://mails.dpdk.org/archives/dev/2018-August/109926.html
> >> 
> >> also builds on ppc64.  The latter has the advantage of possibly not
> >> breaking existing applications.
> 
> I am not seeing any build breaks on upstream code with the 
> net-mlx5-fix-build-on-PPC64.patch applied.
> 
> > But it fixes only mlx5.
> > stdbool is used in many other places.
> > Which PMDs are you compiling?
> 
> CONFIG_RTE_LIBRTE_ARK_PMD=y
> CONFIG_RTE_LIBRTE_AXGBE_PMD=y
> CONFIG_RTE_LIBRTE_BNXT_PMD=y
> CONFIG_RTE_LIBRTE_CXGBE_PMD=y
> CONFIG_RTE_LIBRTE_DPAA_PMD=y
> CONFIG_RTE_LIBRTE_DPAA2_PMD=y
> CONFIG_RTE_LIBRTE_ENETC_PMD=y
> CONFIG_RTE_LIBRTE_ENA_PMD=y
> CONFIG_RTE_LIBRTE_EM_PMD=y
> CONFIG_RTE_LIBRTE_IGB_PMD=y
> CONFIG_RTE_LIBRTE_I40E_PMD=y
> CONFIG_RTE_LIBRTE_AVF_PMD=y
> CONFIG_RTE_LIBRTE_MLX5_PMD=y
> CONFIG_RTE_LIBRTE_NFP_PMD=y
> CONFIG_RTE_LIBRTE_QEDE_PMD=y
> CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD=y
> CONFIG_RTE_LIBRTE_LIO_PMD=y
> CONFIG_RTE_LIBRTE_OCTEONTX_PMD=y
> CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> CONFIG_RTE_LIBRTE_NETVSC_PMD=y
> CONFIG_RTE_LIBRTE_VDEV_NETVSC_PMD=y
> CONFIG_RTE_LIBRTE_IFC_PMD=y
> CONFIG_RTE_TEST_PMD=y
> 
> > Are you compiling examples?
> 
> Yes, no build issues seen.
> 
> >> > And there was no issue raised after 18.11-rc1 release.
> >> > I guess it means DPDK is not used on ppc64.
> >> > In this case, we should mark the ppc port as unmaintained for 18.11.
> >> >
> >> > OR SHOULD I USE MY CAPS LOCK AGAIN?
> >> 
> >> Thanks for your patience while we iron out the issues.
> >> Hopefully, we don't need the CAPS LOCK again.
> > 
> > We have to mention the ppc64 incompatibility in 18.11 release notes.
> > Either it stays as is and we declare DPDK 18.11 not supported on
> > IBM platforms, or we fix it and document the limitations.
> 
> If net-mlx5-fix-build-on-PPC64.patch is accepted I feel power can be 
> listed as supported for 18.11.

I sent this last patch which was thought by Christian (Canonical) and
Adrien (6WIND). It is just fixing the compilation.
Is there someone at IBM checking that basic DPDK features are working?

Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when AltiVec is enabled

[Pradeep Satyanarayana]

Thomas Monjalon <thomas@monjalon.net> wrote on 11/07/2018 01:21:22 PM:

> From: Thomas Monjalon <thomas@monjalon.net>
> To: dwilder <dwilder@us.ibm.com>
> Cc: Pradeep Satyanarayana <pradeep@us.ibm.com>, dev@dpdk.org,
> adrien.mazarguil@6wind.com, Luca Boccassi <bluca@debian.org>, Chao
> Zhu <chaozhu@linux.vnet.ibm.com>, Christian Ehrhardt
> <christian.ehrhardt@canonical.com>, TYOS@jp.ibm.com
> Date: 11/07/2018 01:21 PM
> Subject: Re: [dpdk-dev] [PATCH] ppc64: fix compilation of when
> AltiVec is enabled
>
> 07/11/2018 19:58, dwilder:
> > On 2018-11-07 02:03, Thomas Monjalon wrote:
> > > 05/11/2018 22:20, Pradeep Satyanarayana:
> > >> From: Thomas Monjalon <thomas@monjalon.net>
> > >> > 30/08/2018 13:58, Christian Ehrhardt:
> > >> > > On Thu, Aug 30, 2018 Takeshi T Yoshimura <TYOS@jp.ibm.com>
wrote:
> > >> > > > Hi,
> > >> > > > I could reproduce the issue you reported in 18.08 with my
ppc64le
> > >> > > > box with RedHat 7.5 and GCC4.8.
> > >> > > > The patch resolved the issue in my environment. Thanks!
> > >> > >
> > >> > > I added your test (tanks) and Adrien's extensive
review/discussion as
> > >> > > tags and also addressed a few checkpatch findings.
> > >> > > V2 is up on the list now ...
> > >> > >
> > >> > > > I am a bit newbie in dpdk-dev, but I will try contacting Chao
> > >> > > > and other IBM guys... Sorry for our slow reply.
> > >> > >
> > >> > > Thanks for your participation Takeshi,
> > >> > > we at least now have had a few replies after Thomas used the
> > >> > > superpowers of "CPT. CAPSLOCK" \o/.
> > >> > >
> > >> > > I also have a call later today to make sure this is brought up
> > >> > > inside IBM to make sure someone is maintaining it for real.
> > >> >
> > >> > Summary of the situation:
> > >> >    - I used caps lock on August 30th
> > >> >    - We got replies on the ML in the next 2 days (Alfredo,
> Chao, Takeshi)
> > >> >    - On September 3rd, Adrien raised a major issue for C++
> with the fix v3
> > >> >       INVALID URI REMOVED
>
u=http-3A__mails.dpdk.org_archives_dev_2018-2DSeptember_110733.html&d=DwICAg&c=jf_iaSHvJObTbx-

> siA1ZOg&r=co4lCofxrQP11yIVMply-QYvsUyeKJkYY_jL1QVgeGA&m=QE2-
> XfLmWX5fRwewYIIMAHJI_FETkneZA1XxK2aFv0o&s=DuOB1QOoJ-fW2A-0h6Oz-
> SeHLuynCSyblzo3_bvshDg&e=
> > >> >    - Another email about a possible GCC fix on September 5th
> (David Wilder)
> > >>
> > >> As Dave mentioned that is only expected in GCC 9.
> > >>
> > >> >    - There was a private reply on September 27th, confirming
> an IBM support
> > >> >    - and nothing else
> > >> >
> > >> > Nobody at IBM requests to get a compilation fix for ppc64.
> > >>
> > >> Yes, we do need fixes for ppc64.
> > >>
> > >> (1)
> > >> INVALID URI REMOVED
>
u=http-3A__mails.dpdk.org_archives_dev_2018-2DAugust_110499.html&d=DwICAg&c=jf_iaSHvJObTbx-

> siA1ZOg&r=co4lCofxrQP11yIVMply-QYvsUyeKJkYY_jL1QVgeGA&m=QE2-
>
XfLmWX5fRwewYIIMAHJI_FETkneZA1XxK2aFv0o&s=XgjcGK0kIYU4y3K6zUMcAcVZxxzDYoUUm90oFuzGII8&e=

> > >> (2)
> > >> INVALID URI REMOVED
>
u=http-3A__mails.dpdk.org_archives_dev_2018-2DSeptember_110961.html&d=DwICAg&c=jf_iaSHvJObTbx-

> siA1ZOg&r=co4lCofxrQP11yIVMply-QYvsUyeKJkYY_jL1QVgeGA&m=QE2-
> XfLmWX5fRwewYIIMAHJI_FETkneZA1XxK2aFv0o&s=5XZlfxsqUXgL-
> aFscHGJgdDiqhKnfjz7Kx4KNj2J5Ck&e=
> > >>
> > >> Based on the above 2 URLs (tested both by Takeshi and David Wiler),
we
> > >> assumed that it would get picked up in 18.11.
> > >> We have been more focussed on 17.11 (and likely dropped
> > >> the ball on 18.11)
> > >> since 17.11 is an LTS release and we have had lots of problems on
> > >> ppc64.
> > >
> > > Note that 18.11 is also LTS.

Yes, we do realize that 18.11 is an LTS release. Since there is a larger
usage of
17.11 we have been focussed on that. Attempting to catch up with 18.11 as
well.

> > >
> > >> Should be submitting patch to fix those issues shortly.
> > >
> > > Sorry, I have some doubts for two reasons:
> > >    - track records
> > >    - technical reality: there is no perfect solution outside of GCC
> > >
> > >> We have built 18.11-rc1 with the fix above (1), and it does work on
> > >> ppc64le.
> > >
> > > But it would break C++ applications.
> > >
> > >> An updated version of:
> > >>
> > >> (3)
> > >> INVALID URI REMOVED
>
u=http-3A__mails.dpdk.org_archives_dev_2018-2DAugust_109926.html&d=DwICAg&c=jf_iaSHvJObTbx-

> siA1ZOg&r=co4lCofxrQP11yIVMply-QYvsUyeKJkYY_jL1QVgeGA&m=QE2-
> XfLmWX5fRwewYIIMAHJI_FETkneZA1XxK2aFv0o&s=urcohXf8f-
> T9doxPSqC3wRWT__d0nVmO6QftUwIvcG0&e=
> > >>
> > >> also builds on ppc64.  The latter has the advantage of possibly not
> > >> breaking existing applications.
> >
> > I am not seeing any build breaks on upstream code with the
> > net-mlx5-fix-build-on-PPC64.patch applied.
> >
> > > But it fixes only mlx5.
> > > stdbool is used in many other places.
> > > Which PMDs are you compiling?
> >
> > CONFIG_RTE_LIBRTE_ARK_PMD=y
> > CONFIG_RTE_LIBRTE_AXGBE_PMD=y
> > CONFIG_RTE_LIBRTE_BNXT_PMD=y
> > CONFIG_RTE_LIBRTE_CXGBE_PMD=y
> > CONFIG_RTE_LIBRTE_DPAA_PMD=y
> > CONFIG_RTE_LIBRTE_DPAA2_PMD=y
> > CONFIG_RTE_LIBRTE_ENETC_PMD=y
> > CONFIG_RTE_LIBRTE_ENA_PMD=y
> > CONFIG_RTE_LIBRTE_EM_PMD=y
> > CONFIG_RTE_LIBRTE_IGB_PMD=y
> > CONFIG_RTE_LIBRTE_I40E_PMD=y
> > CONFIG_RTE_LIBRTE_AVF_PMD=y
> > CONFIG_RTE_LIBRTE_MLX5_PMD=y
> > CONFIG_RTE_LIBRTE_NFP_PMD=y
> > CONFIG_RTE_LIBRTE_QEDE_PMD=y
> > CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD=y
> > CONFIG_RTE_LIBRTE_LIO_PMD=y
> > CONFIG_RTE_LIBRTE_OCTEONTX_PMD=y
> > CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> > CONFIG_RTE_LIBRTE_NETVSC_PMD=y
> > CONFIG_RTE_LIBRTE_VDEV_NETVSC_PMD=y
> > CONFIG_RTE_LIBRTE_IFC_PMD=y
> > CONFIG_RTE_TEST_PMD=y


We maybe compiling many PMDs, but for testing purposes mlx5 will be the
main focus on the Power platform, particularly P9.


> >
> > > Are you compiling examples?


Yes, please see below for additional details.

> >
> > Yes, no build issues seen.
> >
> > >> > And there was no issue raised after 18.11-rc1 release.
> > >> > I guess it means DPDK is not used on ppc64.
> > >> > In this case, we should mark the ppc port as unmaintained for
18.11.
> > >> >
> > >> > OR SHOULD I USE MY CAPS LOCK AGAIN?
> > >>
> > >> Thanks for your patience while we iron out the issues.
> > >> Hopefully, we don't need the CAPS LOCK again.
> > >
> > > We have to mention the ppc64 incompatibility in 18.11 release notes.
> > > Either it stays as is and we declare DPDK 18.11 not supported on
> > > IBM platforms, or we fix it and document the limitations.
> >
> > If net-mlx5-fix-build-on-PPC64.patch is accepted I feel power can be
> > listed as supported for 18.11.
>
> I sent this last patch which was thought by Christian (Canonical) and
> Adrien (6WIND). It is just fixing the compilation.
> Is there someone at IBM checking that basic DPDK features are working?

Yes, we are in the process of attempting to run DTS and other tests as
well.
While we learn all of this, we didn't pay enough attention to some of the
recent 18.X releases.


Thanks
Pradeep
pradeep@us.ibm.com