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

uvloop fails to import on SPARC64: OverflowError: Python int too large to convert to C long #427

Closed
mgorny opened this issue Jul 28, 2021 · 5 comments · Fixed by #493
Closed

Comments

@mgorny
Copy link

mgorny commented Jul 28, 2021

  • uvloop version: 4b803b1
  • Python version: 3.9.5
  • Platform: Gentoo Linux
  • Can you reproduce the bug with PYTHONASYNCIODEBUG in env?: yes
  • Does uvloop behave differently from vanilla asyncio? How?: n/a

When built on SPARC64, the uvloop module fails to load:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/mgorny/uvloop/uvloop/__init__.py", line 7, in <module>
    from .loop import Loop as __BaseLoop  # NOQA
  File "uvloop/includes/stdlib.pxi", line 138, in init uvloop.loop
    cdef uint64_t MAIN_THREAD_ID = <uint64_t><int64_t>threading.main_thread().ident
OverflowError: Python int too large to convert to C long

From my short testing, it seems that the Python thread identifiers on sparc are simply bigger than they are on my amd64 machine, and don't fit into int64_t (though I wouldn't be surprised if amd64 didn't have the same problem, just we're lucky not to experience large enough thread IDs). If I remove the intermediate int64_t cast, thing seems to work just fine. The cast seems to have been introduced in b5b4abb.

@1st1
Copy link
Member

1st1 commented Jul 28, 2021

I don't think we have resources to debug SPARC64 compatibility. But you're welcome to submit PRs to fix it!

@mgorny
Copy link
Author

mgorny commented Jul 28, 2021

Well, removing <int64_t> from that line fixes it but I'm afraid it's going to break some other platforms :-(. I've asked Gentoo arch teams to test whether its removal breaks any of the 32-bit platforms we test on but it will probably take a few days before they report back.

@stalkerg
Copy link

stalkerg commented Feb 6, 2022

@mgorny after all why do we need int64_t cast? I have no access to my sparc now :( but it should work very close to AMD64 if it's Linux (on Solaris much more different).

@mgorny
Copy link
Author

mgorny commented Feb 6, 2022

We shouldn't need it. Gentoo's running with my patch since then, and all arch teams have confirmed no regressions (at least as far as the test suite is concerned).

@fantix
Copy link
Member

fantix commented Sep 9, 2022

after all why do we need int64_t cast?

I think this was because Python < 3.7 still return long instead of unsigned long. Now that we don't support Python 3.6, it should be safe to drop the <int64_t> cast. I'll create a PR.


Cython will check if integer overflows when converting a Python int to a C integer type, and because the thread ident can be a negative number in Python 3.6, that's why we needed the <int64_t> conversion in the first place. But still it was buggy on 64-bit platforms just like what you ran into.

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 a pull request may close this issue.

4 participants