Skip to content

gh-110014: Fix _POSIX_THREADS and _POSIX_SEMAPHORES usage #110139

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

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 30, 2023

  • pycore_pythread.h is now the central place to make sure that _POSIX_THREADS and _POSIX_SEMAPHORES macros are defined if available.
  • Make sure that pycore_pythread.h is included when _POSIX_THREADS and _POSIX_SEMAPHORES macros are tested.
  • PY_TIMEOUT_MAX is now defined as a constant, since its value depends on _POSIX_THREADS, instead of being defined as a macro.
  • Document the change and give hints how to fix affected code.

📚 Documentation preview 📚: https://cpython-previews--110139.org.readthedocs.build/

* pycore_pythread.h is now the central place to make sure that
  _POSIX_THREADS and _POSIX_SEMAPHORES macros are defined if
  available.
* Make sure that pycore_pythread.h is included when _POSIX_THREADS
  and _POSIX_SEMAPHORES macros are tested.
* PY_TIMEOUT_MAX is now defined as a constant, since its value
  depends on _POSIX_THREADS, instead of being defined as a macro.
* Prevent integer overflow in the preprocessor when computing
  PY_TIMEOUT_MAX_VALUE on Windows:
  replace "0xFFFFFFFELL * 1000 < LLONG_MAX"
  with "0xFFFFFFFELL < LLONG_MAX / 1000".
* Document the change and give hints how to fix affected code.
@vstinner
Copy link
Member Author

@encukou: "Oops", I have to add an exception to Tools/build/smelly.py to tolerate the new symbol PY_TIMEOUT_MAX. Before, Python only exported symbols starting with Py, _Py and __Py (macOS). Copy of my comment:

# "Legacy": some old symbols are prefixed by "PY_".
EXCEPTIONS = frozenset({
    'PY_TIMEOUT_MAX',
})

I would prefer to prefix all public symbols with Py and only have private symbols, but at least, now we have tooling to detect when new symbols with different prefix are added ;-)

@vstinner vstinner requested review from a team and encukou as code owners September 30, 2023 16:13
@vstinner vstinner merged commit 74e425e into python:main Sep 30, 2023
@vstinner vstinner deleted the internal_posix_threads branch September 30, 2023 17:25
@vstinner
Copy link
Member Author

Oh. I don't know if it's related or not, but I had to write PR gh-110155 to add many <unistd.h>. WASM/WASI builds were badly broken :-(

@@ -1843,6 +1843,10 @@
[function.PyThread_start_new_thread]
added = '3.2'

# Not mentioned in PEP 384, was implemented as a macro in Python <= 3.12
[data.PY_TIMEOUT_MAX]
Copy link
Member

Choose a reason for hiding this comment

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

@vstinner Could we instead exclude this undocumented constant from the limited API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked and apparently, it's safe to remove this constant, so I created PR gh-110217 for that.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…on#110139)

* pycore_pythread.h is now the central place to make sure that
  _POSIX_THREADS and _POSIX_SEMAPHORES macros are defined if
  available.
* Make sure that pycore_pythread.h is included when _POSIX_THREADS
  and _POSIX_SEMAPHORES macros are tested.
* PY_TIMEOUT_MAX is now defined as a constant, since its value
  depends on _POSIX_THREADS, instead of being defined as a macro.
* Prevent integer overflow in the preprocessor when computing
  PY_TIMEOUT_MAX_VALUE on Windows:
  replace "0xFFFFFFFELL * 1000 < LLONG_MAX"
  with "0xFFFFFFFELL < LLONG_MAX / 1000".
* Document the change and give hints how to fix affected code.
* Add an exception for PY_TIMEOUT_MAX  name to smelly.py
* Add PY_TIMEOUT_MAX to the stable ABI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants