Xpra: Ticket #1080: strict-aliasing rules build failures on some platforms

I've tried compiling xpra-0.16.0 with python-2.7.11 on NetBSD, but it fails for me with:

gcc -DNDEBUG -O2 -D_FORTIFY_SOURCE=2 -fstack-protector -DHAVE_DB_185_H -pthread -I/usr/include -I/usr/pkg/include -O2 -D_FORTIFY_SOURCE=2 -fstack-protector -I/usr/pkg/include -I/usr/include -I/usr/pkg/include/ffmpeg1 -I/usr/pkg/include/freetype2 -I/usr/pkg/include/libdrm -I/usr/pkg/include -I/usr/include -I/usr/pkg/include/ffmpeg1 -I/usr/pkg/include/freetype2 -I/usr/pkg/include/libdrm -fPIC -I/usr/pkg/include -I/usr/pkg/include/python2.7 -c xpra/x11/bindings/randr_bindings.c -o build/temp.netbsd-7.99.25-amd64-2.7/xpra/x11/bindings/randr_bindings.o -Wall -Werror -fPIC
xpra/x11/bindings/randr_bindings.c: In function '__pyx_pf_4xpra_3x11_8bindings_14randr_bindings_13RandRBindings_4check_randr':
xpra/x11/bindings/randr_bindings.c:1140:9: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
         __Pyx_INCREF(Py_True);
         ^
xpra/x11/bindings/randr_bindings.c:1179:3: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
   __Pyx_INCREF(Py_False);
   ^
xpra/x11/bindings/randr_bindings.c: In function '__pyx_pf_4xpra_3x11_8bindings_14randr_bindings_13RandRBindings_6has_randr':
xpra/x11/bindings/randr_bindings.c:1250:3: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
   __pyx_t_1 = __Pyx_PyBool_FromLong((!(!__pyx_t_2))); if (unlikely(!__pyx_t_1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 98; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
   ^
xpra/x11/bindings/randr_bindings.c:1250:3: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
xpra/x11/bindings/randr_bindings.c: In function '__pyx_f_4xpra_3x11_8bindings_14randr_bindings_13RandRBindings__set_screen_size':
xpra/x11/bindings/randr_bindings.c:1774:7: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
       __Pyx_INCREF(Py_False);
       ^
xpra/x11/bindings/randr_bindings.c:1867:7: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
       __Pyx_INCREF(Py_False);
       ^
xpra/x11/bindings/randr_bindings.c:1888:5: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
     __Pyx_INCREF(Py_True);
     ^
cc1: all warnings being treated as errors
error: command 'gcc' failed with exit status 1
*** Error code 1

That's when building against the gtk2 backend because I don't know how to build against the gtk3 one instead.



Thu, 07 Jan 2016 14:56:18 GMT - Antoine Martin: attachment set

silence the problematic warnings as per cython ticket


Thu, 07 Jan 2016 14:56:50 GMT - Antoine Martin: owner changed

Someone else reported this but I have never seen it. Which gcc and Cython version are you using?

You may want to add -fno-strict-aliasing to ignore this problem as per Strict aliasing warnings on generated code.

Does the patch attached fix things? (you may need to edit the gcc version check)


Thu, 07 Jan 2016 15:32:05 GMT - Thomas Klausner:

I'm using gcc-4.8.5 (nb3 20151015) on NetBSD-7.99.25/amd64 with python-2.7.11 and cython 0.23.4.

I tried your patch and changed 5,2 to 4,8, but I still see the same failure; the additional flag does not end up on the relevant command line.


Thu, 07 Jan 2016 15:48:15 GMT - Antoine Martin:

Yeah, the patch is wrong I think, try moving it above the check for 4.4, or merging it into that section instead.


Thu, 07 Jan 2016 15:53:54 GMT - Antoine Martin: attachment set

better patch - checks for netbsd


Thu, 07 Jan 2016 16:05:44 GMT - Thomas Klausner:

I've tested your netbsd-patch and it works for me. Thank you!

I don't think that testing for NetBSD is right though, it's a compiler (flags) issue, not one of the operating system.


Fri, 08 Jan 2016 02:36:48 GMT - Antoine Martin: status changed; resolution set

Merged in r11614, if and when others report what platforms and toolchains hit this strict aliasing warning, we can change the platform check to something else. Closing.


Thu, 18 Feb 2016 08:45:20 GMT - Antony Lee:

Kindly requesting this to be reopened, I also see this issue with Python 2.7.11, Cython 0.23.4, gcc 5.3.0 on Arch Linux (standard packages) with a current svn checkout.


Thu, 18 Feb 2016 08:45:37 GMT - Antony Lee: status changed; resolution deleted


Thu, 18 Feb 2016 08:54:53 GMT - Antoine Martin: owner, status changed

I have gcc 5.3.1, cython 0.23.4, Python 2.7.11 and I am not seeing this warning:

gcc -pthread -fno-strict-aliasing -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions \
    -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 \
    -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv \
    -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions \
    -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 \
    -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv \
    -fPIC -I/usr/include/python2.7 -c xpra/x11/bindings/randr_bindings.c \
    -o build/temp.linux-x86_64-2.7/xpra/x11/bindings/randr_bindings.o -Wall -Werror -fPIC

Just to be clear: this code is generated by Cython, and looks correct too. So.. until this warning starts showing up in Fedora / Debian, I will not change the code to accommodate Arch Linux, I can only suggest that you patch the build file to silence the warning when building on Arch. Unless someone can come up with a way to detect the systems that will barf on this and turn off the warning only when needed. (but not using autoconf!)


Thu, 18 Feb 2016 08:56:10 GMT - Antoine Martin: summary changed

(editing ticket title)


Thu, 18 Feb 2016 18:01:37 GMT - Antony Lee:

<edited out> sorry, silly misread. I edited the Arch Linux build file to solve the issue.


Fri, 19 Feb 2016 02:39:59 GMT - Antoine Martin:

I edited the Arch Linux build file to solve the issue.


Can I close this ticket? Can you please link to the change you made?

My understanding is that python is meant to be built with -fno-strict-aliasing, and the cython extensions should use the same flags. I would expect this to work out of the box without us tweaking our build file (and I am tempted to undo the change in r11614 too)


Fri, 19 Feb 2016 02:43:52 GMT - Antony Lee:

The Arch Linux build file for xpra/svn is https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=xpra-winswitch-svn (format should be self-evident)

The build file for the latest xpra release is https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=xpra-winswitch

so I just added CFLAGS=... to the first one. I agree that Python2 is "known" to require -fno-strict-aliasing (http://legacy.python.org/dev/peps/pep-3123/) so you can assume it's there. Feel free to close this.


Fri, 19 Feb 2016 02:45:17 GMT - Antoine Martin: status changed; resolution set

Thanks!

Note to _wiz_: please see the comments above, I may remove the netbsd hack, please update your cflags.


Fri, 19 Feb 2016 11:22:09 GMT - Thomas Klausner:

I'm confused about who is supposed to set -fno-strict-aliasing. If it is a known problem that a part of xpra won't build without that flag, why doesn't xpra set it?

This should at least be mentioned in a README...


Sat, 20 Feb 2016 17:16:36 GMT - Antoine Martin:

_wiz_: The cython extensions need to be built with the same flags as python, every other distro gets it right without us guessing what flags should or should not be used.


Wed, 25 Apr 2018 19:53:26 GMT - Eli Schwartz: status changed; resolution deleted

This happens because https://bugs.python.org/issue969718

Building any python extension when the CFLAGS are set from the environment will completely ignore the CFLAGS python itself was built with.

Fedora locally patches this: https://src.fedoraproject.org/rpms/python2/blob/master/f/00168-distutils-cflags.patch

Debian locally patches this too: https://bazaar.launchpad.net/~doko/python/pkg2.7-debian/view/head:/patches/distutils-sysconfig.diff

Arch Linux does not, and possibly many other distros. People installing python directly from source will certainly not have these patches.

I think the best idea is to take the value of

import sysconfig
sysconfig.get_config_var('CFLAGS')

and add it to the CFLAGS used in your setup.py until the PSF can fix this 13-year-old bug.


Wed, 25 Apr 2018 19:57:48 GMT - Eli Schwartz: cc set


Thu, 26 Apr 2018 06:42:48 GMT - Antoine Martin: owner, status changed

@eschwartz: does this patch work for you?

Index: setup.py
===================================================================
--- setup.py	(revision 19079)
+++ setup.py	(working copy)
@@ -624,6 +624,14 @@
     ignored_flags = kw.pop("ignored_flags", [])
     ignored_tokens = kw.pop("ignored_tokens", [])
+    #for distros that don't patch distutils,
+    #we have to add the python cflags:
+    if not (is_Fedora() or is_Debian() or is_CentOS()):
+        import shlex
+        import sysconfig
+        for x in shlex.split(sysconfig.get_config_var('CFLAGS') or ''):
+            add_to_keywords(kw, 'extra_compile_args', x)
+
     if len(pkgs_options)>0:
         package_names = []
         #find out which package name to use from potentially many options

Thu, 26 Apr 2018 11:16:21 GMT - Eli Schwartz:

Original:

gcc -pthread -DNDEBUG -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -march=native -ggdb -D_FORTIFY_SOURCE=2 -fPIC -Ixpra/buffers -I/usr/include/python2.7 -c xpra/buffers/membuf.c -o build/temp.linux-x86_64-2.7/xpra/buffers/membuf.o -O3 -Wall -Werror -fPIC

With the patch:

Traceback (most recent call last):
  File "setup.py", line 1672, in <module>
    buffers_pkgconfig = pkgconfig(optimize=3)
  File "setup.py", line 628, in exec_pkgconfig
    if not (is_Fedora() or is_Debian() or is_CentOS()):
NameError: global name 'is_Fedora' is not defined

Note I'm using the PKGBUILD for the stable 2.2.6 release, so possibly this has been added since then? However... it shouldn't hurt to add these twice to the gcc command line, except for making the build logs somewhat messier. Up to you if you want to code special detection logic for that use case.

Unconditionally setting it:

gcc -pthread -DNDEBUG -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -march=native -ggdb -D_FORTIFY_SOURCE=2 -fPIC -Ixpra/buffers -I/usr/include/python2.7 -c xpra/buffers/membuf.c -o build/temp.linux-x86_64-2.7/xpra/buffers/membuf.o -O3 -fno-strict-aliasing -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -DNDEBUG -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -Wall -Werror -fPIC

This "works" in the sense of preventing compiler errors, but only by clobbering the environment CFLAGS with the python CFLAGS. The options need to be set *before* the environment CFLAGS, to ensure they are properly overridden by anything the user explicitly sets. e.g. -march=native.


Thu, 26 Apr 2018 12:00:18 GMT - Antoine Martin:

Note I'm using the PKGBUILD for the stable 2.2.6 release, so possibly this has been added since then?

All development is always done against trunk. In this case, for the 2.2.x branch, you can use add from xpra.os_util import is_Fedora above the if statement.


Thu, 26 Apr 2018 12:33:12 GMT - Giancarlo Razzolini:

Replying to Antoine Martin:

Note I'm using the PKGBUILD for the stable 2.2.6 release, so possibly this has been added since then?

All development is always done against trunk. In this case, for the 2.2.x branch, you can use add from xpra.os_util import is_Fedora above the if statement.

I'm a developer on Arch Linux and I'm trying to bring xpra from our unofficial repository:

https://aur.archlinux.org/packages/xpra/

To the main repos. I have based my PKGBUILD on that one and I'm currently building it like this:

https://paste.xinu.at/ZGcX32/

Eli was kind to help me review this and this is the reason why he re-opened this issue.


Tue, 15 May 2018 17:07:18 GMT - bug:

The patch mentioned above works on trunk and 2.3.


Tue, 15 May 2018 17:13:04 GMT - Antoine Martin: status changed; resolution set

The patch mentioned above works on trunk and 2.3.

Thanks, applied in r19327. (will backport to 2.3)


Sat, 23 Jan 2021 05:14:24 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/1080