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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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