xpra icon
Bug tracker and wiki

Opened 3 years ago

Closed 5 months ago

#1080 closed defect (fixed)

strict-aliasing rules build failures on some platforms

Reported by: Thomas Klausner Owned by: Eli Schwartz
Priority: major Milestone: 0.17
Component: core Version: 0.16.x
Keywords: Cc: eschwartz@…

Description

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.

Attachments (2)

silence-cython-strict-aliasing-warning.patch (686 bytes) - added by Antoine Martin 3 years ago.
silence the problematic warnings as per cython ticket
silence-cython-strict-aliasing-warning.2.patch (689 bytes) - added by Antoine Martin 3 years ago.
better patch - checks for netbsd

Download all attachments as: .zip

Change History (25)

Changed 3 years ago by Antoine Martin

silence the problematic warnings as per cython ticket

comment:1 Changed 3 years ago by Antoine Martin

Owner: changed from Antoine Martin to Thomas Klausner

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 http://trac.cython.org/ticket/395 and Strict aliasing warnings on generated code.

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

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by 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.

Changed 3 years ago by Antoine Martin

better patch - checks for netbsd

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

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.

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by Antony Lee

Resolution: fixed
Status: closedreopened

comment:8 Changed 3 years ago by Antoine Martin

Owner: changed from Thomas Klausner to Antony Lee
Status: reopenednew

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

comment:9 Changed 3 years ago by Antoine Martin

Summary: xpra-0.16.0 does not buildstrict-aliasing rules build failures on some platforms

(editing ticket title)

comment:10 Changed 3 years ago by Antony Lee

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

Last edited 3 years ago by Antony Lee (previous) (diff)

comment:11 Changed 3 years ago by 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)

comment:12 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

Thanks!

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

comment:14 Changed 3 years ago by 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...

comment:15 Changed 3 years ago by 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.

comment:16 Changed 6 months ago by Eli Schwartz

Resolution: fixed
Status: closedreopened

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.

comment:17 Changed 6 months ago by Eli Schwartz

Cc: eschwartz@… added

comment:18 Changed 6 months ago by Antoine Martin

Owner: changed from Antony Lee to Eli Schwartz
Status: reopenednew

@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

comment:19 Changed 6 months ago by 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.

comment:20 Changed 6 months ago by 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.

comment:21 in reply to:  20 Changed 6 months ago by 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.

comment:22 Changed 5 months ago by bug

The patch mentioned above works on trunk and 2.3.

comment:23 Changed 5 months ago by Antoine Martin

Resolution: fixed
Status: newclosed

The patch mentioned above works on trunk and 2.3.

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

Note: See TracTickets for help on using tickets.