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

Make _Py_ThreadId() work on PowerPC, IBM Z, etc. #112535

Closed
Tracked by #108219
colesbury opened this issue Nov 29, 2023 · 14 comments
Closed
Tracked by #108219

Make _Py_ThreadId() work on PowerPC, IBM Z, etc. #112535

colesbury opened this issue Nov 29, 2023 · 14 comments
Assignees
Labels
3.13 bugs and security fixes topic-free-threading type-feature A feature request or enhancement

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 29, 2023

Feature or enhancement

Currently, _Py_ThreadId() is only defined in Py_GIL_DISABLED builds and does not compile on PowerPC or IBM Z. It would be convenient to make the functionality available in general: it will be useful to implement thread-safe recursive locks. (Currently our recursive lock implementations rely on the GIL for thread-safety.)

There are two parts to this:

  1. We should use __builtin_thread_pointer() when available
  2. We should fall back to a function that returns the address of a thread-local variable if no other fast implementation is available (instead of erroring out).

The fallback (2) will be a bit slower, but it provides a portable implementation.

Note that checking if __builtin_thread_pointer() is available is not trivial. __has_builtin(__builtin_thread_pointer) is not useful (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952). You need some combination of platform and GCC/clang version check.

cc @vstinner @corona10

Linked PRs

@corona10
Copy link
Member

corona10 commented Nov 30, 2023

@colesbury Can I take a look at it?

@colesbury
Copy link
Contributor Author

@corona10 - of course, thanks!

@vstinner
Copy link
Member

Note that checking if __builtin_thread_pointer() is available is not trivial. __has_builtin(__builtin_thread_pointer) is not useful (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952). You need some combination of platform and GCC/clang version check.

That's fine. We have many tests preprocessor checks like that, #if ....

corona10 added a commit to corona10/cpython that referenced this issue Dec 2, 2023
corona10 added a commit to corona10/cpython that referenced this issue Dec 2, 2023
corona10 added a commit to corona10/cpython that referenced this issue Dec 2, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Dec 4, 2023
Add also test.support.Py_GIL_DISABLED constant.
vstinner added a commit to vstinner/cpython that referenced this issue Dec 4, 2023
Add also test.support.Py_GIL_DISABLED constant.
vstinner added a commit to vstinner/cpython that referenced this issue Dec 4, 2023
Add also test.support.Py_GIL_DISABLED constant.
vstinner added a commit to vstinner/cpython that referenced this issue Dec 4, 2023
Add also test.support.Py_GIL_DISABLED constant.
vstinner added a commit that referenced this issue Dec 4, 2023
Add also test.support.Py_GIL_DISABLED constant.
corona10 added a commit to corona10/cpython that referenced this issue Dec 4, 2023
corona10 added a commit to corona10/cpython that referenced this issue Dec 5, 2023
corona10 added a commit to corona10/cpython that referenced this issue Dec 5, 2023
@corona10
Copy link
Member

corona10 commented Dec 8, 2023

@colesbury Do we need to support more platforms or we can close the issue?

@colesbury
Copy link
Contributor Author

I think we still want the cross-platform fallback that returns the address of a thread-local variable. That will probably need to be implemented in a .c file because accessing thread-local variables across libraries can cause issues on some platforms.

@vstinner
Copy link
Member

You may have a look at _PyThreadState_GET() which is implemented by reading a thread local variable. If Py_BUILD_CORE_MODULE macro is defined, the function is implemented with an opaque function call: implementation for libraries.

@corona10
Copy link
Member

Got it, I will send a PR soon.

I am going to submit the PR early next week :)

corona10 pushed a commit that referenced this issue Dec 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue Dec 15, 2023
corona10 pushed a commit to corona10/cpython that referenced this issue Dec 15, 2023
corona10 added a commit that referenced this issue Dec 18, 2023
@corona10
Copy link
Member

corona10 commented Dec 18, 2023

Okay, let's close the issue since the fallback implementation is merged.
Thank you for all your supports and contribution @colesbury, @vstinner(Awesome test codes!) and @furkanonder (RISC-V)

Now, we can support free-threaded CPython for all platforms where the fallback implementation(it only requires thread-local feature) works!

ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Add also test.support.Py_GIL_DISABLED constant.
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
U2FsdGVkX1 pushed a commit to fedora-riscv/python3.13 that referenced this issue Jun 22, 2024
See python/cpython#112535
And python/cpython#112624
And python/cpython#112751

Rather than backporting a handful of small commits, let's wait for the next release.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Add also test.support.Py_GIL_DISABLED constant.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants