[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Sergio Prado]

PPC altivec vectorization triggers a bug when compiling with -std=c++11
because "bool" is redefined in altivec.h.

src/emucore/Event.hxx:112:23: error: cannot convert ?bool? to ?__vector(4) __bool int? in assignment
         myKeyTable[i] = false;
                       ^

Acording to a bug report in GCC [1], "You need to use -std=g++11 or
undefine bool after the include of altivec.h as context sensitive
keywords is not part of the C++11 standard".

So let's compile with -std=gnu++11 when PPC altivec is enabled.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58241#c3

Fixes:
http://autobuild.buildroot.net/results/0970d2c8e1787ceffc46b589522e53d52675e03c
http://autobuild.buildroot.net/results/ec1bc57675b6e53af0cd33d7b99cd2e3bf5d9d7e

Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
---
 ...XFLAGS-so-we-can-append-values-to-user-de.patch | 40 ++++++++++++++++++++++
 package/stella/stella.mk                           | 14 +++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch

diff --git a/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
new file mode 100644
index 000000000000..7e82c571e2c1
--- /dev/null
+++ b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
@@ -0,0 +1,40 @@
+From f81bec4d6e523df308158d6bd6f948be4d0183ba Mon Sep 17 00:00:00 2001
+From: Sergio Prado <sergio.prado@e-labworks.com>
+Date: Thu, 17 Nov 2016 15:26:56 -0200
+Subject: [PATCH] Override CXXFLAGS so we can append values to user-defined
+ CXXFLAGS.
+
+Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
+---
+ Makefile | 8 ++++----
+ 1 file changed, 4 insertions(+), 4 deletions(-)
+
+diff --git a/Makefile b/Makefile
+index 6dd0129587b3..7133ca58ac49 100644
+--- a/Makefile
++++ b/Makefile
+@@ -49,17 +49,17 @@ ifdef CXXFLAGS
+ else
+   CXXFLAGS:= -O2
+ endif
+-CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
++override CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
+ ifdef HAVE_GCC
+-  CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=c++11
++  override CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor
+ endif
+ 
+ ifdef PROFILE
+   PROF:= -g -pg -fprofile-arcs -ftest-coverage
+-  CXXFLAGS+= $(PROF)
++  override CXXFLAGS+= $(PROF)
+ else
+   ifdef HAVE_GCC
+-    CXXFLAGS+= -fomit-frame-pointer
++    override CXXFLAGS+= -fomit-frame-pointer
+   endif
+ endif
+ 
+-- 
+1.9.1
+
diff --git a/package/stella/stella.mk b/package/stella/stella.mk
index 2e9d57b8c1ea..11cdd6adcd97 100644
--- a/package/stella/stella.mk
+++ b/package/stella/stella.mk
@@ -12,11 +12,23 @@ STELLA_LICENSE_FILES = Copyright.txt License.txt
 
 STELLA_DEPENDENCIES = sdl2 libpng zlib
 
+STELLA_CXXFLAGS = $(TARGET_CFLAGS)
+
+# PPC altivec vectorization triggers a bug when compiling with -std=c++11
+# so let's compile it with -std=gnu++11
+ifeq ($(BR2_POWERPC_CPU_HAS_ALTIVEC),y)
+STELLA_CXXFLAGS += -std=gnu++11
+else
+STELLA_CXXFLAGS += -std=c++11
+endif
+
 STELLA_CONF_OPTS = \
 	--host=$(GNU_TARGET_NAME) \
 	--prefix=/usr \
 	--with-sdl-prefix=$(STAGING_DIR)/usr
 
+STELLA_MAKE_OPTS += CXXFLAGS="$(STELLA_CXXFLAGS)"
+
 # The configure script is not autoconf based, so we use the
 # generic-package infrastructure
 define STELLA_CONFIGURE_CMDS
@@ -28,7 +40,7 @@ define STELLA_CONFIGURE_CMDS
 endef
 
 define STELLA_BUILD_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)
+	$(TARGET_MAKE_ENV) $(MAKE) $(STELLA_MAKE_OPTS) -C $(@D)
 endef
 
 define STELLA_INSTALL_TARGET_CMDS
-- 
1.9.1

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Thomas Petazzoni]

Sam,

Could you review the below patch, which is PowerPC/Altivec related, and
let us know what you think?

Thanks!

Thomas

On Thu, 17 Nov 2016 15:47:12 -0200, Sergio Prado wrote:
> PPC altivec vectorization triggers a bug when compiling with -std=c++11
> because "bool" is redefined in altivec.h.
> 
> src/emucore/Event.hxx:112:23: error: cannot convert ?bool? to ?__vector(4) __bool int? in assignment
>          myKeyTable[i] = false;
>                        ^
> 
> Acording to a bug report in GCC [1], "You need to use -std=g++11 or
> undefine bool after the include of altivec.h as context sensitive
> keywords is not part of the C++11 standard".
> 
> So let's compile with -std=gnu++11 when PPC altivec is enabled.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58241#c3
> 
> Fixes:
> http://autobuild.buildroot.net/results/0970d2c8e1787ceffc46b589522e53d52675e03c
> http://autobuild.buildroot.net/results/ec1bc57675b6e53af0cd33d7b99cd2e3bf5d9d7e
> 
> Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> ---
>  ...XFLAGS-so-we-can-append-values-to-user-de.patch | 40 ++++++++++++++++++++++
>  package/stella/stella.mk                           | 14 +++++++-
>  2 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100644 package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> 
> diff --git a/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> new file mode 100644
> index 000000000000..7e82c571e2c1
> --- /dev/null
> +++ b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> @@ -0,0 +1,40 @@
> +From f81bec4d6e523df308158d6bd6f948be4d0183ba Mon Sep 17 00:00:00 2001
> +From: Sergio Prado <sergio.prado@e-labworks.com>
> +Date: Thu, 17 Nov 2016 15:26:56 -0200
> +Subject: [PATCH] Override CXXFLAGS so we can append values to user-defined
> + CXXFLAGS.
> +
> +Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> +---
> + Makefile | 8 ++++----
> + 1 file changed, 4 insertions(+), 4 deletions(-)
> +
> +diff --git a/Makefile b/Makefile
> +index 6dd0129587b3..7133ca58ac49 100644
> +--- a/Makefile
> ++++ b/Makefile
> +@@ -49,17 +49,17 @@ ifdef CXXFLAGS
> + else
> +   CXXFLAGS:= -O2
> + endif
> +-CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
> ++override CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
> + ifdef HAVE_GCC
> +-  CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=c++11
> ++  override CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor
> + endif
> + 
> + ifdef PROFILE
> +   PROF:= -g -pg -fprofile-arcs -ftest-coverage
> +-  CXXFLAGS+= $(PROF)
> ++  override CXXFLAGS+= $(PROF)
> + else
> +   ifdef HAVE_GCC
> +-    CXXFLAGS+= -fomit-frame-pointer
> ++    override CXXFLAGS+= -fomit-frame-pointer
> +   endif
> + endif
> + 
> +-- 
> +1.9.1
> +
> diff --git a/package/stella/stella.mk b/package/stella/stella.mk
> index 2e9d57b8c1ea..11cdd6adcd97 100644
> --- a/package/stella/stella.mk
> +++ b/package/stella/stella.mk
> @@ -12,11 +12,23 @@ STELLA_LICENSE_FILES = Copyright.txt License.txt
>  
>  STELLA_DEPENDENCIES = sdl2 libpng zlib
>  
> +STELLA_CXXFLAGS = $(TARGET_CFLAGS)
> +
> +# PPC altivec vectorization triggers a bug when compiling with -std=c++11
> +# so let's compile it with -std=gnu++11
> +ifeq ($(BR2_POWERPC_CPU_HAS_ALTIVEC),y)
> +STELLA_CXXFLAGS += -std=gnu++11
> +else
> +STELLA_CXXFLAGS += -std=c++11
> +endif
> +
>  STELLA_CONF_OPTS = \
>  	--host=$(GNU_TARGET_NAME) \
>  	--prefix=/usr \
>  	--with-sdl-prefix=$(STAGING_DIR)/usr
>  
> +STELLA_MAKE_OPTS += CXXFLAGS="$(STELLA_CXXFLAGS)"
> +
>  # The configure script is not autoconf based, so we use the
>  # generic-package infrastructure
>  define STELLA_CONFIGURE_CMDS
> @@ -28,7 +40,7 @@ define STELLA_CONFIGURE_CMDS
>  endef
>  
>  define STELLA_BUILD_CMDS
> -	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)
> +	$(TARGET_MAKE_ENV) $(MAKE) $(STELLA_MAKE_OPTS) -C $(@D)
>  endef
>  
>  define STELLA_INSTALL_TARGET_CMDS



-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Sam Bobroff]

On Sat, Nov 19, 2016 at 10:59:27AM +0100, Thomas Petazzoni wrote:
> Sam,
> 
> Could you review the below patch, which is PowerPC/Altivec related, and
> let us know what you think?

Sure.

> Thanks!
> 
> Thomas
> 
> On Thu, 17 Nov 2016 15:47:12 -0200, Sergio Prado wrote:
> > PPC altivec vectorization triggers a bug when compiling with -std=c++11
> > because "bool" is redefined in altivec.h.
> > 
> > src/emucore/Event.hxx:112:23: error: cannot convert ?bool? to ?__vector(4) __bool int? in assignment
> >          myKeyTable[i] = false;
> >                        ^
> > 
> > Acording to a bug report in GCC [1], "You need to use -std=g++11 or
> > undefine bool after the include of altivec.h as context sensitive
> > keywords is not part of the C++11 standard".
> > 
> > So let's compile with -std=gnu++11 when PPC altivec is enabled.

Yes, this seems to be the same problem I saw in SDL. Using the gnu
standard seems like a good fix to me but I've got a suggestion, below.

> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58241#c3
> > 
> > Fixes:
> > http://autobuild.buildroot.net/results/0970d2c8e1787ceffc46b589522e53d52675e03c
> > http://autobuild.buildroot.net/results/ec1bc57675b6e53af0cd33d7b99cd2e3bf5d9d7e
> > 
> > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> > ---
> >  ...XFLAGS-so-we-can-append-values-to-user-de.patch | 40 ++++++++++++++++++++++
> >  package/stella/stella.mk                           | 14 +++++++-
> >  2 files changed, 53 insertions(+), 1 deletion(-)
> >  create mode 100644 package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> > 
> > diff --git a/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> > new file mode 100644
> > index 000000000000..7e82c571e2c1
> > --- /dev/null
> > +++ b/package/stella/0004-Override-CXXFLAGS-so-we-can-append-values-to-user-de.patch
> > @@ -0,0 +1,40 @@
> > +From f81bec4d6e523df308158d6bd6f948be4d0183ba Mon Sep 17 00:00:00 2001
> > +From: Sergio Prado <sergio.prado@e-labworks.com>
> > +Date: Thu, 17 Nov 2016 15:26:56 -0200
> > +Subject: [PATCH] Override CXXFLAGS so we can append values to user-defined
> > + CXXFLAGS.
> > +
> > +Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com>
> > +---
> > + Makefile | 8 ++++----
> > + 1 file changed, 4 insertions(+), 4 deletions(-)
> > +
> > +diff --git a/Makefile b/Makefile
> > +index 6dd0129587b3..7133ca58ac49 100644
> > +--- a/Makefile
> > ++++ b/Makefile
> > +@@ -49,17 +49,17 @@ ifdef CXXFLAGS
> > + else
> > +   CXXFLAGS:= -O2
> > + endif
> > +-CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
> > ++override CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
> > + ifdef HAVE_GCC
> > +-  CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=c++11
> > ++  override CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor
> > + endif
> > + 
> > + ifdef PROFILE
> > +   PROF:= -g -pg -fprofile-arcs -ftest-coverage
> > +-  CXXFLAGS+= $(PROF)
> > ++  override CXXFLAGS+= $(PROF)
> > + else
> > +   ifdef HAVE_GCC
> > +-    CXXFLAGS+= -fomit-frame-pointer
> > ++    override CXXFLAGS+= -fomit-frame-pointer
> > +   endif
> > + endif
> > + 
> > +-- 
> > +1.9.1
> > +
> > diff --git a/package/stella/stella.mk b/package/stella/stella.mk
> > index 2e9d57b8c1ea..11cdd6adcd97 100644
> > --- a/package/stella/stella.mk
> > +++ b/package/stella/stella.mk
> > @@ -12,11 +12,23 @@ STELLA_LICENSE_FILES = Copyright.txt License.txt
> >  
> >  STELLA_DEPENDENCIES = sdl2 libpng zlib
> >  
> > +STELLA_CXXFLAGS = $(TARGET_CFLAGS)
> > +
> > +# PPC altivec vectorization triggers a bug when compiling with -std=c++11
> > +# so let's compile it with -std=gnu++11
> > +ifeq ($(BR2_POWERPC_CPU_HAS_ALTIVEC),y)
> > +STELLA_CXXFLAGS += -std=gnu++11
> > +else
> > +STELLA_CXXFLAGS += -std=c++11
> > +endif
> > +
> >  STELLA_CONF_OPTS = \
> >  	--host=$(GNU_TARGET_NAME) \
> >  	--prefix=/usr \
> >  	--with-sdl-prefix=$(STAGING_DIR)/usr
> >  
> > +STELLA_MAKE_OPTS += CXXFLAGS="$(STELLA_CXXFLAGS)"
> > +
> >  # The configure script is not autoconf based, so we use the
> >  # generic-package infrastructure
> >  define STELLA_CONFIGURE_CMDS
> > @@ -28,7 +40,7 @@ define STELLA_CONFIGURE_CMDS
> >  endef
> >  
> >  define STELLA_BUILD_CMDS
> > -	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)
> > +	$(TARGET_MAKE_ENV) $(MAKE) $(STELLA_MAKE_OPTS) -C $(@D)
> >  endef
> >  
> >  define STELLA_INSTALL_TARGET_CMDS

This seems fine to me. I assume you're using this method so that the
change has minimal chance to break other arches, but it would be much
simpler if you could just change it globally so let's consider it:

What seems to be happening before this patch, is:

The configure script selects a compiler that supports -std=c++11 (see
configure around line 805).

Then the Makefile wll add -std=c++11 if the compiler is GCC, like this:

ifdef HAVE_GCC
  CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=c++11
endif

(But it looks like non-gcc compilers that do support -std=c++11 will be
used but they won't get -std=c++11... odd. I think the proposed patch, above,
would change this slightly so that -std=c++11 is always used but I
suspect that is a good thing!)

If we can rely on c++11 support in gcc implying gnu++11 support (which
seems reasonable), we could just patch the Makefile to:

ifdef HAVE_GCC
  CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=gnu++11
endif

What do you think?

Cheers,
Sam.

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Sergio Prado]

Hi Sam,

Thanks for reviewing the patch.

> This seems fine to me. I assume you're using this method so that the
> change has minimal chance to break other arches, but it would be much
> simpler if you could just change it globally so let's consider it:
>
> What seems to be happening before this patch, is:
>
> The configure script selects a compiler that supports -std=c++11 (see
> configure around line 805).
>
> Then the Makefile wll add -std=c++11 if the compiler is GCC, like this:
>
> ifdef HAVE_GCC
>   CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual
-Wnon-virtual-dtor -std=c++11
> endif
>
> (But it looks like non-gcc compilers that do support -std=c++11 will be
> used but they won't get -std=c++11... odd. I think the proposed patch,
above,
> would change this slightly so that -std=c++11 is always used but I
> suspect that is a good thing!)
>
> If we can rely on c++11 support in gcc implying gnu++11 support (which
> seems reasonable), we could just patch the Makefile to:
>
> ifdef HAVE_GCC
>   CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual
-Wnon-virtual-dtor -std=gnu++11
> endif
>
> What do you think?

Your solution is simpler and seems reasonable to just change from c++11 to
gnu++11. I will do some tests on x86 and ARM and let you know the results.

Best regards,

Sergio Prado

>
> Cheers,
> Sam.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20161123/a28e20ac/attachment.html>

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Thomas Petazzoni]

Hello,

On Wed, 23 Nov 2016 19:59:11 -0200, Sergio Prado wrote:

> Your solution is simpler and seems reasonable to just change from c++11 to
> gnu++11. I will do some tests on x86 and ARM and let you know the results.

Great, thanks. I'll mark your patch as Changes Requested in patchwork.
Can you resubmit a simplified version?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Sergio Prado]

Hi Thomas,

2016-11-26 12:37 GMT-02:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:
>
> Hello,
>
> On Wed, 23 Nov 2016 19:59:11 -0200, Sergio Prado wrote:
>
> > Your solution is simpler and seems reasonable to just change from c++11
to
> > gnu++11. I will do some tests on x86 and ARM and let you know the
results.
>
> Great, thanks. I'll mark your patch as Changes Requested in patchwork.
> Can you resubmit a simplified version?

Sure. I'll finish testing and submit v2 in the next days.

Best regards,

Sergio Prado

>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20161126/c75b8d8f/attachment.html>

[Buildroot] [PATCH] stella: fix bug when compiling with PPC altivec vectorization

[Arnout Vandecappelle]

On 17-11-16 18:47, Sergio Prado wrote:
> Acording to a bug report in GCC [1], "You need to use -std=g++11 or
> undefine bool after the include of altivec.h as context sensitive
> keywords is not part of the C++11 standard".
> 
> So let's compile with -std=gnu++11 when PPC altivec is enabled.

 So this is the second package that requires this kind of hack. I wonder if we
shouldn't try to fix this globally in the toolchain wrapper. Like, search for
'--std=c++' in the arguments and replace it with '--std=gnu++' (while conserving
the numbers behind the ++, so not entirely trivial...).


 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF