Skip to content

Build failure without thread local support #127865

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

Closed
velemas opened this issue Dec 12, 2024 · 13 comments
Closed

Build failure without thread local support #127865

velemas opened this issue Dec 12, 2024 · 13 comments
Labels
build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-unsupported type-bug An unexpected behavior, bug, or error

Comments

@velemas
Copy link
Contributor

velemas commented Dec 12, 2024

Bug report

Bug description:

When building environments without thread local support, this build failure occurs:

~/cpython-3.13.1/Python/import.c:750: error: argument of type "PyMutex" is incompatible with parameter of type "PyThread_type_lock"
      PyThread_acquire_lock(EXTENSIONS.mutex, WAIT_LOCK);
                            ^
~/cpython-3.13.1/Python/import.c:760: error: argument of type "PyMutex" is incompatible with parameter of type "PyThread_type_lock"
      PyThread_release_lock(EXTENSIONS.mutex);
                            ^
~/cpython-3.13.1/Python/import.c:769: error: argument of type "PyMutex" is incompatible with parameter of type "PyThread_type_lock"
      PyThread_acquire_lock(EXTENSIONS.mutex, WAIT_LOCK);
                            ^
~/cpython-3.13.1/Python/import.c:774: error: argument of type "PyMutex" is incompatible with parameter of type "PyThread_type_lock"
      PyThread_release_lock(EXTENSIONS.mutex);

The issue is caused by incomplete change introduced with the commit 628f6eb from the PR #112207.

CPython versions tested on:

3.13

Operating systems tested on:

Other

Linked PRs

@velemas velemas added the type-bug An unexpected behavior, bug, or error label Dec 12, 2024
@ZeroIntensity ZeroIntensity added interpreter-core (Objects, Python, Grammar, and Parser dirs) build The build process and cross-build OS-unsupported labels Dec 12, 2024
@ZeroIntensity
Copy link
Member

I suspect the fix is to just change that to PyMutex_Lock, but which system is this? We explicitly dropped support for systems without multithreading back in 3.7, but I'm not sure if the ability to have thread-locals falls under that umbrella.

@skirpichev skirpichev added the pending The issue will be closed if no feedback is provided label Dec 12, 2024
@velemas
Copy link
Contributor Author

velemas commented Dec 12, 2024

@ZeroIntensity thread-locals are not necessary for multithreading.
The affected system is a mainframe.

@ZeroIntensity ZeroIntensity removed the pending The issue will be closed if no feedback is provided label Dec 12, 2024
@colesbury
Copy link
Contributor

It's good to fix the erroneous calls to PyThread_acquire_lock, but I think you'll still run into:

cpython/Python/pystate.c

Lines 74 to 78 in 487fdbe

#ifdef HAVE_THREAD_LOCAL
return _Py_tss_tstate;
#else
// XXX Fall back to the PyThread_tss_*() API.
# error "no supported thread-local variable storage classifier"

@colesbury
Copy link
Contributor

@velemas what compiler and system are you using?

  • Python requires C11 (without optional features) since 3.11
  • _Thread_local is a non-optional part of C11. As far as I can tell it's independent of _STDC_NO_THREADS_. Some relevant discussion on Stack Overflow.

That makes me think we should adjust the condition in pyport.h to remove the _STDC_NO_THREADS_ guard:

cpython/Include/pyport.h

Lines 508 to 509 in 487fdbe

# elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_THREADS__)
# define _Py_thread_local _Thread_local

@velemas
Copy link
Contributor Author

velemas commented Dec 12, 2024

@colesbury It is the local mainframe system compiler based on LLVM with C11 support but all C11 threading features are still missing there.

@velemas
Copy link
Contributor Author

velemas commented Dec 12, 2024

It's good to fix the erroneous calls to PyThread_acquire_lock, but I think you'll still run into:

cpython/Python/pystate.c

Lines 74 to 78 in 487fdbe

#ifdef HAVE_THREAD_LOCAL
return _Py_tss_tstate;
#else
// XXX Fall back to the PyThread_tss_*() API.
# error "no supported thread-local variable storage classifier"

That part is covered with local patching by falling back to PyThread_tss_*() API as the comment suggests.

@ZeroIntensity
Copy link
Member

What do you mean by "local patching"? The thread state doesn't fall back to PyThread_tss (it's a TODO comment).

@velemas
Copy link
Contributor Author

velemas commented Dec 12, 2024

@ZeroIntensity There is a port of python for the mainframe and that part is covered with the port patches from the mainframe maintainers (incl. me).

velemas added a commit to velemas/cpython that referenced this issue Dec 12, 2024
velemas added a commit to velemas/cpython that referenced this issue Dec 12, 2024
colesbury pushed a commit that referenced this issue Dec 12, 2024
…GH-127866)

This PR fixes the build issue introduced by the commit 628f6eb from
GH-112207 on systems without thread local support.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 12, 2024
…upport (pythonGH-127866)

This PR fixes the build issue introduced by the commit 628f6eb from
pythonGH-112207 on systems without thread local support.
(cherry picked from commit f823910)

Co-authored-by: velemas <10437413+velemas@users.noreply.github.com>
colesbury pushed a commit that referenced this issue Dec 12, 2024
…support (GH-127866) (GH-127882)

This PR fixes the build issue introduced by the commit 628f6eb from
GH-112207 on systems without thread local support.
(cherry picked from commit f823910)

Co-authored-by: velemas <10437413+velemas@users.noreply.github.com>
@colesbury
Copy link
Contributor

Thanks!

@ZeroIntensity
Copy link
Member

Do you think it's worth trying to fix (and test) building Python without thread locals? Using PyThread_tss shouldn't be too difficult for the thread state, but I'm not sure what other cases exist.

@colesbury
Copy link
Contributor

I think the opposite: we should get rid of HAS_THREAD_LOCAL and move the #error message to pyport.h:

  • All the supported platforms support thread local variables
  • C11 (and C23) have thread local as part of the language
  • We require C11 since Python 3.11

I don't think a PyThread_tss fallback is worth the maintenance effort just to support unnamed hardware from an unnamed company 👁️ 🐝 M.

There was a proposal WG14 N2291 for C23 to make _Thread_local dependent on __STDC_NO_THREADS__. It wasn't accepted. (I bet you can guess the author's company!)

@ZeroIntensity
Copy link
Member

Now I'm curious what "unnamed company" has against thread locals!

Anyways, removing the macro seems reasonable if everything we support must have thread locals. It might be worth deprecating PyThread_tss too.

@velemas
Copy link
Contributor Author

velemas commented Dec 13, 2024

I think the opposite: we should get rid of HAS_THREAD_LOCAL and move the #error message to pyport.h:

* All the [supported platforms](https://peps.python.org/pep-0011/#support-tiers) support thread local variables

* C11 (and C23) have thread local as part of the language

* We require C11 since Python 3.11

I don't think a PyThread_tss fallback is worth the maintenance effort just to support unnamed hardware from an unnamed company 👁️ 🐝 M.

There was a proposal WG14 N2291 for C23 to make _Thread_local dependent on __STDC_NO_THREADS__. It wasn't accepted. (I bet you can guess the author's company!)

It is not 👁️ 🐝 M. The mainframe system was originally developed by Siemens. But I guess all mainframes have their hardships supporting thread locals.

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this issue Jan 8, 2025
…upport (pythonGH-127866)

This PR fixes the build issue introduced by the commit 628f6eb from
pythonGH-112207 on systems without thread local support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-unsupported type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

4 participants