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

Add support for SuperH #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for SuperH #398

wants to merge 1 commit into from

Conversation

pietro
Copy link

@pietro pietro commented Mar 7, 2024

Based on the SH4 Software Manual and on the SH-4 generic and C specific application binary interface.

Untested because i currently don't have access to a SH4 board running Linux and I can't install python in a Debian qemu chroot.

Fixes #166.

@glaubitz
Copy link

glaubitz commented Mar 7, 2024

Tested on my SH7785LCR board. While it compiles without any problems, the testsuite actually segfaults:

I: pybuild base:305: cd /build/python-greenlet-SfE6Y1/python-greenlet-3.0.1/.pybuild/cpython3_3.12/build; python3.12 -m unittest discover -v 
test_break_ctxvars (greenlet.tests.test_contextvars.ContextVarsTests.test_break_ctxvars) ... Segmentation fault
E: pybuild pybuild:391: test: plugin distutils failed with: exit code=139: cd /build/python-greenlet-SfE6Y1/python-greenlet-3.0.1/.pybuild/cpython3_3.12/build; python3.12 -m unittest discover -v 
I: pybuild base:305: cd /build/python-greenlet-SfE6Y1/python-greenlet-3.0.1/.pybuild/cpython3_3.11/build; python3.11 -m unittest discover -v 
test_break_ctxvars (greenlet.tests.test_contextvars.ContextVarsTests.test_break_ctxvars) ... Illegal instruction
E: pybuild pybuild:391: test: plugin distutils failed with: exit code=132: cd /build/python-greenlet-SfE6Y1/python-greenlet-3.0.1/.pybuild/cpython3_3.11/build; python3.11 -m unittest discover -v

Note, I applied the patch against version 3.0.1. I did not test on master.

SLP_SAVE_STATE(stackref, stsizediff);
__asm__ volatile(
"add r15, %0\n"
"add r14, %0\n"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are mixing up source and target here.

It should be:

            "add %0, r15\n"
            "add %0, r14\n"

int *stackref, stsizediff;
__asm__ volatile("" : : : REGS_TO_SAVE);
__asm__ volatile("mov.l r14, %0" : "=m"(fp) : :);
__asm__("mov %0, r15" : "=r"(stackref));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing up source and target.

This should be:

    __asm__("mov r15, %0" : "=r"(stackref));

@glaubitz
Copy link

With the suggested changes above, all but the following tests pass.

test_unhandled_exception_in_greenlet_aborts (greenlet.tests.test_cpp.CPPTests.test_unhandled_exception_in_greenlet_aborts) ... FAIL
test_unhandled_nonstd_exception_aborts (greenlet.tests.test_cpp.CPPTests.test_unhandled_nonstd_exception_aborts) ... FAIL
test_unhandled_std_exception_aborts (greenlet.tests.test_cpp.CPPTests.test_unhandled_std_exception_aborts) ... FAIL
test_unhandled_std_exception_as_greenlet_function_aborts (greenlet.tests.test_cpp.CPPTests.test_unhandled_std_exception_as_greenlet_function_aborts) ... FAIL
(...)
test_failed_to_slp_switch_into_running (greenlet.tests.test_greenlet.TestBrokenGreenlets.test_failed_to_slp_switch_into_running) ... FAIL
(...)
test_throw_to_dead_thread_doesnt_crash_wait (greenlet.tests.test_greenlet.TestGreenlet.test_throw_to_dead_thread_doesnt_crash_wait) ... Time limit exceeded.
Threads: Waiting for only 1 --> 1
MGlets : Waiting for only 1 --> 2

However, I'm seeing those failures on SPARC as well, so I assume these are unrelated.

@glaubitz
Copy link

Also, I would prefer making the code sub-architecture-agnostic, i.e. available for all SuperH CPUs:

diff --git a/src/greenlet/slp_platformselect.h b/src/greenlet/slp_platformselect.h
index 0099efc..6096cb5 100644
--- a/src/greenlet/slp_platformselect.h
+++ b/src/greenlet/slp_platformselect.h
@@ -64,8 +64,8 @@ extern "C" {
 # include "platform/switch_aarch64_gcc.h" /* LLVM Aarch64 ABI for Windows */
 #elif defined(__GNUC__) && defined(__loongarch64) && defined(__linux__)
 # include "platform/switch_loongarch64_linux.h" /* LoongArch64 */
-#elif defined(__GNUC__) && defined(__SH4__)
-# include "platform/switch_sh4_gcc.h" /* SH4 */
+#elif defined(__GNUC__) && defined(__sh__)
+# include "platform/switch_sh_gcc.h" /* SH */
 #endif
 
 #ifdef __cplusplus

and rename the header accordingly:

src/greenlet/platform/switch_sh4_gcc.h -> src/greenlet/platform/switch_sh_gcc.h

and change the architecture name in the commit message from "SH4" to "SH".

@glaubitz
Copy link

I have incorporated all changes here now: glaubitz@e73592d

Let me know if you want me to open a PR. You are still being credited as the author, of course.

@glaubitz
Copy link

I have fixed your patch and resubmitted as #401 while keeping you as the author.

@pietro pietro force-pushed the sh4 branch 2 times, most recently from 12201ff to 4ea4a9c Compare March 20, 2024 13:47
@pietro
Copy link
Author

pietro commented Mar 20, 2024

I have fixed your patch and resubmitted as #401 while keeping you as the author.

Thanks. I didn't see you had opened a PR so I updated my branch with your suggestions.

@pietro pietro changed the title Add support for SH4 Add support for SuperH Mar 20, 2024
@glaubitz
Copy link

Can you change your commit message to:

Add support for SuperH

Fixes #166

I think the additional "CPUs" is not needed and the fixes tag is useful as it will automatically close the issue.

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

Successfully merging this pull request may close these issues.

SuperH/sh4 architecture unsupported
2 participants