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

Build fails on ppc64 #139

Closed
J0WI opened this issue Oct 26, 2018 · 15 comments
Closed

Build fails on ppc64 #139

J0WI opened this issue Oct 26, 2018 · 15 comments

Comments

@J0WI
Copy link

J0WI commented Oct 26, 2018

creating build/temp.linux-ppc64le-3.6
gcc -DNDEBUG -g -fwrapv -O3 -Wall -O0 -O0 -fPIC -I/usr/include/python3.6m -c greenlet.c -o build/temp.linux-ppc64le-3.6/greenlet.o -fno-tree-dominator-opts
In file included from slp_platformselect.h:16,
                 from greenlet.c:343:
platform/switch_ppc64_linux.h: In function 'slp_switch':
platform/switch_ppc64_linux.h:103:1: error: 31 cannot be used in asm here
 }
 ^
platform/switch_ppc64_linux.h:103:1: error: 31 cannot be used in asm here
error: command 'gcc' failed with exit status 1

http://build.alpinelinux.org/buildlogs/build-edge-ppc64le/community/py-greenlet/py-greenlet-0.4.15-r0.log

A patch is provided here:
https://github.com/alpinelinux/aports/pull/5505/files#diff-f4fe2c03cb2e1908147a049fea8c41d7
Introduced in:
alpinelinux/aports@511302f

@andypost
Copy link

andypost commented Oct 27, 2018

Removal of 31 register helps, the same was in #94 (comment) so I removed it alpinelinux/aports@4a696c3

But after successful build tests on py36 fails

Linking /home/andypost/aports/community/py-greenlet/src/greenlet-0.4.15/build/lib.linux-ppc64le-3.6/greenlet.cpython-36m-powerpc64le-linux-gnu.so to /home/andypost/aports/community/py-greenlet/src/greenlet-0.4.15/greenlet.cpython-36m-powerpc64le-linux-gnu.so
test_circular_greenlet (tests.test_gc.GCTests) ... ok
test_dead_circular_ref (tests.test_gc.GCTests) ... qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault (core dumped)

@andypost
Copy link

On builders it fails different

Linking /home/buildozer/aports/community/py-greenlet/src/greenlet-0.4.15/build/lib.linux-ppc64le-3.6/greenlet.cpython-36m-powerpc64le-linux-gnu.so to /home/buildozer/aports/community/py-greenlet/src/greenlet-0.4.15/greenlet.cpython-36m-powerpc64le-linux-gnu.so
test_version (tests.test_version.VersionTests) ... ok
test_genlet_bad (tests.test_generator_nested.NestedGeneratorTests) ... ok
test_genlet_simple (tests.test_generator_nested.NestedGeneratorTests) ... Segmentation fault

@snaury
Copy link
Contributor

snaury commented Oct 27, 2018

Why people think that removing these registers to fix compilation issues is ever ok?

Try this patch, maybe it would help:

diff --git a/platform/switch_ppc64_linux.h b/platform/switch_ppc64_linux.h
index 88e6847..d534a9b 100644
--- a/platform/switch_ppc64_linux.h
+++ b/platform/switch_ppc64_linux.h
@@ -66,7 +66,6 @@
 
 #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", \
        "fr30", "fr31", \
@@ -80,9 +79,11 @@ slp_switch(void)
     register long *stackref, stsizediff;
     void * toc;
     void * r30;
+    void * r31;
     __asm__ volatile ("" : : : REGS_TO_SAVE);
     __asm__ volatile ("std 2, %0" : "=m" (toc));
     __asm__ volatile ("std 30, %0" : "=m" (r30));
+    __asm__ volatile ("std 31, %0" : "=m" (r31));
     __asm__ ("mr %0, 1" : "=r" (stackref) : );
     {
         SLP_SAVE_STATE(stackref, stsizediff);
@@ -95,6 +96,7 @@ slp_switch(void)
             );
         SLP_RESTORE_STATE();
     }
+    __asm__ volatile ("ld 31, %0" : : "m" (r31));
     __asm__ volatile ("ld 30, %0" : : "m" (r30));
     __asm__ volatile ("ld 2, %0" : : "m" (toc));
     __asm__ volatile ("" : : : REGS_TO_SAVE);

@snaury
Copy link
Contributor

snaury commented Oct 27, 2018

However looking at #94 and remembering the workaround in 7352e9f it may be wrong, depends on the code that's generated.

@andypost
Copy link

This patch helped to build but test still fails, I used to try -03 - -Os

@andypost
Copy link

platform.machine() reports ppc64le I guess that's why https://github.com/alpinelinux/aports/blob/master/community/py-greenlet/APKBUILD#L21 it was forced to -O0

@andypost
Copy link

@snaury As you can see in builder log it running with -fomit-frame-pointer and this register (31) is in the ABI v2 a framepointer so should get clobbering

Ref http://build.alpinelinux.org/buildlogs/build-edge-ppc64le/community/py-greenlet/py-greenlet-0.4.15-r0.log

@snaury
Copy link
Contributor

snaury commented Oct 27, 2018

I haven't checked, but if it's a frame pointer then moving it with the stack may help:

diff --git a/platform/switch_ppc64_linux.h b/platform/switch_ppc64_linux.h
index 88e6847..feb60d4 100644
--- a/platform/switch_ppc64_linux.h
+++ b/platform/switch_ppc64_linux.h
@@ -66,7 +66,6 @@
 
 #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", \
        "fr30", "fr31", \
@@ -80,21 +79,25 @@ slp_switch(void)
     register long *stackref, stsizediff;
     void * toc;
     void * r30;
+    void * r31;
     __asm__ volatile ("" : : : REGS_TO_SAVE);
     __asm__ volatile ("std 2, %0" : "=m" (toc));
     __asm__ volatile ("std 30, %0" : "=m" (r30));
+    __asm__ volatile ("std 31, %0" : "=m" (r31));
     __asm__ ("mr %0, 1" : "=r" (stackref) : );
     {
         SLP_SAVE_STATE(stackref, stsizediff);
         __asm__ volatile (
             "mr 11, %0\n"
             "add 1, 1, 11\n"
+            "add 31, 31, 11\n"
             : /* no outputs */
             : "r" (stsizediff)
             : "11"
             );
         SLP_RESTORE_STATE();
     }
+    __asm__ volatile ("ld 31, %0" : : "m" (r31));
     __asm__ volatile ("ld 30, %0" : : "m" (r30));
     __asm__ volatile ("ld 2, %0" : : "m" (toc));
     __asm__ volatile ("" : : : REGS_TO_SAVE);

But to be honest it probably won't, or would be extremely fragile. Someone needs to look at the actual generated code, it may be possible to add -save-temps to CFLAGS and then find those .s files somewhere.

@andypost
Copy link

I did it but no idea how to read it yet

greenlet-i-s.zip

@andypost
Copy link

and another one using second patch (add)
greenlet-i-s-2.zip

@andypost
Copy link

And using second patch tests for py3 pass!

@awilfox
Copy link

awilfox commented Oct 27, 2018

I'm not sure if that's the proper way to fix it; r31 is a non-volatile local variable in ELFv1 ABI which is still used by glibc on ppc64. It is that way in ELFv2 as well, however, it is used as a frame pointer when necessary.

@snaury
Copy link
Contributor

snaury commented Oct 28, 2018

I'm not sure if that's the proper way to fix it

It's not, it's just a bandaid that may be useful for alpine linux in a very specific configuration (e.g. when compiling py3 version of greenlet), but not for the mainline greenlet. If the compiler is choosing r31 as a basis for stack access and there's no way to clobber it, then it's essentially game over (to save/restore it manually you'd need access to the stack, hence the chicken and egg problem).

However a similar trick is used in x86 (where ebp/rbp is both manually saved/restored and adjusted on stack switch). On x86 it's a similar situation where rbp may be used as a general purpose register, however in practice there's not much compiler may use it in slp_switch for, and so it works in practice (fingers crossed). So it may actually work well on ppc64, time will tell.

@andypost
Copy link

Meanwhile another workaround was suggested alpinelinux/aports#5518

@jamadden
Copy link
Contributor

There have been standalone builds on ppc64le systems on Travis CI for some time now, running on Ubuntu 18.04.

There are now also builds using the manylinux2014 PPC64LE environment, which is derived from CentOS 7.

I am hopeful this is no longer an issue, since there haven't been any updates since late 2018, so I'm going to close it.

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

5 participants