Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.4.12 doesn't compile on ppc64/ppc64le with gcc7 #119

Closed
nirik opened this issue Feb 2, 2017 · 11 comments
Closed

0.4.12 doesn't compile on ppc64/ppc64le with gcc7 #119

nirik opened this issue Feb 2, 2017 · 11 comments

Comments

@nirik
Copy link

nirik commented Feb 2, 2017

Greetings.

I'm seeing ppc64/ppc64le builds fail with 0.4.12 and gcc7:
(This is Fedora rawhide)

+ CFLAGS='-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 -mcpu=power8 -mtune=power8'
+ /usr/bin/python2 setup.py build '--executable=/usr/bin/python2 -s'
running build
running build_ext
building 'greenlet' extension
creating build
creating build/temp.linux-ppc64le-2.7
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 -mcpu=power8 -mtune=power8 -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 -mcpu=power8 -mtune=power8 -D_GNU_SOURCE -fPIC -fwrapv -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 -mcpu=power8 -mtune=power8 -fPIC -I/usr/include/python2.7 -c greenlet.c -o build/temp.linux-ppc64le-2.7/greenlet.o -fno-tree-dominator-opts
In file included from slp_platformselect.h:16:0,
                 from greenlet.c:328:
platform/switch_ppc64_linux.h: In function 'slp_switch':
platform/switch_ppc64_linux.h:72:5: error: PIC register clobbered by 'r2' in 'asm'
     __asm__ volatile ("" : : : REGS_TO_SAVE);
     ^~~~~~~
platform/switch_ppc64_linux.h:85:5: error: PIC register clobbered by 'r2' in 'asm'
     __asm__ volatile ("" : : : REGS_TO_SAVE);
     ^~~~~~~
error: command 'gcc' failed with exit status 1
error: Bad exit status from /var/tmp/rpm-tmp.7B5UnQ (%build)
RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.7B5UnQ (%build)
Child return code was: 1

Full build at: https://koji.fedoraproject.org/koji/taskinfo?taskID=17554406

@snaury
Copy link
Contributor

snaury commented Feb 2, 2017

Was 0.4.11 working before?

@nirik
Copy link
Author

nirik commented Feb 2, 2017

Yep. With gcc 6.2.1.

This may be a bug in gcc 7

@snaury
Copy link
Contributor

snaury commented Feb 2, 2017

Probably not a bug, just a different compilation mode and now r2 is a PIC register, which is a pain to save and load. It needs to be saved/restored separately, but I'm not very familiar with the architecture, it would need something like:

void* pic;
__asm__ volatile ("<save r2 to %0>" : "=m" (pic));
// ... current save/load code
__asm__ volatile ("<load r2 from %0>" : : "m" (pic));

See other arches for similar technique, e.g. switch_amd64_unix.h. Unfortunately I don't know which instructions can actually be used for that.

@sharkcz
Copy link

sharkcz commented Feb 13, 2017

see xianyi/OpenBLAS#1078 for the same issue in OpenBLAS

@nehaljwani
Copy link

Debian folks seems to have applied this patch:

$ cat dont_clobber_r2_on_ppc64el.patch 
Description: don't touch 'r2' register on ppc64el
 It was invalid even before, now GCC 7 no longer accept this.
Author: Laszlo Boszormenyi (GCS) <gcs@debian.org>
Bug-Debian: https://bugs.debian.org/873694
Last-Update: 2017-09-09

---

--- python-greenlet-0.4.12.orig/platform/switch_ppc64_linux.h
+++ python-greenlet-0.4.12/platform/switch_ppc64_linux.h
@@ -56,7 +56,7 @@
 #define ALTIVEC_REGS
 #endif
 
-#define REGS_TO_SAVE "r2", "r14", "r15", "r16", "r17", "r18", "r19", "r20", \
+#define REGS_TO_SAVE "r14", "r15", "r16", "r17", "r18", "r19", "r20", \
        "r21", "r22", "r23", "r24", "r25", "r26", "r27", "r28", "r29", "r31", \
        "fr14", "fr15", "fr16", "fr17", "fr18", "fr19", "fr20", "fr21", \
        "fr22", "fr23", "fr24", "fr25", "fr26", "fr27", "fr28", "fr29", \

@snaury
Copy link
Contributor

snaury commented Jan 23, 2018

Debian folks seems to have applied this patch

I would like to remind everyone that Debian as an operating system can get away with it, because they control their build flag. Unfortunately greenlet as an upstream project cannot just remove the register, as it might result in silent intermittent failures for people compiling in non-pic modes. As I noted in #120 it needs more work by someone familiar with the architecture.

@tuliom
Copy link
Contributor

tuliom commented Mar 1, 2018

@snaury I have some knowledge about the 3 ABIs (ppc, ppc64 and ppc64le) and I think I can help.
I made some comments in PR #120 trying to point some problems there, but I haven't received a reply yet.
I'm afraid that PR and the patch posted in this issue are incomplete.

@snaury
Copy link
Contributor

snaury commented Mar 1, 2018

@tuliom yes, there are some additional things that needed to be done, could you please prepare a new corrected PR?

@tuliom
Copy link
Contributor

tuliom commented Mar 1, 2018

@snaury, if necessary, I can correct it, but I'll let the original author @gcsideal try it first.
Anyway, I don't have enough knowledge about greenlet and I'd appreciate if someone that knows it could answer my questions there.

@snaury
Copy link
Contributor

snaury commented Mar 2, 2018

@tuliom it's been almost 6 months, I'm not sure why you would want to wait any longer. I'm not even sure who could answer you regarding r30, I don't know why it's not there, but if your research shows it's a callee saved register, then it probably needs clobbering. I don't have any easy way to test it, so I'll trust whoever can.

I'm mostly concerned with proper attribution and correct saving/restoring of the frame pointer register.

@jamadden
Copy link
Contributor

This was fixed in #130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants