Skip to content

Add FFI definitions from pythread.h #4872

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

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

Conversation

LegionMammal978
Copy link

This PR adds all definitions from pythread.h and cpython/pythread.h that haven't been deprecated or removed as of CPython 3.13. Of the functions, some are fully documented, some are undocumented but still contained in the limited API list, and some are neither documented nor in the limited API. Some of the limited-API functions, such as PyThread_get_thread_ident(), are widely used by 3rd-party code despite being undocumented, so it would be useful to include them in the bindings. The full breakdown:

  • Documented, limited API: functions PyThread_tss_alloc, PyThread_tss_create, PyThread_tss_delete, PyThread_tss_free, PyThread_tss_get, PyThread_tss_is_created, PyThread_tss_set; struct Py_tss_t.
  • Undocumented, limited API: functions PyThread_GetInfo, PyThread_acquire_lock, PyThread_acquire_lock_timed, PyThread_allocate_lock, PyThread_exit_thread, PyThread_free_lock, PyThread_get_stacksize, PyThread_get_thread_ident, PyThread_get_thread_native_id, PyThread_init_thread, PyThread_release_lock, PyThread_set_stacksize, PyThread_start_new_thread; enum PyLockStatus; typedefs PY_TIMEOUT_T, PyThread_type_lock; constants NOWAIT_LOCK, PY_LOCK_ACQUIRED, PY_LOCK_FAILURE, PY_LOCK_INTR, WAIT_LOCK.
  • Documented, non-limited API: constant Py_tss_NEEDS_INIT.
  • Undocumented, non-limited API: static PY_TIMEOUT_MAX; constant PYTHREAD_INVALID_THREAD_ID.

PyThread_get_thread_native_id() is a particularly fiddly definition due to all the OS-based #ifdefs guarding it: I've approximated them with cfg(target_os) values to the best of my ability, but it would be much easier if we could directly check that the PY_HAVE_THREAD_NATIVE_ID macro is defined. I've also gone through the PyPy and GraalPy versions of the headers to see which functions they support.

@LegionMammal978 LegionMammal978 force-pushed the add-pythread branch 3 times, most recently from 400c8c5 to 8a431d6 Compare January 24, 2025 20:27
@LegionMammal978
Copy link
Author

Hmm, I'm not entirely sure how to replicate the NATIVE_TSS_KEY_T without a billion cfg() guards. In CPython, it's the pthread_key_t provided by cpython/pthread_stubs.h:

#ifdef HAVE_PTHREAD_H
#   include <pthread.h>
#   define NATIVE_TSS_KEY_T     pthread_key_t
#elif defined(NT_THREADS)
#   define NATIVE_TSS_KEY_T     unsigned long
#elif defined(HAVE_PTHREAD_STUBS)
#   include "cpython/pthread_stubs.h"
#   define NATIVE_TSS_KEY_T     pthread_key_t
#else
#   error "Require native threads. See https://bugs.python.org/issue31370"
#endif

It might make sense just to add a carve-out for wasi here.

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the wait. The PR looks good to me, just I think we should do away with the exit function. Regarding the ci failure, libc::pthread_key_t is not defined on all platforms. I'm not sure how exactly libc defines it but it'd be best to just follow that, I think.

@LegionMammal978 LegionMammal978 force-pushed the add-pythread branch 3 times, most recently from f46fa95 to f4fceed Compare April 13, 2025 23:44
@LegionMammal978
Copy link
Author

Alright, I've replaced PyThread_exit_thread() with a comment, and I've set NATIVE_TSS_KEY_T to c_uint on WASI to match the definition in cpython/pthread_stubs.h. It looks like CPython's configure script enables pthread_stubs.h only on WASI: on all other platforms without <pthread.h>, CPython will simply refuse to build. So adding a carve-out seems to be the correct option after all.

(As for following whatever libc does, it's unfortunately not nearly as simple as extracting a few cfgs. The crate has a whole tree of different platforms, each with its own set of type and function definitions, and replicating that would require a big, finicky list of explicitly-enabled platforms.)

@mejrs
Copy link
Member

mejrs commented Apr 14, 2025

I've set NATIVE_TSS_KEY_T to c_uint on WASI to match the definition in cpython/pthread_stubs.h. It looks like CPython's configure script enables pthread_stubs.h only on WASI: on all other platforms without <pthread.h>, CPython will simply refuse to build. So adding a carve-out seems to be the correct option after all.

That sounds reasonable, can you please add a comment about that?

It might make sense just to add a carve-out for wasi here.

That does seem the simplest. Can you sprinkle a bunch of #[cfg_attr(docsrs, doc(cfg(all())))] on that so the cfg banners in the docs aren't weird?

Anyway, I've been looking at the definition of PY_TIMEOUT_MAX and i'm not sure they're correct. The constants are different between versions. Also, all the cfg attributes will make them look weird in the docs.

I think we should try to mirror the python files as much as possible...I'm happy for ideas here.

/// private helper for `PY_TIMEOUT_MAX`
const LLONG_MAX: c_longlong  = ...;

/// Note: This is a static on 3.13 and up, thank the gods
#[cfg_attr(docsrs, doc(cfg(not(PyPy))))]
#[cfg(all(not(PyPy), not(Py_3_13)))]
pub const PY_TIMEOUT_MAX: c_longlong = if cfg!(Py_3_12) {
    if cfg!(not(windows)) {
        LLONG_MAX / 1000
    } else if cfg!(windows) {
        if 0xFFFFFFFELL.saturating_mul(1000) < LLONG_MAX {
            0xFFFFFFFELL.saturating_mul(1000)
        } else {
            LLONG_MAX
        }
    } else {
        LLONG_MAX
    }
} else if cfg!(Py_3_11) {

} else if cfg!(Py_3_10) {

} else if cfg!(Py_3_9) {

} else if cfg!(Py_3_8) {

} else {
    panic!()
};

There's also a definition for 3.7 which would need to live in src/pythread.rs or be re-exported. Or just not exist, I guess - I'm not sure when we're moving on from 3.7.

@LegionMammal978
Copy link
Author

LegionMammal978 commented Apr 14, 2025

That sounds reasonable, can you please add a comment about that?

Sure, will do, once I get the other stuff sorted out.

That does seem the simplest. Can you sprinkle a bunch of #[cfg_attr(docsrs, doc(cfg(all())))] on that so the cfg banners in the docs aren't weird?

Sorry, how exactly do these doc() attributes work? (I'm guessing it has something to do with doc_auto_cfg magic.) Why should we be using cfg(all())? And how do I test what it will look like?

Anyway, I've been looking at the definition of PY_TIMEOUT_MAX and i'm not sure they're correct. The constants are different between versions.

I copied the different versions as best I could. The change in CPython 3.11 (mirrored in GraalPy 24.1.0) is what these two separate lines are for:

#[cfg(all(not(PyPy), not(Py_3_11), windows))]
pub const PY_TIMEOUT_MAX: c_longlong = (0xFFFFFFFF as c_longlong).saturating_mul(1000);
#[cfg(all(not(PyPy), Py_3_11, not(Py_3_13), windows))]
pub const PY_TIMEOUT_MAX: c_longlong = (0xFFFFFFFE as c_longlong).saturating_mul(1000);

I use a saturating_mul() here because that's precisely the meaning of the #if/#else in the header. I can move it all into a const block, if the separate definitions look too wacky.

One lingering question: If neither _POSIX_THREADS nor NT_THREADS is defined, PY_TIMEOUT_MAX is set to LLONG_MAX as a fallback. The only such target is WASI on CPython 3.11+. On WASI, cpython/pthread_stubs.h defines _POSIX_THREADS, but PY_TIMEOUT_MAX is defined before that header is included. So on CPython 3.11 and 3.12, does WASI receive the _POSIX_THREADS LLONG_MAX / 1000, or the !_POSIX_THREADS LLONG_MAX? (On CPython 3.13+, the static value is set from a C file that indirectly includes cpython/pthread_stubs.h beforehand, so it should always be LLONG_MAX / 1000.) It may be a good idea to double-check what the value is on 3.11–3.13, if you have a working WASI setup.

There's also a definition for 3.7 which would need to live in src/pythread.rs or be re-exported. Or just not exist, I guess - I'm not sure when we're moving on from 3.7.

In fact, it existed in the limited src/pythread.h up until CPython 3.13, when it was removed from the limited API (python/cpython#110217). So I figured we should keep it out of abi3 on all Python versions, the same way we omit deprecated and removed functions. But I could change that (or just note it) if you'd like.

@mejrs
Copy link
Member

mejrs commented Apr 14, 2025

Reading these header files gives me a headache. Is this constant actually useful? I'm leaning towards just omitting everything but the static version.

Sorry, how exactly do these doc() attributes work

The problem is when you have something like

#[cfg(Py_3_10)]
pub const X = 10;

#[cfg(not(Py_3_10))]
pub const X = 5;

then if (say) the environment has a python 3.11 available and pyo3 detects that, it will be marked as "this is only available on 3.10 and up". doc_cfg lets you override that, and with doc(cfg(all())) (which is always true), that flag will not show up. Wrong banners can be confusing, see #2426 for example.

Or (in this case) you can do pub const X = if cfg!(Py_3_10) { 10} else {5};

And how do I test what it will look like?

See https://github.com/PyO3/pyo3/blob/main/Contributing.md#help-write-great-docs

I figured we should keep it out of abi3 on all Python versions

I agree, I read that PR as "it was never documented as being in the stable api", therefore it isn't.

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.

2 participants