Re: [PATCH v3] ppc64: fix compilation of when AltiVec is enabled

[Thomas Monjalon <thomas@monjalon.net>]

03/09/2018 11:29, Adrien Mazarguil:
> Hi Christian,
> 
> Couldn't follow up on this last week, however I still have some concerns and
> comments, please see below.
> 
> On Thu, Aug 30, 2018 at 01:59:59PM +0200, Christian Ehrhardt wrote:
> > The definition of almost any newer standard like --stc=c11 will drop
> > __APPLCE_ALTIVEC__ which otherwise would be defined.
> > If that is the case then altivec.h will redefine bool to a type
> > conflicting with those defined by stdbool.h.
> > 
> > This breaks compilation of 18.08 on ppc64 like:
> >   mlx5_nl_flow.c:407:17: error: incompatible types when assigning
> >   to type ‘__vector __bool int’ {aka ‘__vector(4) __bool int’}
> >   from type ‘int’ in_port_id_set = false;
> > 
> > Other alternatives were pursued on [1] but they always ended up being
> > more complex than what would be appropriate for the issue we face.
> > 
> > [1]: http://mails.dpdk.org/archives/dev/2018-August/109926.html
> > 
> > Tested-by: Takeshi T Yoshimura <TYOS@jp.ibm.com>
> > Reviewed-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  .../common/include/arch/ppc_64/rte_memcpy.h           | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_memcpy.h
> > @@ -37,6 +37,17 @@
> >  #include <string.h>
> >  /*To include altivec.h, GCC version must  >= 4.8 */
> >  #include <altivec.h>
> > +/*
> > + * Compilation workaround for PPC64 targets when AltiVec is fully
> > + * enabled e.g. with std=c11. Otherwise there would be a type conflict
> > + * of "bool" between stdbool and altivec.
> > + */
> > +#if defined(__PPC64__) && !defined(__APPLE_ALTIVEC__)
> > + #undef bool
> > + /* redefine as in stdbool.h */
> > + #define bool _Bool
> > +#endif
> > +
> 
> The above will break existing C++ programs that include rte_memcpy.h.
> 
> Problem is that bool is an actual C++ type. C99 has _Bool which doesn't
> exist in C++ along with a bool macro that appears only after including
> stdbool.h.
> 
> To make things worse, nothing prevents C++ programs from importing a C-style
> bool macro by including stdbool.h (or cstdbool).
> 
> Enclosing it in #ifdef __cplusplus won't help because you never know what
> bool is supposed to be in the first place as it depends on how applications
> are written. I think something like this prior suggestion [1]
> (saving/restoring bool) is the only way to deal with that in a safe-ish
> fashion.
> 
> Pending something better, the above #undef/#define workaround is only safe
> to use inside mlx5 PMD code that triggers the compilation issue. It must not
> be found in a public header.
> 
> >  #ifdef __cplusplus
> >  extern "C" {
> 
> [1] https://mails.dpdk.org/archives/dev/2018-August/110401.html

Thank you for the review Adrien. I think we could accept the patch
if some notes mention it breaks C++ applications.
But...
After 2 months, nobody replied or complained about the issue not fixed.
So I classify this patch as rejected.

Summary of alerts here:
	http://mails.dpdk.org/archives/dev/2018-November/118259.html